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

Fetch calico manifests from release artifacts #2149

Merged

Conversation

CecileRobertMichon
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon commented Mar 4, 2022

What type of PR is this?

What this PR does / why we need it: Currently, we fetch Calico manifest from https://docs.projectcalico.org/v3.22/manifests/ with kustomize and add patches to make it work for Azure (since #2021). The issue is that https://docs.projectcalico.org/v3.22/manifests/ is not pinned to a specific release, so the manifests are mutable. This means make verify will start failing whenever a new release of Calico causes the manifest to be updated and we'll be forced to update Calico to fix it. To be able to update the version of Calico on our own schedule, we need another level of indirection so we keep a copy of the source manifest in the repo and use it with kustomize to generate the Calico spec.

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 #

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:

Fetch calico manifests from release artifacts

@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. labels Mar 4, 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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 4, 2022
.gitignore Show resolved Hide resolved

.PHONY: fetch-calico-manifests
fetch-calico-manifests: ## Get Calico release manifests and unzip them.
@echo "Fetching Calico release manifests from release artifacts, this might take a minute..."
Copy link
Contributor

Choose a reason for hiding this comment

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

does this get fetched everytime? can us a file locally prevent that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everytime. The issue I'm trying to deal with is having to fetch it everytime vs. making sure generated files aren't outdated. If we don't fetch it how do we know the base manifest wasn't manually edited?

Copy link
Contributor

Choose a reason for hiding this comment

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

can we drop a symlink to a versioned file name, to check if it is been downloaded and do a diff with previous version on the file to determine if it was changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do a diff with previous version on the file

Are you saying looking at a previous git version to compare the file? our CI just checks out the branch it's running on, we don't have any precedent with comparing things in previous versions... If I make it compare with "main" it wouldn't be accurate once this gets put into a release branch if we want to do a backport and the main file has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looking at the durations in job history for make verify (https://prow.k8s.io/job-history/gs/kubernetes-jenkins/pr-logs/directory/pull-cluster-api-provider-azure-verify) it doesn't look like this is adding that much extra time compared to previous runs, we're talking about ~ 1 minute difference so if we can't find a way around it I think we can take the hit

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking from a developer experience perspective since generate flavors calls this:

generate-flavors: $(KUSTOMIZE) generate-addons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the diff on my machine (it's pretty slow so might be faster for others):

➜  cluster-api-provider-azure git:(main) ✗ time make generate-flavors
/Users/cerobert/go/src/sigs.k8s.io/cluster-api-provider-azure/hack/tools/bin/kustomize-v4.5.2 build templates/addons/metrics-server > templates/addons/metrics-server/metrics-server.yaml
/Users/cerobert/go/src/sigs.k8s.io/cluster-api-provider-azure/hack/tools/bin/kustomize-v4.5.2 build templates/addons/calico > templates/addons/calico.yaml
/Users/cerobert/go/src/sigs.k8s.io/cluster-api-provider-azure/hack/tools/bin/kustomize-v4.5.2 build templates/addons/calico-ipv6 > templates/addons/calico-ipv6.yaml
./hack/gen-flavors.sh
make generate-flavors  37.28s user 2.41s system 98% cpu 40.261 total


➜  cluster-api-provider-azure git:(fix-calico-fetch) time make generate-flavors
Fetching Calico release manifests from release artifacts, this might take a minute...
wget -qO- https://github.com/projectcalico/calico/releases/download/v3.22.1/release-v3.22.1.tgz | tar xz release-v3.22.1/manifests/calico-vxlan.yaml release-v3.22.1/manifests/calico-policy-only.yaml
mv release-v3.22.1/manifests/calico-vxlan.yaml templates/addons/calico
mv release-v3.22.1/manifests/calico-policy-only.yaml templates/addons/calico-ipv6
/Users/cerobert/go/src/sigs.k8s.io/cluster-api-provider-azure/hack/tools/bin/kustomize-v4.5.2 build templates/addons/metrics-server > templates/addons/metrics-server/metrics-server.yaml
/Users/cerobert/go/src/sigs.k8s.io/cluster-api-provider-azure/hack/tools/bin/kustomize-v4.5.2 build templates/addons/calico > templates/addons/calico.yaml
/Users/cerobert/go/src/sigs.k8s.io/cluster-api-provider-azure/hack/tools/bin/kustomize-v4.5.2 build templates/addons/calico-ipv6 > templates/addons/calico-ipv6.yaml
./hack/gen-flavors.sh
make generate-flavors  43.69s user 4.54s system 49% cpu 1:36.70 total

I was using the make verify job for comparison because it runs a full make generate (which includes make generate-flavors)

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to add about six seconds to the related make targets, at least for me. I think that's acceptable.

@CecileRobertMichon CecileRobertMichon changed the title [WIP] Fetch calico manifests from release artifacts Fetch calico manifests from release artifacts Mar 4, 2022
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed 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. labels Mar 4, 2022
@CecileRobertMichon
Copy link
Contributor Author

/retest

@CecileRobertMichon
Copy link
Contributor Author

Gathering make verify duration data

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

1 similar comment
@CecileRobertMichon
Copy link
Contributor Author

Gathering make verify duration data

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

@CecileRobertMichon
Copy link
Contributor Author

rebased on top of #2147

@CecileRobertMichon
Copy link
Contributor Author

/retest

Copy link
Contributor

@mboersma mboersma 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 k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2022
@CecileRobertMichon
Copy link
Contributor Author

@jsturtevant thoughts?

@jsturtevant
Copy link
Contributor

/lgtm

@CecileRobertMichon
Copy link
Contributor Author

/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 Mar 8, 2022
@k8s-ci-robot k8s-ci-robot merged commit 789c263 into kubernetes-sigs:main Mar 8, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.3 milestone Mar 8, 2022
@CecileRobertMichon
Copy link
Contributor Author

/cherry-pick release-1.2

@k8s-infra-cherrypick-robot

@CecileRobertMichon: #2149 failed to apply on top of branch "release-1.2":

Applying: Fetch calico manifests from release artifacts
.git/rebase-apply/patch:3945: trailing whitespace.
      - watch 
.git/rebase-apply/patch:8450: trailing whitespace.
      - watch 
.git/rebase-apply/patch:4539: new blank line at EOF.
+
.git/rebase-apply/patch:8997: new blank line at EOF.
+
warning: 4 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	Makefile
M	templates/addons/calico-ipv6/kustomization.yaml
M	templates/addons/calico/kustomization.yaml
Falling back to patching base and 3-way merge...
Auto-merging templates/addons/calico/kustomization.yaml
CONFLICT (content): Merge conflict in templates/addons/calico/kustomization.yaml
Auto-merging templates/addons/calico-ipv6/kustomization.yaml
CONFLICT (content): Merge conflict in templates/addons/calico-ipv6/kustomization.yaml
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fetch calico manifests from release artifacts
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.2

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.

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.

5 participants