-
Notifications
You must be signed in to change notification settings - Fork 296
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
✨ Add Namespace Scoped Zone Discovery and Watch #3146
Conversation
4247967
to
e19a5cd
Compare
@zhanggbj Should we already take a look or wait until the PR is out of draft? |
@sbueringer @chrischdi @fabriziopandini About testing results with a real supervisor:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to add a watch ca. at:
https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/main/controllers/vspherecluster_controller.go#L75
But only if the featuregate is enabled.
It should then use a EnqueueRequestFromMapFunc
for all clusters in the namespace where the zone change was detected.
Sure, we need to watch the Zone event, but I was planning to add it in a separate PR, as so far I cannot test it locally as the supervisor hasn't fully supported Zone deletion. Currently only Zone discovery and cluster creation can be tested. |
I really would prefer adding the watch in this PR already, even if we can't manually test it. Although it's not clear to me why we can't test it. Basically you can add/remove Zone objects and these should be then reflected in VSphereCluster and directly afterwards on Cluster, right? |
Have to add a dot after this line, otherwise godot linter will fail
|
@chrischdi @sbueringer |
Let's add a global ignore to this path in .golangci.yaml Should then be something like:
|
404ed05
to
73e4950
Compare
LGTM label has been added. Git tree hash: 73bebc55504d8679c600b53018c2cd01d0629ff6
|
/assign @chrischdi for a final review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/hold cancel
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrischdi, 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 |
/hold |
- Introduce a feature flag to enable Namespace Scoped Zone. - Enhance zone discovery to support Namespace Scoped Zones. - Filter out zones marked for deletion during the discovery process.
Signed-off-by: Gong Zhang <[email protected]>
/lgtm /test help |
@chrischdi: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
LGTM label has been added. Git tree hash: 09e4e29248e364c519a92078215bfb1640c9f861
|
/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main |
/hold Waiting for jobs here and at team-cluster-api#7 to succeed |
Tests are green. Nice work everyone! /hold cancel |
What this PR does / why we need it:
This is to support the new Namespace Scoped Zone for isolation.
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 #