Reconcile VMLocation when it's moved to different RP and set appropriate conditions#1558
Open
anwithaun-1 wants to merge 21 commits into
Open
Reconcile VMLocation when it's moved to different RP and set appropriate conditions#1558anwithaun-1 wants to merge 21 commits into
anwithaun-1 wants to merge 21 commits into
Conversation
github-actions
Bot
added
size/M
anwithaun-1
changed the title
anwithaun-1
changed the title
aakashchan
reviewed
Apr 22, 2026
aakashchan
reviewed
Apr 22, 2026
added 2 commits
April 22, 2026 18:50
bryanv
reviewed
Apr 24, 2026
hpannem
reviewed
Apr 27, 2026
anwithaun-1
commented
Apr 30, 2026
Contributor
Author
anwithaun-1
left a comment
anwithaun-1
left a comment
There was a problem hiding this comment.
Please take a look at the addressed comments.
added 2 commits
April 30, 2026 15:23
anwithaun-1
commented
May 11, 2026
Contributor
Author
anwithaun-1
left a comment
anwithaun-1
left a comment
There was a problem hiding this comment.
Resolved all the comments. pls review again.
bryanv
reviewed
May 11, 2026
github-actions
Bot
added
size/L
Contributor
Author
|
@aruneshpa @bryanv @hpannem , pls review |
hpannem
reviewed
May 16, 2026
hpannem
reviewed
May 16, 2026
hpannem
reviewed
May 16, 2026
| } | ||
|
|
||
| // 3. Validate Ancestry | ||
| // Check if the VM is in the Root RP or a Child RP (common for VKS) |
Contributor
There was a problem hiding this comment.
what happens if the vm is in correct namespace however wrong child RP? Is this not under the PR's scope?
Contributor
Author
There was a problem hiding this comment.
what is considered as wrong Child RP?
If the VM is in correct namespace and moved to any of the child RP of that namespace, then it's considered as a valid location as of now
Contributor
There was a problem hiding this comment.
ok. However we need to confirm this with Bryan or Andrew.
Collaborator
There was a problem hiding this comment.
Hmm. I am with Hemanth on this. The only valid use-case for child RP is VKS. But if the VM is not in the VKS child RP, then we need to report it as an error / invalid location.
Contributor
Author
There was a problem hiding this comment.
@aruneshpa How to ensure that if the child RP is VKS or not?
hpannem
reviewed
May 18, 2026
| return err | ||
| } | ||
|
|
||
| pkgcnd.Delete(vmCtx.VM, vmopv1.VirtualMachineInAuthorizedLocation) |
Contributor
There was a problem hiding this comment.
So, absence of this condition has two scenarios.
- the reconcile has not yet happened.
- The reconcile happened and its in correct location.
Would it be better to distinguish these two scenarios?
True: In correct location.
False: not in correct location.
Absent: Not yet run or unable to determine.
Collaborator
There was a problem hiding this comment.
Correct. The way the condition is named right now, it must be set to True during every reconcile if the VM is in a valid location.
If it is only supposed to be set when the VM is in invalid location, then we can call it "VMInInvalidLocation" or something but that violates the principal that the condition types must have positive polarity.
Contributor
Author
There was a problem hiding this comment.
@hpannem @aruneshpa
ReconcileLocation is called when VM is created or when resourcePool is changed. So in my opinion (Absent: Not yet run or unable to determine ) is not required. We can distinguish between the VM in correct location and in incorrect location. Also "unable to determine" can happen for many reasons. if we are not able to reconcile VM's location and just remove the condition in this case, then it leads to the impression that the VM is normal.
Are these states Ok ?
True: In correct location (when vm is created or or after creation when it's moved to valid location )
False: not in correct location (when vm is moved to invalid location)
Contributor
There was a problem hiding this comment.
Think of user perspective, who is monitoring the CR.
- VM reconcile is not yet run. There could be many reasons for this. Operator down... or CR is queued and reconciler didn't get a chance to this. Condition absence == reconciler not run. User waits until condition is inserted and checks the status.
- Once reconciled, condition is put. Usually, I believe conditions should not get deleted once they are inserted. Just change true or false or unknown. There may be exceptions in some cases like upgrade etc. However, here, it would be good to keep it.
One correction: "Unable to determine" will go as an unknown on the condition. Condition will be set as "unknown" with a reason and message. Standard k8s pattern. aka. metav1.ConditionUnknown.
True: In correct location.
False: not in correct location.
Unknown: unable to determine.
Absent: Not yet run
aruneshpa
reviewed
May 19, 2026
| ) | ||
| if err != nil { | ||
| logger.Error(err, "failed to get expected namespace resource pool") | ||
| return nil |
Collaborator
There was a problem hiding this comment.
Hmm. Should this error be ignored? If we are can't be certain that we are in the RP that we should be in, it is okay to error and requeue IMO.
Contributor
Author
There was a problem hiding this comment.
Reasons why I am not returning the Err here:
- In case if we fail to get namespace RP, then it'll block all the next VM reconcilations. That is the reason , I went ahead with ignoring this error. Still I have handled err in case if I am not able to validate the location or if the VM is in the RP that it shouldn't be in.
- Unit tests failure due to the empty zonename property which is expected by the GetNamespaceFolderAndRPMoID api.
@aruneshpa could you pls provide suggestions on this.
| return err | ||
| } | ||
|
|
||
| pkgcnd.Delete(vmCtx.VM, vmopv1.VirtualMachineInAuthorizedLocation) |
Collaborator
There was a problem hiding this comment.
Correct. The way the condition is named right now, it must be set to True during every reconcile if the VM is in a valid location.
If it is only supposed to be set when the VM is in invalid location, then we can call it "VMInInvalidLocation" or something but that violates the principal that the condition types must have positive polarity.
| "summary.runtime.connectionState", | ||
| "summary.runtime.host", | ||
| "summary.runtime.powerState", | ||
| "resourcePool", |
Collaborator
There was a problem hiding this comment.
Should we also watch the folder ("parent") here?
Contributor
Author
There was a problem hiding this comment.
@aruneshpa Initially I had added "parent" and removed it as @bryanv mentioned that "the VM managed object doesn't have a parent field" in of the review thread above. I am not sure about this. Could you please provide more clarity.
anwithaun-1
changed the title
added 2 commits
May 19, 2026 19:15
Minimum allowed line rate is |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do, and why is it needed?
Problem:
Currently, the VM Operator does not set appropriate conditions or halt reconciliation when a VM is moved out of its designated resource pool. This leads to two major issues:
Permission Creep: DevOps admin privileges are inadvertently extended to reconfigure VMs that have drifted into different inventory locations.
Lack of Visibility: There is no "stop-gap" or clear status indicator telling administrators that the VM is in an orphaned state, leading to unpredictable reconciliation behavior.
Solution:
State Conflict: Two instances of the same workload existing simultaneously in different locations.
Which issue(s) is/are addressed by this PR? (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
VMSVC-3539
VMSVC-3541
Are there any special notes for your reviewer:
Please add a release note if necessary: