Skip to content

fix: Fix ownership of ClusterAutoscaler resources #810

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
Merged
merged 1 commit into from
Jul 18, 2024
Merged

fix: Fix ownership of ClusterAutoscaler resources #810

merged 1 commit into from
Jul 18, 2024

Conversation

jimmidyson
Copy link
Member

@jimmidyson jimmidyson commented Jul 18, 2024 •
edited
Loading

The Cluster Autoscaler addon (either HelmChartProxy or ClusterResourceSet
) has to be deployed in the management cluster namespace and as such cannot
be owned by the workload cluster, which commonly resides in a different
namespace.

This commit fixes that and introduces a BeforeClusterDelete hook to delete
the HelmChartProxy or ClusterResourceSet rather than relying no Kubernetes
GC as was previously expected.

Tested locally. Created workload cluster in different namespace to management cluster. HCP does not have ownership set:

apiVersion: addons.cluster.x-k8s.io/v1alpha1
kind: HelmChartProxy
metadata:
  creationTimestamp: "2024-07-18T13:02:30Z"
  finalizers:
  - helmchartproxy.addons.cluster.x-k8s.io
  generation: 1
  name: cluster-autoscaler-my-docker-cluster
  namespace: test
  resourceVersion: "6238"
  uid: 0a5ebb11-5318-4114-bedd-9bcc9459f85e
spec:

And is properly cleaned up on deletion:

$ k get hcp -A
No resources found

@jimmidyson jimmidyson requested review from dkoshkin and faiq July 18, 2024 12:44
@github-actions github-actions bot added the fix label Jul 18, 2024
@jimmidyson jimmidyson force-pushed the jimmi/fix-cluster-autoscaler-workload branch from 3db2fad to 91c6585 Compare July 18, 2024 13:08
@github-actions github-actions bot added fix and removed fix labels Jul 18, 2024
@jimmidyson jimmidyson force-pushed the jimmi/fix-cluster-autoscaler-workload branch 2 times, most recently from 10adc65 to eabc689 Compare July 18, 2024 13:37
@jimmidyson jimmidyson enabled auto-merge (squash) July 18, 2024 14:13
dkoshkin
dkoshkin approved these changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@dkoshkin dkoshkin 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 fixing both CAAPH and CRS!

@jimmidyson
fix: Fix ownership of ClusterAutoscaler resources
93b16f8 
The Cluster Autoscaler addon (either HelmChartProxy or ClusterResourceSet
) has to be deployed in the management cluster namespace and as such cannot
be owned by the workload cluster, which commonly resides in a different
namespace.

This commit fixes that and introduces a BeforeClusterDelete hook to delete
the HelmChartProxy or ClusterResourceSet rather than relying no Kubernetes
GC as was previously expected.
@jimmidyson jimmidyson force-pushed the jimmi/fix-cluster-autoscaler-workload branch from eabc689 to 93b16f8 Compare July 18, 2024 15:25
@jimmidyson jimmidyson merged commit 1edc9c1 into main Jul 18, 2024
15 checks passed
@jimmidyson jimmidyson deleted the jimmi/fix-cluster-autoscaler-workload branch July 18, 2024 16:56
@github-actions github-actions bot mentioned this pull request Jul 18, 2024
Merged
jimmidyson added a commit that referenced this pull request Jul 18, 2024
@jimmidyson @github-actions
release(main): v0.13.0 (#804)
79fcaca 
🤖 I have created a release *beep* *boop*
---


## 0.13.0 (2024-07-18)

<!-- Release notes generated using configuration in .github/release.yaml
at main -->

## What's Changed
### Exciting New Features 🎉
* feat: Secure ciphers, min TLS v1.2, and disable auto TLS for etcd by
@jimmidyson in
#808
* feat: Bump default k8s version for tests to v1.29.6 by @jimmidyson in
#784
### Fixes 🔧
* fix: add omitempty to addon strategy by @dkoshkin in
#795
* fix: update CCM to 0.3.4 to fix sweet32 issue by @tuxtof in
#805
* fix: Clean up MetalLB pod security standards labels by @jimmidyson in
#807
* fix: Fix ownership of ClusterAutoscaler resources by @jimmidyson in
#810
### Other Changes
* ci: Run e2e jobs only if unit-test, lint-*, and pre-commit jobs pass
by @dlipovetsky in
#796
* ci: Enable verbose output for e2e tests by @dlipovetsky in
#797
* test: Verify ServiceLoadBalancer in e2e Docker and Nutanix tests by
@dlipovetsky in
#788
* refactor: Use CAPI conditions check where possible by @dlipovetsky in
#789
* test(e2e): Use parallel tests for providers other than Docker by
@jimmidyson in
#787

## New Contributors
* @tuxtof made their first contribution in
#805

**Full Changelog**:
v0.12.1...v0.13.0

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkoshkin dkoshkin dkoshkin approved these changes

@deepakm-ntnx deepakm-ntnx deepakm-ntnx approved these changes

@faiq faiq Awaiting requested review from faiq

@dlipovetsky dlipovetsky Awaiting requested review from dlipovetsky

@supershal supershal Awaiting requested review from supershal

@thunderboltsid thunderboltsid Awaiting requested review from thunderboltsid

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jimmidyson @dkoshkin @deepakm-ntnx