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

Introduce the concept of a package namespace #169

Merged
merged 2 commits into from
Jul 21, 2021

Conversation

kegsay
Copy link
Member

@kegsay kegsay commented Jul 20, 2021

This is the beginning of the work for #119.

  • Add the concept of PackageNamespace to config.Complement. Set to "pkg" for now.
  • Update all filters to look for a matching package namespace when creating blueprints
    and containers.
  • Cleanup docker.Builder to read from the config more and from copies of config fields less.

This change should be invisible to any existing Complement users.

Background:

Previously, Complement assumed that each container could be uniquely
identified by the combination of deployment_namespace + blueprint_name + hs_name.
For example, if you were running BlueprintAlice then a unique string might look like
5_alice_hs1 where 5 is an atomic, monotonically increasing integer incremented when
Deploy() is called (see #113 for more info).

In a parallel by default world this is no longer true because the deployment_namespace
is not shared between different test processes. This means we cannot co-ordinate non-clashing
namespaces like before. Instead, we will bring in another namespace for the test process
(which in #119 will be on a per-package basis, hence the name PackageNamespace).

As of this PR, literally everything Complement makes (images, containers, networks, etc) are prefixed with
this package namespace, which allows multiple complement instances to share the same underlying
docker daemon, with caveats:

  • Creating CA certificates will race and needs a lockfile to prevent 2 processes trying to create
    the certificate at the same time.
  • Complement federation servers cannot run together due to trying to bind to :8448 at the same time.

That being said, this PR should enable the parallelisation of a number of CS API only tests,
which will come in another PR.

This is the beginning of the work for #119.

 - Add the concept of `PackageNamespace` to `config.Complement`. Set to `""` for now.
 - Update all filters to look for a matching package namespace when creating blueprints
   and containers.
 - Cleanup `docker.Builder` to read from the config more and from copies of config fields less.

This change should be invisible to any existing Complement users.

Background:

Previously, Complement assumed that each container could be uniquely
identified by the combination of `deployment_namespace + blueprint_name + hs_name`.
For example, if you were running `BlueprintAlice` then a unique string might look like
`5_alice_hs1` where `5` is an atomic, monotonically increasing integer incremented when
`Deploy()` is called (see #113 for more info).

In a parallel by default world this is no longer true because the `deployment_namespace`
is not shared between different test processes. This means we cannot co-ordinate non-clashing
namespaces like before. Instead, we will bring in another namespace for the test process
(which in #119 will be on a per-package basis, hence the name `PackageNamespace`).

As of this PR, literally everything Complement makes (images, containers, networks, etc) are prefixed with
this package namespace, which allows multiple complement instances to share the same underlying
docker daemon, with caveats:
 - Creating CA certificates will race and needs a lockfile to prevent 2 processes trying to create
  the certificate at the same time.
 - Complement federation servers cannot run together due to trying to bind to `:8448` at the same time.

That being said, this PR should enable the parallelisation of a number of CS API only tests,
which will come in another PR.
Copy link
Contributor

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

Makes sense, LGTM.

@kegsay kegsay merged commit 644ae7c into master Jul 21, 2021
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to review this. The idea is sound, and this PR forms a good foundation for the proposed (and even slightly tweaked) package scheme.

LGTM.

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.

3 participants