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

🐛 Skip HelmChartProxy reconciliation for paused clusters #189

Merged
merged 1 commit into from
Mar 20, 2024
Merged

🐛 Skip HelmChartProxy reconciliation for paused clusters #189

merged 1 commit into from
Mar 20, 2024

Conversation

jimmidyson
Copy link
Member

This fixes a bug leading to duplicate HelmReleaseProxy resources
following a clusterctl move that can happen due to a race between
reconciling the HelmChartProxy and moving and unpausing the Cluster.

Fixes #188.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 13, 2024
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 13, 2024
@jimmidyson jimmidyson changed the title fix: Skip HelmChartProxy reconciliation for paused clusters 🐛 Skip HelmChartProxy reconciliation for paused clusters Mar 13, 2024
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 13, 2024
@jimmidyson
Copy link
Member Author

Tested locally and this fixed my issue btw.

mboersma
mboersma reviewed Mar 13, 2024
View reviewed changes
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.

Thanks for this! It does seem like a fix.

Would you be up for writing a unit test that captures this behavior in helmchartproxy_controller_test.go? It should be mostly copy-and-paste to add a new Cluster definition with Spec.Paused == true, then a new test case that ensures the HCP is Ready (and that it isn't ready without your change).

@mboersma
Copy link
Contributor

/cc @Jont828

@jimmidyson
Copy link
Member Author

@mboersma On it! Thanks for your review!

Jont828
Jont828 reviewed Mar 13, 2024
View reviewed changes
Copy link
Contributor

@Jont828 Jont828 left a comment

Choose a reason for hiding this comment

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

Small nits, but otherwise a pretty straightforward change.

controllers/helmchartproxy/helmchartproxy_controller.go Outdated Show resolved Hide resolved
controllers/helmchartproxy/helmchartproxy_controller.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 14, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 14, 2024
@jimmidyson
Copy link
Member Author

Added test cases, tested the HRP gets created without this change, and with this change does not. Also added test case to ensure when a cluster is unpaused the HRP gets created.

@jimmidyson jimmidyson mentioned this pull request Mar 14, 2024
Merged
mboersma
mboersma approved these changes Mar 18, 2024
View reviewed changes
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 18, 2024
@mboersma
Copy link
Contributor

/assign @Jont828

@jimmidyson jimmidyson requested a review from Jont828 March 19, 2024 16:29
@Jont828
Copy link
Contributor

Jont828 commented Mar 19, 2024

Please squash your commits, otherwise looks good!

@jimmidyson
fix: Skip HelmChartProxy reconciliation for paused clusters
369c00c 
This fixes a bug leading to duplicate `HelmReleaseProxy` resources
following a `clusterctl move` that can happen due to a race between
reconciling the `HelmChartProxy` and moving and unpausing the `Cluster`.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 20, 2024
@jimmidyson
Copy link
Member Author

@Jont828 @mboersma Squashed! Please lgtm approve when you get a moment 🙏

And if you could cut a release once this is merged that would be most appreciated, it's blocking some of our e2e tests.

mboersma
mboersma approved these changes Mar 20, 2024
View reviewed changes
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 20, 2024
@jimmidyson
Copy link
Member Author

/assign @Jont828

jackfrancis
jackfrancis approved these changes Mar 20, 2024
View reviewed changes
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
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, jimmidyson, mboersma

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 20, 2024
@k8s-ci-robot k8s-ci-robot merged commit fc3a5cd into kubernetes-sigs:main Mar 20, 2024
12 checks passed
@mboersma
Copy link
Contributor

mboersma commented Mar 20, 2024 •
edited

@jimmidyson unfortunately @Jont828 is away for several days, and the other maintainers aren't familiar with the release process.

We have a bus factor problem here. I'll create an issue for the lack of release documentation. (If you or someone you know would like to be a reviewer or maintainer on CAAPH, I think we would entertain that idea too.)

Edit: see #198

@jimmidyson
Copy link
Member Author

@mboersma Thanks for the update. I'd happily help out as a maintainer if you'll have me 😅

@mboersma
Copy link
Contributor

Thanks @jimmidyson! Let's wait until later next week when the current maintainers are all available and I'll propose it.

@Jont828
Copy link
Contributor

Jont828 commented Apr 1, 2024

@jimmidyson Hey, I'm back from vacation. Super glad to hear that you're interested in getting involved! I'll reach out on Slack if you'd like to chat more.

@mboersma mboersma mentioned this pull request Apr 23, 2024
Closed
jimmidyson added a commit to nutanix-cloud-native/cluster-api-runtime-extensions-nutanix that referenced this pull request Apr 29, 2024
@jimmidyson @dkoshkin
test(e2e): Add self-hosted e2e test (#439)
3ad5620 
Creating as draft until
kubernetes-sigs/cluster-api-addon-provider-helm#189 is merged and
released.

Depends on #494.

---------

Co-authored-by: Dimitri Koshkin <dimitri.koshkin@nutanix.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mboersma mboersma mboersma approved these changes

@jackfrancis jackfrancis jackfrancis approved these changes

@Jont828 Jont828 Awaiting requested review from Jont828

Assignees

@mboersma mboersma

@jackfrancis jackfrancis

@Jont828 Jont828

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Duplicate HelmReleaseProxy resources after move - potential race
5 participants
@jimmidyson @mboersma @Jont828 @k8s-ci-robot @jackfrancis