Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Monolithic Control Service POC #3590

Merged
merged 6 commits into from
Jan 10, 2020
Merged

Monolithic Control Service POC #3590

merged 6 commits into from
Jan 10, 2020

Conversation

scrye
Copy link
Contributor

@scrye scrye commented Jan 8, 2020

This commit introduces a monolithic control service server which
aggregates BS, CS, and PS services functionality.

The monolith removes the need for several intra-AS RPC calls (e.g., a BS
no longer needs to push crypto and segments to the CS and PS) and
removes the need for multiple database instances.

The current implementation is just a POC, and is inefficient. Some calls
from the monolithic process go via the SCION RPC mechanism to itself,
and the Certificate Service modules within the monolith still use the
local SCIOND for paths even though that SCIOND goes back to the Path
Service modules in the same monolith. These should be fixed in future
work.

The Go Dispatcher is patched to support applications binding to a
wildcard SVC. Traffic for BS, CS, and PS is delivered to the wildcard
SVC. This is not a valid wire SVC address, and is only locally relevant
on a host supporting the monolithic control service.

Also includes generator support for the monolithic control service and a
CI end2end test.

Fixes #3584.


This change is Reviewable

@scrye scrye self-assigned this Jan 8, 2020
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 167 of 167 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @karampok, @oncilla, and @scrye)


BUILD.bazel, line 16 at r1 (raw file):

        "//go/beacon_srv:beacon_srv",
        "//go/border:border",
		"//go/cs:cs",

this file usually doesn't use tabs.


.buildkite/pipeline_buildlint.yml, line 104 at r1 (raw file):

    artifact_paths:
      - "artifacts.out/**/*"
    timeout_in_minutes: 5

make this 10 for both, this and the normal integration test above.


go/cs/main.go, line 103 at r1 (raw file):

	}
	defer log.Flush()
	defer env.LogAppStopped(common.BS, cfg.General.ID)

this should match what is used in the startup message, so use common.CS.


go/cs/main.go, line 264 at r1 (raw file):

	}
	// TODO(scrye): might need to run this too
	// msger.AddHandler(infra.SignedRev, handlers.NewRevocHandler(args))

It is probably required still. I guess you will need a dispatcher handler, that sends it to the BS and the PS handler.


go/cs/config/bs_sample.go, line 1 at r1 (raw file):

// Copyright 2019 Anapaya Systems

It seems for sample files you didn't remove the original, maybe it makes sense to do this, so that we have it only once and Git recognizes it as a move.


go/lib/integration/integration.go, line 195 at r1 (raw file):

		os.Exit(1)
	}
	cs := topo.CS["cs"+ia.FileFmt(false)+"-1"]

why is this change needed?


python/topology/config.py, line 111 at r1 (raw file):

            def_network, self.args.docker, self.args.in_docker)
        self.prvnet_gen = SubnetGenerator(
            priv_net, self.args.docker, self.args.in_docker)

Why are those newline changes needed?


python/topology/go.py, line 94 at r1 (raw file):

    def _build_br_conf(self, topo_id, ia, base, name, v):
        config_dir = '/share/conf' if self.args.docker else os.path.join(
            base, name)

Again why are those line wrap changes needed?


python/topology/go.py, line 141 at r1 (raw file):

                'Path': get_default_sciond_path(topo_id),
            },
            'cs': {

yay more overload for cs 🎉

Copy link
Contributor Author

@scrye scrye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @karampok, @lukedirtwalker, and @oncilla)


BUILD.bazel, line 16 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

this file usually doesn't use tabs.

Done.


.buildkite/pipeline_buildlint.yml, line 104 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

make this 10 for both, this and the normal integration test above.

Done.


go/cs/main.go, line 103 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

this should match what is used in the startup message, so use common.CS.

Switched to the name for the monolith service.


go/cs/main.go, line 264 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

It is probably required still. I guess you will need a dispatcher handler, that sends it to the BS and the PS handler.

Done.


go/cs/config/bs_sample.go, line 1 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

It seems for sample files you didn't remove the original, maybe it makes sense to do this, so that we have it only once and Git recognizes it as a move.

Done.


go/lib/integration/integration.go, line 195 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

why is this change needed?

It's not, I just put CS everywhere else so decided to also put it here.


python/topology/config.py, line 111 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Why are those newline changes needed?

My python linter was misconfigured. Reverted.


python/topology/go.py, line 94 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Again why are those line wrap changes needed?

Done.


python/topology/go.py, line 141 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

yay more overload for cs 🎉

🎉

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 32 of 32 files at r2.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @karampok, @lukedirtwalker, @oncilla, and @scrye)

a discussion (no related file):
in the topo generator there are still a lot of unneeded newline changes. I marked many of them with newline but please also fix the rest



go/cs/main.go, line 791 at r2 (raw file):

	for _, handler := range h.handlers {
		result := handler.Handle(r)
		if result.Status != prom.StatusOk {

I think the result can be ignored, we should remove it anyway.


go/cs/config/ps_sample.go, line 18 at r2 (raw file):

psconfigSample

Wouldn't correct camel-casing be psConfigSample ?


python/topology/config.py, line 222 at r2 (raw file):

        # from docker, but the tool runs on the host, so we resolve the bind mount here.
        conf_entry['Connection'] = conf_entry['Connection'].replace(
            '/share/cache', 'gen-cache')

still newline changes.


python/topology/config.py, line 235 at r2 (raw file):

        config.write(text)
        write_file(os.path.join(self.args.output_dir,
                                out_file), text.getvalue())

newline


python/topology/go.py, line 88 at r2 (raw file):

                base = topo_id.base_dir(self.args.output_dir)
                br_conf = self._build_br_conf(
                    topo_id, topo["ISD_AS"], base, k, v)

newline


python/topology/go.py, line 90 at r2 (raw file):

                    topo_id, topo["ISD_AS"], base, k, v)
                write_file(os.path.join(base, k, BR_CONFIG_NAME),
                           toml.dumps(br_conf))

newline


python/topology/go.py, line 163 at r2 (raw file):

                    base = topo_id.base_dir(self.args.output_dir)
                    bs_conf = self._build_bs_conf(
                        topo_id, topo["ISD_AS"], base, elem_id, elem)

newline


python/topology/go.py, line 165 at r2 (raw file):

                        topo_id, topo["ISD_AS"], base, elem_id, elem)
                    write_file(os.path.join(base, elem_id,
                                            BS_CONFIG_NAME), toml.dumps(bs_conf))

newline


python/topology/go.py, line 169 at r2 (raw file):

    def _build_bs_conf(self, topo_id, ia, base, name, infra_elem):
        config_dir = '/share/conf' if self.args.docker else os.path.join(
            base, name)

newline


python/topology/go.py, line 193 at r2 (raw file):

                    base = topo_id.base_dir(self.args.output_dir)
                    ps_conf = self._build_ps_conf(
                        topo_id, topo["ISD_AS"], base, elem_id, elem)

newline


python/topology/go.py, line 195 at r2 (raw file):

                        topo_id, topo["ISD_AS"], base, elem_id, elem)
                    write_file(os.path.join(base, elem_id,
                                            PS_CONFIG_NAME), toml.dumps(ps_conf))

newline


python/topology/go.py, line 199 at r2 (raw file):

    def _build_ps_conf(self, topo_id, ia, base, name, infra_elem):
        config_dir = '/share/conf' if self.args.docker else os.path.join(
            base, name)

newline


python/topology/go.py, line 231 at r2 (raw file):

                    base = topo_id.base_dir(self.args.output_dir)
                    co_conf = self._build_co_conf(
                        topo_id, topo["ISD_AS"], base, elem_id, elem)

newline


python/topology/go.py, line 233 at r2 (raw file):

                        topo_id, topo["ISD_AS"], base, elem_id, elem)
                    write_file(os.path.join(base, elem_id,
                                            CO_CONFIG_NAME), toml.dumps(co_conf))

newline


python/topology/go.py, line 243 at r2 (raw file):

    def _build_co_conf(self, topo_id, ia, base, name, infra_elem):
        config_dir = '/share/conf' if self.args.docker else os.path.join(
            base, name)

newline


python/topology/go.py, line 265 at r2 (raw file):

        topo = self.args.topo_dicts[ia]
        if_ids = {iface for br in topo['BorderRouters'].values(
        ) for iface in br['Interfaces']}

newline
also please fix the rest of the file :P


python/topology/topo.py, line 297 at r2 (raw file):

                'Interfaces': {
                    l_ifid: self._gen_br_intf(
                        remote, public_addr, remote_addr, attrs, remote_type)

newline


python/topology/topo.py, line 303 at r2 (raw file):

            # There is already a BR entry, add interface
            intf = self._gen_br_intf(
                remote, public_addr, remote_addr, attrs, remote_type)

newline

scrye added 4 commits January 10, 2020 11:22

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
This commit introduces a monolithic control service server which
aggregates BS, CS, and PS services functionality.

The monolith removes the need for several intra-AS RPC calls (e.g., a BS
no longer needs to push crypto and segments to the CS and PS) and
removes the need for multiple database instances.

The current implementation is just a POC, and is inefficient. Some calls
from the monolithic process go via the SCION RPC mechanism to itself,
and the Certificate Service modules within the monolith still use the
local SCIOND for paths even though that SCIOND goes back to the Path
Service modules in the same monolith. These should be fixed in future
work.

The Go Dispatcher is patched to support applications binding to a
wildcard SVC. Traffic for BS, CS, and PS is delivered to the wildcard
SVC. This is not a valid wire SVC address, and is only locally relevant
on a host supporting the monolithic control service.

Also includes generator support for the monolithic control service and a
CI end2end test.
Copy link
Contributor Author

@scrye scrye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 158 of 177 files reviewed, 18 unresolved discussions (waiting on @karampok, @lukedirtwalker, and @oncilla)

a discussion (no related file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

in the topo generator there are still a lot of unneeded newline changes. I marked many of them with newline but please also fix the rest

Done, hope I got all of them.



go/cs/config/ps_sample.go, line 18 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…
psconfigSample

Wouldn't correct camel-casing be psConfigSample ?

Ah, forgot that I wanted to get rid of these names. Done.


python/topology/config.py, line 222 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

still newline changes.

Done.


python/topology/config.py, line 235 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.


python/topology/go.py, line 88 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.


python/topology/go.py, line 90 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.


python/topology/go.py, line 163 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.


python/topology/go.py, line 165 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.


python/topology/go.py, line 169 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.


python/topology/go.py, line 193 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.


python/topology/go.py, line 195 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.


python/topology/go.py, line 199 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.


python/topology/go.py, line 231 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.


python/topology/go.py, line 233 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.


python/topology/go.py, line 243 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.


python/topology/go.py, line 265 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline
also please fix the rest of the file :P

Done.


python/topology/topo.py, line 297 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.


python/topology/topo.py, line 303 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

newline

Done.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 19 of 22 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @karampok, @oncilla, and @scrye)


python/topology/go.py, line 381 at r3 (raw file):

            config_file_path = os.path.join(elem_dir, DISP_CONFIG_NAME)
            write_file(config_file_path, toml.dumps(
                self._build_disp_conf("dispatcher")))

still :trollface:

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @karampok and @oncilla)

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @karampok and @oncilla)

@scrye scrye merged commit 76bb67e into scionproto:master Jan 10, 2020
@scrye scrye deleted the pubpr-cs-monolith branch January 10, 2020 11:16
lukedirtwalker added a commit to lukedirtwalker/scion that referenced this pull request Jan 14, 2020
This was broken with the monolith service commit:
scionproto#3590
@lukedirtwalker lukedirtwalker mentioned this pull request Jan 14, 2020
lukedirtwalker added a commit that referenced this pull request Jan 14, 2020
This was broken with the monolith service commit:
#3590
@scrye scrye mentioned this pull request Jan 15, 2020
scrye added a commit that referenced this pull request Jan 15, 2020
This reverts the metric changes accidentally introduced by
#3590.

Fixes #3610.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explore having a single SCION Control Service
2 participants