✨ WIP: Add affinity/anti-affinity to VirtualMachine spec #3645
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
|
|
||
| // Check the presence of the node-pool label on the VirtualMachineGroup | ||
| nodePool := supervisorMachineCtx.Machine.Labels[clusterv1.MachineDeploymentNameLabel] | ||
| if zone, ok := vmOperatorVMGroup.Labels[fmt.Sprintf("capv/%s", nodePool)]; ok && zone != "" { |
There was a problem hiding this comment.
As discussed in capv design doc, adding prefix may will fail badly when using long node pool names. I've dropped the prefix in VMG controller.
| } | ||
| } | ||
|
|
||
| // Check the presence of the node-pool label on the VirtualMachineGroup |
There was a problem hiding this comment.
nit: let's use Machine Deployment in capv instead of node pool.
| // Check the presence of the node-pool label on the VirtualMachineGroup | ||
| nodePool := supervisorMachineCtx.Machine.Labels[clusterv1.MachineDeploymentNameLabel] | ||
| if zone, ok := vmOperatorVMGroup.Labels[fmt.Sprintf("capv/%s", nodePool)]; ok && zone != "" { | ||
| supervisorMachineCtx.VSphereMachine.Spec.FailureDomain = ptr.To(zone) |
There was a problem hiding this comment.
For day 2 operations, we only need to create the VM with a label "topology.kubernetes.io/zone: Zone-x", but no need to set VSphereMachine.Spec.FailureDomain or Machine.Spec.FailureDomain. We should keep them consistent since they're empty from cluster specification.
After create VM with lable, VM Service will place it into that zone.
srm09
force-pushed
the
nodepool/vm-aff-antiaff
branch
from
597af80 to
87609a7
Compare
k8s-ci-robot
added
size/XL
srm09
force-pushed
the
nodepool/vm-aff-antiaff
branch
from
41a03e3 to
1759351
Compare
- Bump VMOP including Node AF/AAF support - Add NodeAutoPlacement Feature Gate (cherry picked from commit 700c8ae)
(cherry picked from commit cfeb862)
Removes the extra cases for VMG creation, such that VMG is created for: 1. Multiple zones, multiple MDs with no failureDomain 2. Multiple zones, multiple MDs with failureDomain 3. Single zone, existing cluster with no failureDomain MDs Signed-off-by: Sagar Muchhal <[email protected]>
- Updates VMOP API dependency Misc VMG fixes - Use namingStrategy to calculate VM names - Use MachineDeployment names for VMG placement label - Includes all machinedeployments to generate node-pool -> zone mapping Fixes VMG webhook validation error - Adds cluster-name label to Af/AAF spec - re-adds zone topology key back to anti-aff spec Signed-off-by: Sagar Muchhal <[email protected]>
srm09
force-pushed
the
nodepool/vm-aff-antiaff
branch
from
1759351 to
fa7432f
Compare
| // } | ||
| // } | ||
| // log.V(5).Info("Identified worker Machines linked to MachineSets", "count", len(workerMachines)) | ||
|
|
There was a problem hiding this comment.
Hi @srm09 , to get the current vspheremachine, I think it works during initial creations.
However during an rolling upgrade
- The old MachineSet (based on the previous template) starts scaling down.
- The new MachineSet (based on the updated template) starts scaling up.
I'm afraid it's not accurate to get VSphereMachine objects by list and compare with the desired replica count, so we should filter out by the MachineSets and wait. WDYT?
| }, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
@srm09
I noticed this is removed from design doc "Is failureDomain specified? If yes, create VM with zone label."
Could you help to clarify? If failureDomain is specified, I think we could simply honor it and create VM directly with zone label. Why we are still adding AF/AAF configs here?
There was a problem hiding this comment.
To keep the configuration of all VMs similar and avoiding code branching when not necessarily needed.
Signed-off-by: Sagar Muchhal <[email protected]>
k8s-ci-robot
added
size/XXL
Signed-off-by: Sagar Muchhal <[email protected]>
* Refine VMG controller when generate per-MD zone labels - Skip legacy already-placed VM which do not have placement info - Skip VM which do not have zone info * Apply suggestions from code review --------- Co-authored-by: Sagar Muchhal <[email protected]>
3 participants
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 #