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

Allow for a separate API model per variant #588

Merged
merged 1 commit into from
Dec 16, 2019
Merged

Allow for a separate API model per variant #588

merged 1 commit into from
Dec 16, 2019

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Dec 13, 2019

Pull #578 added support for build variants to support different features and
use cases, starting with aws-k8s and aws-dev. These variants use different
software, and therefore need different configuration. This change allows for
having a different API model (and default values) per variant.

The model was moved to workspaces/models. This crate actually contains the
model for each variant in separate subdirectories. Cargo's build.rs is used
to symlink in the proper source code before build.

The "proper" source code is chosen using the VARIANT environment variable. The
build system passes this through using the value of BUILDSYS_VARIANT, which is
set in the top-level Makefile.toml and can be overridden on the command-line.
For local cargo builds, the developer must set the VARIANT environment
variable; we don't currently want to choose a default model.

To start out, the aws-k8s and aws-dev models (and related default values) are
copies, with aws-dev having the Kubernetes-related values removed. The next
step is to figure out an appropriate method for sharing structure; until then,
we'll need to duplicate most changes.


Implementation notes:

  • I had to add the SettingsResponse structure, and update the related methods in apiserver, because it no longer owns the Settings structure and so can't define traits on it. No change in behavior.
  • No changes to the model or modeled_types, other than removing unused types from the aws-dev model.
  • Same for defaults; they moved up into the models crate, and no changes other than removing unused k8s settings from aws-dev.

Testing done:

All unit tests in workspaces/ pass (for both aws-k8s and aws-dev variants).

First, I built with the default BUILDSYS_VARIANT, resulting in an aws-k8s build, and launched it as an AMI. systemctl status showed running, it joined my Kubernetes cluster, and I launched a pod OK.

Then I changed BUILDSYS_VARIANT=aws-dev, built, and launched it as an AMI. systemctl status showed running. I saw that I had the aws-dev variant's tooling, like ps, and no kubelet. I was able to docker run busybox and see it download the image. It won't actually run yet, because we need to change the models so that containerd configuration is written out; it's currently under settings.kubernetes even though it's not k8s-specific. That will be a separate PR.

@tjkirch
Copy link
Contributor Author

tjkirch commented Dec 13, 2019

This push updates a few README files to clarify the new location of defaults.toml.

workspaces/models/Cargo.toml Outdated Show resolved Hide resolved
workspaces/models/README.md Outdated Show resolved Hide resolved
workspaces/models/README.md Outdated Show resolved Hide resolved
workspaces/models/aws-dev/defaults.toml Show resolved Hide resolved
workspaces/models/aws-dev/lib.rs Outdated Show resolved Hide resolved
workspaces/models/aws-dev/lib.rs Outdated Show resolved Hide resolved
workspaces/models/aws-dev/lib.rs Outdated Show resolved Hide resolved
tools/buildsys/src/builder.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

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

☃️

@tjkirch
Copy link
Contributor Author

tjkirch commented Dec 14, 2019

This push addresses Ben's comments, the major points of which are:

  • We no longer need the src -> current symlink because we set the source path in Cargo.toml
  • We no longer rebuild ~20 packages when changing variants, by fixing tracking of variant-sensitive packages in buildsys

Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

Nice work!

🏂

Pull #578 added support for build variants to support different features and
use cases, starting with aws-k8s and aws-dev.  These variants use different
software, and therefore need different configuration.  This change allows for
having a different API model (and default values) per variant.

The model was moved to workspaces/models.  This crate actually contains the
model for each variant in separate subdirectories.  Cargo's `build.rs` is used
to symlink in the proper source code before build.

The "proper" source code is chosen using the VARIANT environment variable.  The
build system passes this through using the value of BUILDSYS_VARIANT, which is
set in the top-level Makefile.toml and can be overridden on the command-line.
For local cargo builds, the developer must set the VARIANT environment
variable; we don't currently want to choose a default model.

To start out, the aws-k8s and aws-dev models (and related default values) are
copies, with aws-dev having the Kubernetes-related values removed.  The next
step is to figure out an appropriate method for sharing structure; until then,
we'll need to duplicate most changes.
@tjkirch
Copy link
Contributor Author

tjkirch commented Dec 16, 2019

This push just expands on the comment in Dockerfile to clarify when the cache is refreshed, which helps explain its purpose.

@tjkirch tjkirch merged commit 64dfe3d into develop Dec 16, 2019
@tjkirch tjkirch deleted the fancy-models branch December 16, 2019 17:37
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