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

🐛 fix: considers objects in kube-system for cert-manager to avoid upgrading twice #11351

Merged
merged 3 commits into from
Nov 15, 2024
Merged

🐛 fix: considers objects in kube-system for cert-manager to avoid upgrading twice #11351

merged 3 commits into from
Nov 15, 2024

Conversation

faiq
Copy link
Contributor

@faiq faiq commented Oct 29, 2024

What this PR does / why we need it:

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 #11348

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 29, 2024
@faiq
Copy link
Contributor Author

faiq commented Oct 29, 2024

area/clusterctl

@faiq faiq force-pushed the faiq/fix-cert-manager-upgrades branch from b1e4aea to 6c17c21 Compare October 29, 2024 20:49
@faiq
Copy link
Contributor Author

faiq commented Oct 29, 2024

/area clusterctl

@k8s-ci-robot k8s-ci-robot added area/clusterctl Issues or PRs related to clusterctl and removed do-not-merge/needs-area PR is missing an area label labels Oct 29, 2024
@faiq faiq mentioned this pull request Oct 29, 2024
Merged
@faiq faiq changed the title fix: considers objects in kube-system for cert-manager to avoid upgrading twice 🐛 fix: considers objects in kube-system for cert-manager to avoid upgrading twice Oct 29, 2024
neolit123
neolit123 reviewed Oct 30, 2024
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

one nit from me as well.
maybe clustercuctl can eventually keep a better inventory of the cert-manager components.
but this fix seems good to me. thanks

cmd/clusterctl/client/cluster/cert_manager.go Outdated Show resolved Hide resolved
@faiq faiq force-pushed the faiq/fix-cert-manager-upgrades branch from 6c17c21 to 903f3f4 Compare October 30, 2024 17:44
@faiq
Copy link
Contributor Author

faiq commented Oct 30, 2024

/test ?

@k8s-ci-robot
Copy link
Contributor

@faiq: The following commands are available to trigger required jobs:

  • /test pull-cluster-api-build-main
  • /test pull-cluster-api-e2e-blocking-main
  • /test pull-cluster-api-e2e-conformance-ci-latest-main
  • /test pull-cluster-api-e2e-conformance-main
  • /test pull-cluster-api-e2e-latestk8s-main
  • /test pull-cluster-api-e2e-main
  • /test pull-cluster-api-e2e-mink8s-main
  • /test pull-cluster-api-e2e-upgrade-1-31-1-32-main
  • /test pull-cluster-api-test-main
  • /test pull-cluster-api-test-mink8s-main
  • /test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-apidiff-main

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

  • pull-cluster-api-apidiff-main
  • pull-cluster-api-build-main
  • pull-cluster-api-e2e-blocking-main
  • pull-cluster-api-test-main
  • pull-cluster-api-verify-main

In response to this:

/test ?

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-sigs/prow repository.

@faiq
Copy link
Contributor Author

faiq commented Oct 30, 2024

/test pull-cluster-api-e2e-blocking-main

@faiq
Copy link
Contributor Author

faiq commented Oct 30, 2024

/test pull-cluster-api-e2e-upgrade-1-31-1-32-main

@faiq faiq force-pushed the faiq/fix-cert-manager-upgrades branch from 903f3f4 to f6d0740 Compare October 31, 2024 17:25
@faiq faiq force-pushed the faiq/fix-cert-manager-upgrades branch from f6d0740 to 317fefb Compare October 31, 2024 18:29
@faiq faiq force-pushed the faiq/fix-cert-manager-upgrades branch from 317fefb to fae02e5 Compare October 31, 2024 20:58
@faiq
Copy link
Contributor Author

faiq commented Oct 31, 2024

I reworked this commit to remove the endpoint/endpointSlices objects in shouldUpgrade now all of the code for calculating the upgrade lives in there and no longer requires the caller to have advanced knowledge of what resources it should or shouldn't include.

@dlipovetsky
Copy link
Contributor

The diff reads really easily now. Makes sense. Thank you for finding the root cause of this issue!

/lgtm

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 4, 2024
@sbueringer
Copy link
Member

Please run pull-cluster-api-e2e-main once you're happy with the PR (I want to take a look at the clusterctl upgrade test logs)

@faiq
Copy link
Contributor Author

faiq commented Nov 4, 2024

/test pull-cluster-api-e2e-main

@faiq
Copy link
Contributor Author

faiq commented Nov 4, 2024

/test pull-cluster-api-e2e-upgrade-1-31-1-32-main

@faiq
Copy link
Contributor Author

faiq commented Nov 5, 2024

/test pull-cluster-api-e2e-upgrade-1-31-1-32-main

@faiq
Copy link
Contributor Author

faiq commented Nov 5, 2024

/test pull-cluster-api-e2e-main

@dlipovetsky
Copy link
Contributor

/lgtm

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

LGTM label has been added.

Git tree hash: 45b2af3dc53e5bcd409d77601e5e73fa24534398

@hackeramitkumar
Copy link
Member

/lgtm

@sbueringer
Copy link
Member

/assign

@sbueringer
Copy link
Member

For reference:

main branch: some upgrade test:

e2e test on this PR:

@sbueringer sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 15, 2024
@sbueringer
Copy link
Member

Thx!

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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 15, 2024
@sbueringer
Copy link
Member

Feel free to cherry-pick

@k8s-ci-robot k8s-ci-robot merged commit dc1051f into kubernetes-sigs:main Nov 15, 2024
21 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.9 milestone Nov 15, 2024
@faiq
Copy link
Contributor Author

faiq commented Nov 20, 2024

/cherry-pick help

@k8s-infra-cherrypick-robot
Copy link

@faiq: cannot checkout help: error checking out "help": exit status 1 error: pathspec 'help' did not match any file(s) known to git

In response to this:

/cherry-pick help

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-sigs/prow repository.

@faiq
Copy link
Contributor Author

faiq commented Nov 20, 2024

/cherry-pick release-1.8
/cherry-pick release-1.7

@k8s-infra-cherrypick-robot
Copy link

@faiq: new pull request created: #11455

In response to this:

/cherry-pick release-1.8
/cherry-pick release-1.7

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-sigs/prow repository.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Nov 20, 2024
Merged
@k8s-infra-cherrypick-robot
Copy link

@faiq: new pull request created: #11456

In response to this:

/cherry-pick release-1.8
/cherry-pick release-1.7

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-sigs/prow repository.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Nov 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@dlipovetsky dlipovetsky dlipovetsky left review comments

@Jont828 Jont828 Awaiting requested review from Jont828

@richardcase richardcase Awaiting requested review from richardcase

Assignees

@dlipovetsky dlipovetsky

@sbueringer sbueringer

@hackeramitkumar hackeramitkumar

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/clusterctl Issues or PRs related to clusterctl 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
v1.9
Development

Successfully merging this pull request may close these issues.

cert-manager is always updated
7 participants
@faiq @k8s-ci-robot @dlipovetsky @sbueringer @hackeramitkumar @k8s-infra-cherrypick-robot @neolit123