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 calico manifests from source #2021

Merged

Conversation

CecileRobertMichon
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon commented Jan 29, 2022

What type of PR is this?

What this PR does / why we need it: Instead of keeping a static Calico manifests in the repo, this PR uses the Kustomize feature to pull a base manifest from a url to get the vxlan manifest from https://docs.projectcalico.org/v3.20/manifests/calico-vxlan.yaml and makes a few patches necessary for Azure and IPv6. This will make sure we don't drift from the recommended Calico configuration and will make updating the Calico version a lot easier.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
partly addresses #1917

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Generate calico manifests from source

@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. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 29, 2022
@CecileRobertMichon
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-full

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 29, 2022
@k8s-ci-robot k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Jan 29, 2022
Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

The diffs where a little harder than I had hoped due to the whitespace but that should be better in the future since kustomize should help standardize the final output.

name: FELIX_IPV6SUPPORT
value: "true"


Copy link
Contributor

Choose a reason for hiding this comment

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

extra line

@@ -386,6 +386,8 @@ generate-e2e-templates: $(KUSTOMIZE)
.PHONY: generate-addons
generate-addons:
$(KUSTOMIZE) build $(ADDONS_DIR)/metrics-server > $(ADDONS_DIR)/metrics-server/metrics-server.yaml
$(KUSTOMIZE) build $(ADDONS_DIR)/calico > $(ADDONS_DIR)/calico.yaml
$(KUSTOMIZE) build $(ADDONS_DIR)/calico-ipv6 > $(ADDONS_DIR)/calico-ipv6.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

This is cool to see!

@jsturtevant
Copy link
Contributor

We should also bump the Windows calico versions to sigwindowstools/calico-node:v3.22.0-hostprocess

image: sigwindowstools/calico-install:v3.20.0-hostprocess

image: sigwindowstools/calico-node:v3.20.0-hostprocess

image: sigwindowstools/calico-node:v3.20.0-hostprocess

@CecileRobertMichon
Copy link
Contributor Author

tests passed 🎉

going to squash and bump version in a separate commit

@CecileRobertMichon
Copy link
Contributor Author

/test ls

@k8s-ci-robot
Copy link
Contributor

@CecileRobertMichon: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test pull-cluster-api-provider-azure-build
  • /test pull-cluster-api-provider-azure-e2e
  • /test pull-cluster-api-provider-azure-e2e-windows-dockershim
  • /test pull-cluster-api-provider-azure-test
  • /test pull-cluster-api-provider-azure-verify

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-provider-azure-apidiff
  • /test pull-cluster-api-provider-azure-apiversion-upgrade
  • /test pull-cluster-api-provider-azure-capi-e2e
  • /test pull-cluster-api-provider-azure-ci-entrypoint
  • /test pull-cluster-api-provider-azure-conformance
  • /test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
  • /test pull-cluster-api-provider-azure-coverage
  • /test pull-cluster-api-provider-azure-e2e-exp
  • /test pull-cluster-api-provider-azure-e2e-full
  • /test pull-cluster-api-provider-azure-e2e-workload-upgrade
  • /test pull-cluster-api-provider-azure-upstream-windows-dockershim
  • /test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts
  • /test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts-serial-slow

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-provider-azure-apidiff
  • pull-cluster-api-provider-azure-build
  • pull-cluster-api-provider-azure-ci-entrypoint
  • pull-cluster-api-provider-azure-coverage
  • pull-cluster-api-provider-azure-e2e
  • pull-cluster-api-provider-azure-e2e-exp
  • pull-cluster-api-provider-azure-e2e-windows-dockershim
  • pull-cluster-api-provider-azure-test
  • pull-cluster-api-provider-azure-verify

In response to this:

/test ls

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@CecileRobertMichon
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-conformance

@CecileRobertMichon CecileRobertMichon changed the title [WIP] Generate calico manifests from source Generate calico manifests from source Feb 4, 2022
@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 Feb 4, 2022
@CecileRobertMichon
Copy link
Contributor Author

/retest

@CecileRobertMichon
Copy link
Contributor Author

/hold

ipv6 test failing after updating version

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2022
@CecileRobertMichon
Copy link
Contributor Author

/retest

@CecileRobertMichon
Copy link
Contributor Author

@jsturtevant for some reason 3.22 breaks ipv6. All tests passed with 3.20 (the current version in main). I'm thinking of reverting the bump to 3.22 to unblock this for now and investigate why 3.22 isn't working separately. However the windows templates use 3.20.0, whereas the latest 3.20 release used in the Calico 3.20 manifest is 3.20.3. Does it matter if Windows and Linux aren't on the same patch version? Is it possible to create an image for calico host process with 3.20.3? I don't see one available right now.

@jsturtevant
Copy link
Contributor

jsturtevant commented Feb 7, 2022

Does it matter if Windows and Linux aren't on the same patch version?

It is likely a good idea to keep them on the same version.

Is it possible to create an image for calico host process with 3.20.3?

Built a package for this version.

for some reason 3.22 breaks ipv6. All tests passed with 3.20 (the current version in main). I'm thinking of reverting the bump to 3.22 to unblock this for now and investigate why 3.22 isn't working separately

This makes sense. It will be easier to identify the difference between 3.20 and 3.22

@CecileRobertMichon
Copy link
Contributor Author

reverted 1.22 bump and upgraded to 1.20.4 which just came out

/test pull-cluster-api-provider-azure-e2e-full

@CecileRobertMichon
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-full

@CecileRobertMichon
Copy link
Contributor Author

/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 Feb 7, 2022
@CecileRobertMichon
Copy link
Contributor Author

prow got stuck

/retest

@jackfrancis
Copy link
Contributor

I think the commit history of the version bump and reversion is lost to me, but this idea is a huge lgtm. Since there appears to be Windows impact @jsturtevant can you assign to me after you've given a final lgtm and I'll follow along with a review after that?

@CecileRobertMichon
Copy link
Contributor Author

/retest

2nd time running into the prow timeout on the full job with no apparent test failure

@CecileRobertMichon
Copy link
Contributor Author

/assign @jsturtevant

@jsturtevant
Copy link
Contributor

/lgtm
/assign @jackfrancis

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

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Contributor

@CecileRobertMichon: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-provider-azure-e2e-full 2a705e9 link false /test pull-cluster-api-provider-azure-e2e-full

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@CecileRobertMichon
Copy link
Contributor Author

I'm going to say this is good to go based on the fact that the last full test run 8/9 tests passed. The one that failed was an AKS provisioning error, and AKS doesn't use the Calico CNI we maintain in this repo, and that the AKS spec passed separately in the exp test.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2022
@k8s-ci-robot k8s-ci-robot merged commit 6861356 into kubernetes-sigs:main Feb 9, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.1 milestone Feb 9, 2022
@jsturtevant
Copy link
Contributor

this looks to have broken the conformance jobs. We ran it early in the cycle but didn't run with latest updates

/test pull-cluster-api-provider-azure-conformance

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. area/provider/azure Issues or PRs related to azure provider 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. 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