-
Notifications
You must be signed in to change notification settings - Fork 48
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 vm.spec.guestID API and its implementation #563
Conversation
51a2fcf
to
bcf1100
Compare
9b35051
to
dfd30d5
Compare
api/v1alpha3/virtualmachine_types.go
Outdated
|
||
// VirtualMachineConditionPrePowerOnReconfigReady exposes the status of the | ||
// VM's pre-power-on reconfiguration task. | ||
VirtualMachineConditionPrePowerOnReconfigReady = "VirtualMachinePrePowerOnReconfigReady" |
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.
I'm a little uneasy that this condition exposes an internal implementation detail. In a lot of cases we don't need that reconfigure b/c of the 7605 work, and ideally I want to get us to where the VM is created with its desired configuration in all cases. And with resize coming that is going to take the place of the current prepower on reconfigure, and that will have its own conditon.
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.
The purpose of this new condition is to inform users when they attempt to deploy a VM with an invalid guestID, as it's no longer verified by a generated list from the imported govmomi package. While implementing this change, I thought why make it specific to guestID only, hence the PrePowerOnReconfigureReady
name above to expose any VM reconfigure failure (it still has config.guestID in the condition's reason if the task fails by that).
On the other hand, today VMOP already sends a K8s event if the PrePowerOnReconfigure
task fails. This event contains specific invalid configs as well, such as guestID shown in the following example. So I think users are not entirely in the dark if they have specified an unsupported guestID:
Warning CreateOrUpdateFailure 29s (x16 over 116s) vmware-system-vmop/vmware-system-vmop-controller-manager-745fcd885b-v2xhm/virtualmachine-controller reconfigure VM task failed: A specified parameter was not correct: configSpec.guestId
@akutz Any thoughts on dropping this condition and leveraging existing K8s events for this?
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.
I'd prefer to not start exposing prePowerOnReconfigure as a condition too.
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.
Updated with the Guest ID specific condition. Please refer to the PR's description for all possible success/failure cases.
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.
lgtm, just some minor doc suggestions.
dfd30d5
to
6cd9ec1
Compare
6cd9ec1
to
fe10fcc
Compare
fe10fcc
to
4edea50
Compare
4edea50
to
9920639
Compare
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.
lgtm
Minimum allowed line rate is |
What does this PR do, and why is it needed?
This PR includes the following changes:
vm.spec.guestID
API in v1alpha3 and related conversion in v1alpha1 and v1alpha2.configSpec.guestID
if it's set in the VM Spec.vm.spec.guestID
if a VM is powered on.Depending on if a VM has been created, the VM's condition will be updated when an unsupported guestID is specified:
VirtualMachineCreated
condition will be marked as false with a message indicating an invalid guestID.GuestIDReconfigured
condition will be marked as false. It will be marked as true when a valid guestID is passed afterwards.Testing Done:
Are there any special notes for your reviewer:
A separate change has been submitted internally to the vSphere Content Library service to avoid overwriting the VM's guestID from the OVF template.
Please add a release note if necessary:
📚 Documentation preview 📚: https://vm-operator--563.org.readthedocs.build/en/563/