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.

Sign up for GitHub

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

Jump to bottom

Use Helm to install Calico CNI in e2e tests instead of ClusterResourceSets #2495

Merged
merged 4 commits into from
Nov 30, 2022
Merged

Use Helm to install Calico CNI in e2e tests instead of ClusterResourceSets #2495

merged 4 commits into from
Nov 30, 2022

Conversation

CecileRobertMichon
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon commented Jul 22, 2022 •
edited
Loading

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR replaces the existing ClusterResourceSet-dependent solution for automating calico CNI delivery onto test clusters (and reference cluster templates) with a helm solution. For the private cluster scenario, we're keeping the CRS-dependent solution as a practical way to install the CNI from the management cluster (we have no direct access to the apiserver to helm install gestures and we don't yet have a E2E solution to indirectly access the apiserver in such scenarios).

This work is a continuation of @jackfrancis' work in #2334.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2063
Contributes to #2179

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:

Use Helm to install Calico CNI

@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/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. 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 Jul 22, 2022
@jackfrancis
Copy link
Contributor

oooh 👀👀👀👀👀👀👀

jackfrancis
jackfrancis reviewed Jul 22, 2022
View reviewed changes
test/e2e/helpers.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2022
Jont828
Jont828 reviewed Jul 25, 2022
View reviewed changes
test/e2e/helpers.go Show resolved Hide resolved
test/e2e/cni.go Outdated Show resolved Hide resolved
test/e2e/cni.go Outdated Show resolved Hide resolved
@CecileRobertMichon CecileRobertMichon force-pushed the calico-helm branch 2 times, most recently from 7e2d23e to 2a7b7fc Compare July 26, 2022 20:35
@CecileRobertMichon
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2022
@CecileRobertMichon CecileRobertMichon force-pushed the calico-helm branch 4 times, most recently from 9b07dca to 5706814 Compare July 28, 2022 20:47
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2022
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 28, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2022
jackfrancis
jackfrancis reviewed Nov 28, 2022
View reviewed changes
Tiltfile Show resolved Hide resolved
jackfrancis
jackfrancis reviewed Nov 28, 2022
View reviewed changes
docs/book/src/topics/addons.md

For Windows nodes, you also need to copy the kubeadm-config configmap to the calico-system namespace so the calico-node-windows Daemonset can find it:

```bash
Copy link
Contributor

Choose a reason for hiding this comment

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

yikes, is there no automated way to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not AFAIK but let me know if you think of a better way, this is hopefully a very temporary workaround #2495 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

The only thing I can think of is doing this automatically in the capz control loop. That's not better. :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marosset @jsturtevant are there still plans to fix this for 1.26?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to wait until containerd v1.7 drops :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's address this as a separate change

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm

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 due to using the calico operator. The images were build with the release manifests they publish on github. We could probably clean the images to remove this but there are better ways to handle a lot of this once containerd 1.7 lands.

@CecileRobertMichon
Copy link
Contributor Author

/milestone v1.7

@k8s-ci-robot k8s-ci-robot modified the milestones: next, v1.7 Nov 29, 2022
@CecileRobertMichon
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-optional
/test pull-cluster-api-provider-azure-capi-e2e
/test pull-cluster-api-provider-azure-e2e-workload-upgrade
/test pull-cluster-api-provider-azure-conformance
/test pull-cluster-api-provider-azure-e2e

@CecileRobertMichon
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-optional
/test pull-cluster-api-provider-azure-e2e-workload-upgrade
/test pull-cluster-api-provider-azure-conformance

@CecileRobertMichon
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-csi-migration

@k8s-ci-robot
Copy link
Contributor

@CecileRobertMichon: The following tests 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-csi-migration 4726be8 link false /test pull-cluster-api-provider-azure-e2e-csi-migration
pull-cluster-api-provider-azure-e2e-optional 4726be8 link false /test pull-cluster-api-provider-azure-e2e-optional

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

Looks like some flakes on machine provisioning

/retest

@jackfrancis jackfrancis mentioned this pull request Nov 30, 2022
Merged
3 tasks
jackfrancis
jackfrancis reviewed Nov 30, 2022
View reviewed changes
scripts/ci-entrypoint.sh

# Add FeatureOverride for ChecksumOffloadBroken in FelixConfiguration.
# This is the recommended workaround for https://github.com/projectcalico/calico/issues/3145.
"${KUBECTL}" apply -f "${REPO_ROOT}"/templates/addons/calico/felix-override.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do the Windows foo here as well for Windows scenarios that use ci-entrypoint?

@jackfrancis
Copy link
Contributor

/lgtm
/approve

take some well deserved 🏖️ @CecileRobertMichon 😎

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

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 Nov 30, 2022
@k8s-ci-robot k8s-ci-robot merged commit 1d3194a into kubernetes-sigs:main Nov 30, 2022
This was referenced Nov 30, 2022
Merged
Merged
Merged
This was referenced Dec 13, 2022
Merged
Merged
@CecileRobertMichon CecileRobertMichon mentioned this pull request Dec 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marosset marosset marosset left review comments

@Jont828 Jont828 Jont828 left review comments

@jackfrancis jackfrancis jackfrancis left review comments

@jsturtevant jsturtevant jsturtevant left review comments

Assignees

@jackfrancis jackfrancis

@CecileRobertMichon CecileRobertMichon

@Jont828 Jont828

@marosset marosset

@sonasingh46 sonasingh46

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. lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.

Update e2e tests to verify all system pods are ready before executing tests
7 participants
@CecileRobertMichon @jackfrancis @k8s-ci-robot @jsturtevant @Jont828 @marosset @sonasingh46