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

🌱 Generate cluster templates with kustomize #1271

Merged
merged 3 commits into from
Jun 29, 2022

Conversation

mdbooth
Copy link
Contributor

@mdbooth mdbooth commented Jun 21, 2022

What this PR does / why we need it:

This PR changes the way we generate our cluster templates for both release and e2e tests to use kustomize. We now have the following dependency structure between templates:

graph TB
  subgraph Release
    release/without-lb([Without LoadBalancer]) --> release/default([Default])
    release/external-cloud-provider([External Cloud Provider]) --> release/default
  end

  subgraph E2E Tests
    test/default([Default]) --> release/default
    test/external-cloud-provider([External Cloud Provider]) --> release/external-cloud-provider
    test/without-lb([Without LoadBalancer]) --> release/without-lb
    test/multi-az([Multi AZ]) --> test/default
    test/multi-network([Multi Network]) --> test/default
  end
Loading

Additionally, all the E2E templates now use a common set of patches to uniformly apply configuration we use in tests but not in release. This means that any change in the release templates is now reflected automatically in the E2E tests, which gives us a level of test coverage for changes to the release templates.

That said, in order to keep this PR simple the templates produced by this PR are identical to the previous templates except in the order of the objects and fields. The order difference is an artifact of using kustomize: it picks a consistent order according to its own rules. I have verified that the templates are identical by the following:

For every existing template in /templates and /test/e2e/data/infrastructure-openstack I generated a kustomization.yaml containing only a resources section specifying only the original template file, and then I built the kustomization. i.e. This is the original template whose only transformation is the re-ordering done by kustomize. I then generated templates from the new kustomize and verified that there were no diffs.

Release templates are generated by a new make target: templates.
E2E templates are generated by a new make target: e2e-templates.

I have created directories for v1alpha5 and v1alpha6, which are currently identical except for apiVersion, and we should consider adding a directory for v1alpha4 in the future as long as we support it. The intention here is to for reuse when adding tests for supported previous versions.

I have used strategic merge patches in the release templates because I find them more readable. However, in the e2e templates I have used a json patch everywhere we reference an OpenStack object. This allows the patches to be reusable across apiVersions.

/hold

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 21, 2022
@netlify
Copy link

netlify bot commented Jun 21, 2022

Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
🔨 Latest commit 122d7c1
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/62bc3094773bf500090d67d8
😎 Deploy Preview https://deploy-preview-1271--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 21, 2022
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2022
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2022
@mdbooth mdbooth force-pushed the kustomize-release branch from 880086d to 1c78102 Compare June 28, 2022 15:21
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 28, 2022
@mdbooth mdbooth marked this pull request as ready for review June 28, 2022 15:55
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2022
@k8s-ci-robot k8s-ci-robot requested a review from apricote June 28, 2022 15:55
@@ -469,3 +497,6 @@ verify-gen: generate
.PHONY: compile-e2e
compile-e2e: ## Test e2e compilation
go test -c -o /dev/null -tags=e2e ./test/e2e/suites/conformance

.PHONY: FORCE
FORCE:
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not find any way to make the target depend on all files in a directory. A target that depends on a .PHONY target will always be rebuilt. The targets which use FORCE are pattern targets, and AFAIK there is no way to make them directly .PHONY.

Makefile Outdated
templates/cluster-template-without-lb.yaml \
templates/cluster-template-external-cloud-provider.yaml

templates/cluster-template.yaml: kustomize/v1alpha6/default FORCE
Copy link
Member

Choose a reason for hiding this comment

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

This fails for me with:

$ make templates
hack/tools/bin/kustomize build "kustomize/v1alpha6/default" > "templates/cluster-template.yaml"
bash: line 1: hack/tools/bin/kustomize: No such file or directory
make: *** [Makefile:404: templates/cluster-template.yaml] Error 127
make: *** Deleting file 'templates/cluster-template.yaml'

I would suggest we add the $(KUSTOMIZE) target as a dependency, so the binary is always available

Suggested change
templates/cluster-template.yaml: kustomize/v1alpha6/default FORCE
templates/cluster-template.yaml: kustomize/v1alpha6/default FORCE $(KUSTOMIZE)

The same applies to targets templates/cluster-template-%.yaml, $(E2E_TEMPLATES_DIR)/cluster-template.yaml and $(E2E_TEMPLATES_DIR)/cluster-template-%.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated with this change.

@mdbooth mdbooth force-pushed the kustomize-release branch from 1c78102 to 6c7bb72 Compare June 29, 2022 09:11
@apricote
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2022
Copy link
Member

@tobiasgiese tobiasgiese left a comment

Choose a reason for hiding this comment

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

Also validated the changes via

git clone https://github.com/kubernetes-sigs/cluster-api-provider-openstack/ /tmp/cluster-api-provider-openstack
for template in templates/*yaml; do diff -u <(sort $template) <(sort /tmp/cluster-api-provider-openstack/$template); done

Noticed only some quoting and list syntax changes

Edit: btw, there is also a yaml differ called dyff. But as there are multiple yamls inside the manifest dyff is not able to do its proper job 😿

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth, tobiasgiese

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [mdbooth,tobiasgiese]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tobiasgiese
Copy link
Member

Edit: btw, there is also a yaml differ called dyff. But as there are multiple yamls inside the manifest dyff is not able to do its proper job

But there is ksort.
Combined I couldn't find any changes

$ for template in templates/*yaml; do dyff between --omit-header <(ksort -f $template) <(ksort -f /tmp/cluster-api-provider-openstack/$template); done
$

mdbooth added 3 commits June 29, 2022 11:59
templates/cluster-template*.yaml is now generated from kustomize in the
new kustomize/ directory.

This makes no functional changes to the release templates. The resulting
templates are identical except for the order of the objects.
The e2e cluster templates are now based on the release templates, with
explicit modifications for testing using kustomize.

The resulting templates are identical to the previous templates except
for the order of objects.
@mdbooth mdbooth force-pushed the kustomize-release branch from 6c7bb72 to 122d7c1 Compare June 29, 2022 10:59
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2022
@mdbooth
Copy link
Contributor Author

mdbooth commented Jun 29, 2022

Edit: btw, there is also a yaml differ called dyff. But as there are multiple yamls inside the manifest dyff is not able to do its proper job

But there is ksort. Combined I couldn't find any changes

$ for template in templates/*yaml; do dyff between --omit-header <(ksort -f $template) <(ksort -f /tmp/cluster-api-provider-openstack/$template); done
$

I did something like this against an unmodified repo to create a new directory (k) containing original templates which have been sorted by kustomize:

mkdir k
for i in cluster-template*.yaml; do
  echo -e "resources:\n- $i" > kustomization.yaml
  kustomize build . > k/$i
end

Then I diffed the new generated templates against the kustomize-sorted original templates. They are literally identical.

Because the original doesn't apply any patches I think this even avoids kustomize pitfalls involving incorrect API definitions, so I'm fairly confident this is a robust test.

@apricote
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2022
@mdbooth
Copy link
Contributor Author

mdbooth commented Jun 29, 2022

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 29, 2022
@k8s-ci-robot k8s-ci-robot merged commit 5caab7e into kubernetes-sigs:main Jun 29, 2022
@stephenfin stephenfin deleted the kustomize-release branch October 26, 2022 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants