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

Autogenerate per-region YAML manifests from a common template #986

Merged
merged 1 commit into from
May 21, 2020

Conversation

anguslees
Copy link
Contributor

@anguslees anguslees commented May 20, 2020

Autogenerate per-region manifests from a common template in order to
prevent drift across versions.

Also: Separate in-development (unreleased) manifest changes from the
most recent release, by creating a config/master directory. The idea
is that on each new vX.Y release, we:

  • copy the latest version of config/master to config/vX.Y
  • set version = vX.Y near top of vX.Y/manifests.jsonnet
  • re-run make generate

I have confirmed (using a yaml diff tool) that the generated manifests
are identical to latest config/v1.6, except for:

  • re-ordering of documents within the top-level YAML stream
  • re-ordering of container env array
  • "v1.6.1" docker tag (intentionally) replaced with "latest" (because
    this is config/master)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@anguslees anguslees requested a review from mogren May 20, 2020 06:25
@anguslees
Copy link
Contributor Author

Possible objections:

  • I don't like this template tool, we should use xyz instead (cool, tell me what you'd prefer)
  • I don't like the extra yaml quotes (I can pass this through an extra cleanup tool if you'd like)
  • I don't like the name 'config/next', please use config/latest or config/master for consistency with docker xor git (sg, tell me which you'd prefer)

@mogren
Copy link
Contributor

mogren commented May 20, 2020

This is great. I really want to get away from the mess of having multiple CNI version config folders.

It would be a lot better to have the current recommended version configs directly in /config. For the next release, we create a release-1.7 branch, and the /config/aws-k8s-cni.yaml file there will always have the image tag :v.1.7.x. What x is depends on the tag on that branch. That means, by default you will get the latest patch release for that branch, but if you want to stick to 1.7.0, you use the tag. No version subfolders at all, just config/*.yaml and for the latest nightly builds, config/latest/*.yaml.

Copy link
Contributor

@mogren mogren left a comment

Choose a reason for hiding this comment

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

To me /config/next feels a bit weird. How about nightly? Or is latest better, since that's kind of the default tag? This would work better for testing CNI updates as well.

This will make it a lot easier to keep these configs in sync. For example, #955 requires us to update the sample config to fully use it. It still works with the current config, but we want to enable the init container by default eventually.

Also, I think we should get rid of the readiness probe, it's just delaying the startup for no reason.

@anguslees
Copy link
Contributor Author

Sg. How do you want me to do this? One logical change per PR, or rip the band-aid off in one go?

Autogenerate per-region manifests from a common template in order to
prevent drift across versions.

Also: Separate in-development (unreleased) manifest changes from the
most recent release, by creating a `config/master` directory.  The idea
is that on each new vX.Y release, we:
- copy the latest version of config/master to config/vX.Y
- set version = `vX.Y` near top of vX.Y/manifests.jsonnet
- re-run `make generate`

I have confirmed (using a yaml diff tool) that the generated manifests
are identical to latest config/v1.6, except for:
- re-ordering of documents within the top-level YAML stream
- re-ordering of container `env` array
- "v1.6.1" docker tag (intentionally) replaced with "latest" (because
  this is config/master)
@anguslees
Copy link
Contributor Author

(Updated PR description to reflect config/next => config/master)

Copy link
Contributor

@mogren mogren left a comment

Choose a reason for hiding this comment

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

LGTM!

@mogren mogren merged commit 53f212e into aws:master May 21, 2020
anguslees added a commit to anguslees/amazon-vpc-cni-k8s that referenced this pull request May 25, 2020
Default to testing upgrades to `config/master` manifests in
`run-integration-tests.sh`.  Follow-on to aws#986.

Correct internal comment re starting version: we test upgrades from
whatever version is installed by `eks create cluster`.

Also: Switch to shell `${foo:=bar}` idiom to reduce duplication (and
chance for typos) in `foo=${foo:-bar}`.
mogren pushed a commit that referenced this pull request May 26, 2020
Default to testing upgrades to `config/master` manifests in
`run-integration-tests.sh`.  Follow-on to #986.

Correct internal comment re starting version: we test upgrades from
whatever version is installed by `eks create cluster`.

Also: Switch to shell `${foo:=bar}` idiom to reduce duplication (and
chance for typos) in `foo=${foo:-bar}`.
bnapolitan pushed a commit to bnapolitan/amazon-vpc-cni-k8s that referenced this pull request May 27, 2020
Default to testing upgrades to `config/master` manifests in
`run-integration-tests.sh`.  Follow-on to aws#986.

Correct internal comment re starting version: we test upgrades from
whatever version is installed by `eks create cluster`.

Also: Switch to shell `${foo:=bar}` idiom to reduce duplication (and
chance for typos) in `foo=${foo:-bar}`.
@mogren mogren added this to the v1.7.0 milestone Jun 24, 2020
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.

2 participants