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

helm: loadBalancerClass for Cluster Mesh APIserver #33033

Merged
merged 1 commit into from
Jun 17, 2024
Merged

helm: loadBalancerClass for Cluster Mesh APIserver #33033

merged 1 commit into from
Jun 17, 2024

Conversation

PhilipSchmid
Copy link
Contributor

  • Added loadBalancerClass Helm value for the Cluster Mesh APIserver Kubernetes Service.
  • Refactored the existing loadBalancerIP Helm value so it's clearer that it exists (instead of having it commented out in the values.yaml file).
helm: loadBalancerClass for Cluster Mesh APIserver

@PhilipSchmid PhilipSchmid requested review from a team as code owners June 10, 2024 17:46
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 10, 2024
@PhilipSchmid
Copy link
Contributor Author

Let's please backport this to 1.15 :).

sayboras
sayboras approved these changes Jun 11, 2024
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Looks good for helm changes.

@sayboras sayboras added area/clustermesh Relates to multi-cluster routing functionality in Cilium. area/helm Impacts helm charts and user deployment experience release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jun 11, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 11, 2024
@PhilipSchmid
Copy link
Contributor Author

Fixing https://github.com/cilium/cilium/actions/runs/9453106239/job/26037749772?pr=33033#step:5:602 in the next force-commit in a couple of minutes.

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/helm_cm-apiserver_lbclass_support branch from e7195d0 to 95e894d Compare June 11, 2024 20:19
@aanm aanm requested a review from giorio94 June 13, 2024 12:02
@aanm
Copy link
Member

aanm commented Jun 13, 2024

This looks more a feature than a bug fix but I'll leave it to @giorio94 to decide if we should backport it or not.

@aanm
Copy link
Member

aanm commented Jun 13, 2024

/test

@aanm aanm enabled auto-merge June 13, 2024 12:03
giorio94
giorio94 approved these changes Jun 13, 2024
View reviewed changes
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks!

@giorio94
Copy link
Member

/test

@giorio94
Copy link
Member

This looks more a feature than a bug fix but I'll leave it to @giorio94 to decide if we should backport it or not.

Hmm, I'm a bit on the fence honestly. I agree that the PR looks more as a feature than a bug fix, but on the other hand one may argue that the lack of this setting may prevent users from successfully deploying the clustermesh-apiserver in environments including multiple LB classes. Given that the change is straightforward, and has a very low risk of introducing regressions, I'd probably lean towards backporting it to v1.15 to be on the safe side (it is instead definitely not a serious enough issue to justify backporting to older stable versions).

@giorio94
Copy link
Member

I've added the backport label, feel free to remove it if you disagree.

@giorio94 giorio94 added the needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch label Jun 13, 2024
@giorio94 giorio94 removed the request for review from nathanjsweet June 13, 2024 12:23
@giorio94
Copy link
Member

Sorry for retriggering the tests 😿, I missed that they had already been triggered.

@PhilipSchmid @giorio94
helm: loadBalancerClass for Cluster Mesh APIserver
0661fb6 
* Added `loadBalancerClass` Helm value for the Cluster Mesh APIserver
  Kubernetes Service.
* Refactored the existing `loadBalancerIP` Helm value so it's clearer
  that it exists (instead of having it commented out in the values.yaml
  file).

Signed-off-by: Philip Schmid <[email protected]>
@giorio94 giorio94 force-pushed the pr/philip/helm_cm-apiserver_lbclass_support branch from 95e894d to 0661fb6 Compare June 14, 2024 15:44
@giorio94
Copy link
Member

/test

@giorio94
Copy link
Member

Rebased using the GH UI, hopefully that should fix the failing tests.

@aanm aanm added this pull request to the merge queue Jun 17, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 17, 2024
Merged via the queue into cilium:main with commit caadba1 Jun 17, 2024
64 checks passed
@jschwinger233 jschwinger233 mentioned this pull request Jun 18, 2024
Merged
4 tasks
@jschwinger233 jschwinger233 added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Jun 18, 2024
@PhilipSchmid PhilipSchmid mentioned this pull request Jun 24, 2024
Merged
1 task
@youngnick youngnick added backport/author The backport will be carried out by the author of the PR. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Jun 25, 2024
@aanm aanm added the backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. label Jul 2, 2024
@github-actions github-actions bot added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Jul 2, 2024
This was referenced Jul 10, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giorio94 giorio94 giorio94 approved these changes

@sayboras sayboras sayboras approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
area/clustermesh Relates to multi-cluster routing functionality in Cilium. area/helm Impacts helm charts and user deployment experience backport/author The backport will be carried out by the author of the PR. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.15.6
Status: Needs backport from main
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@PhilipSchmid @aanm @giorio94 @sayboras @jschwinger233 @youngnick