Detect VMI guest-agent availability before quiesce, with explicit user override - #218
Conversation
|
Hi @Copilot. Thanks for your PR. I'm waiting for a migtools member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>
Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>
| } | ||
|
|
||
| vmi := &kubevirtcorev1.VirtualMachineInstance{} | ||
| if err := r.Get(ctx, types.NamespacedName{Name: vmRef.Name, Namespace: vmRef.Namespace}, vmi); err != nil { |
There was a problem hiding this comment.
Could we use r.APIReader for this lookup when it is configured, with the cached client as a fallback? This is a one-shot safety decision, and a stale cached VMI can still report AgentConnected=True after the agent has disconnected. That would create SkipQuiesce=false and reintroduce the freeze warning this PR is meant to avoid. The controller already uses APIReader for fresh VMB status reads.
| - apiGroups: | ||
| - kubevirt.io | ||
| resources: | ||
| - virtualmachineinstances |
There was a problem hiding this comment.
Please propagate this new virtualmachineinstances permission to the OADP Operator manifests as part of this change. The controller is deployed through OADP Operator, which vendors this RBAC into config/kubevirt-datamover-controller_rbac/role.yaml and the bundle CSV. Without updating that copy, OADP deployments will not have permission to read VMIs and will always fall back to skip-quiesce.
| }, | ||
| } | ||
|
|
||
| tests := []struct { |
There was a problem hiding this comment.
Could we add cases for an invalid or "auto" annotation value and for explicit "false" when the VMI is missing? These are documented behaviors and are especially relevant to the plugin integration, where absent/auto means automatic detection and false must remain an explicit override.
| // controller quiesces only when the VM's VirtualMachineInstance | ||
| // reports status.conditions[type=AgentConnected].status == "True". | ||
| // This override always takes precedence over automatic detection. | ||
| AnnotationQuiesce = "kubevirt-datamover.io/quiesce" |
There was a problem hiding this comment.
Lets stick with SkipQuiesce, since that's the name of the VMBackup spec field. So across this PR, s/AnnotationQuiesce/AnnotationSkipQuiesce/, change the value to "kubevirt-datamover.io/skipQuiesce, with the appropriate s/false/true/ wherever needed.
| // backup failure from attempting to quiesce a VM whose agent state is | ||
| // unknown. | ||
| func (r *KubeVirtDataUploadReconciler) determineSkipQuiesce(ctx context.Context, logger logr.Logger, du *velerov2alpha1.DataUpload, vmRef *common.VMReference) bool { | ||
| if raw, ok := du.Annotations[common.AnnotationQuiesce]; ok { |
| if raw, ok := du.Annotations[common.AnnotationQuiesce]; ok { | ||
| if override, err := strconv.ParseBool(raw); err == nil { | ||
| logger.Info("Quiesce explicitly overridden via annotation", "quiesce", override) | ||
| return !override |
- Rename AnnotationQuiesce -> AnnotationSkipQuiesce, matching the VirtualMachineBackup.Spec.SkipQuiesce field name, with inverted semantics: "true" forces skip-quiesce, "false" forces quiesce. - Use r.APIReader (uncached) for the VMI lookup when configured, since this is a one-shot safety decision and a stale cached VMI can still report AgentConnected=True after the agent disconnected. - Fix double-negative typo (!override -> override) that fell out of the rename. - Add test coverage for invalid/"auto" annotation values and an explicit force-quiesce override when no VMI exists. Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Copilot, shubham-pampattiwar, sseago The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubevirt-datamover-controller PR migtools/kubevirt-datamover-controller#218 adds a virtualmachineinstances get/list/watch permission needed for its guest-agent-based quiesce detection. Propagate it here so OADP-deployed controllers actually have it, instead of always falling back to skip-quiesce. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
|
/cherry-pick oadp-1.6 |
|
@sseago: #218 failed to apply on top of branch "oadp-1.6": DetailsIn 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. |
kubevirt-datamover-controller PR migtools/kubevirt-datamover-controller#218 adds a virtualmachineinstances get/list/watch permission needed for its guest-agent-based quiesce detection. Propagate it here so OADP-deployed controllers actually have it, instead of always falling back to skip-quiesce. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
- Rename AnnotationQuiesce -> AnnotationSkipQuiesce, matching the VirtualMachineBackup.Spec.SkipQuiesce field name, with inverted semantics: "true" forces skip-quiesce, "false" forces quiesce. - Use r.APIReader (uncached) for the VMI lookup when configured, since this is a one-shot safety decision and a stale cached VMI can still report AgentConnected=True after the agent disconnected. - Fix double-negative typo (!override -> override) that fell out of the rename. - Add test coverage for invalid/"auto" annotation values and an explicit force-quiesce override when no VMI exists. Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
kubevirt-datamover-controller PR migtools/kubevirt-datamover-controller#218 adds a virtualmachineinstances get/list/watch permission needed for its guest-agent-based quiesce detection. Propagate it here so OADP-deployed controllers actually have it, instead of always falling back to skip-quiesce. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
KubeVirt datamover backups always attempted to quiesce the guest filesystem, producing noisy freeze-failure warnings on VMs without a QEMU guest agent (e.g. Cirros test VMs). The controller now makes the quiesce decision from the VM's guest-agent status instead of requiring workload-specific annotations.
Guest-agent detection
common.IsGuestAgentConnected(vmi): reports whetherVirtualMachineInstance.Status.Conditions[type=AgentConnected].Status == True.determineSkipQuiescein the DataUpload controller: fetches the VM's VMI and setsVirtualMachineBackup.Spec.SkipQuiesceaccordingly. If the VMI can't be fetched, defaults to skip-quiesce (crash-consistent) as the safe fallback.Explicit user override
kubevirt-datamover.io/quiesceannotation onDataUpload:"true"forces quiesce,"false"forces skip-quiesce, regardless of guest-agent state. Any other value (or unset) falls back to auto-detection. The override always takes precedence.Wiring & permissions
ensureVMBackupnow threads the resolvedskipQuiescevalue into the createdVirtualMachineBackupspec.get;list;watchonvirtualmachineinstances;config/rbac/role.yamlregenerated viamake manifests.Documentation