Repository navigation
feat(virtual-machine): make the guest shutdown grace period configurable - #3991
Closed
lf (lfinmauritius) wants to merge 1 commit into
Closed
lf (lfinmauritius) wants to merge 1 commit into
lf (lfinmauritius) wants to merge 1 commit into
Conversation
`terminationGracePeriodSeconds` was hardcoded to 30 in packages/apps/vm-instance/templates/vm.yaml, so no consumer of the aggregated API could change how long a guest is given to shut down cleanly. Why 30 seconds is not always enough. Stopping a VM is an ACPI powerdown: the guest closes its applications, flushes its journals, then hands back control. Linux does that in a few seconds. Windows applies pending updates during shutdown — the "Do not turn off your computer" screen — which routinely exceeds 30 s, and at 30 s KubeVirt destroys the domain. The result is an unclean shutdown of the guest filesystem on every stop of a machine that had updates queued. This is not limited to a user pressing "stop". The same field bounds every shutdown path: a node drain, an eviction, and a workload update rolled out by KubeVirt itself. A cluster upgrade therefore hard-kills those guests, and no portal or dashboard button is involved to work around it. The default stays 30, so behaviour is unchanged on upgrade. An operator who needs longer sets it per VM. Testing: rendered both paths with `helm template` (absent => 30, set => the value). No `helm unittest` suite is added for templates/vm.yaml: its instanceType and disks are gated by cluster `lookup` calls, so the template cannot render without a live cluster — the same reason the existing suite in this package scopes itself to templates/vm-update-hook.yaml. Assisted-By: Claude <[email protected]> Signed-off-by: Loïc Fontaine <[email protected]>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 28, 2026
Aleksei Sviridkin (lexfrei)
added a commit
that referenced
this pull request
Sep 11, 2026
## What this PR does Guards the Kube-OVN `IP` lookup behind API discovery. The `vm-instance` chart now renders on clusters using another CNI instead of trying to query a CRD that is not installed. The Helm regression suite mocks the IP object through `kubernetesProvider` and pins both sides: with `kubeovn.io/v1/IP` advertised the annotations carry the mocked MAC and IP, without it they are absent even though the object exists, which is the branch the guard adds. helm-unittest's fake lookup never errors on an unserved kind, so only the second case distinguishes the guard; it fails on main and passes here. Open PRs #3991, #3978, #3931, and #3900 touch the same template. Their current hunks do not alter this lookup; #3900 adds secondary-interface annotations immediately after it, so it is the only nearby merge hunk to watch. ### Screenshots Not applicable; there is no UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(virtual-machine): render VM instances on clusters where the Kube-OVN IP API is not installed. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * VM instances now render correctly in environments without the Kube-OVN IP capability. * OVN IP lookups and MAC/IP annotations are only generated when the required Kube-OVN API is available. * MAC/IP annotations are omitted when the capability is unavailable, even if an IP object exists. * **Tests** * Added coverage for both supported and unsupported Kube-OVN IP discovery scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Contributor
|
lf (@lfinmauritius) closing this draft, it has not moved since August. The hardcoded 30 seconds is real, but a new per-VM field needs a real user case behind it. If you see Windows guests killed during updates on a real cluster, please open an issue with that case. |
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
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 this PR does
terminationGracePeriodSecondsis hardcoded to30inpackages/apps/vm-instance/templates/vm.yaml, so no consumer of the aggregated API can change how long aguest is given to shut down cleanly. This exposes it as a chart value, default
30— behaviour unchangedon upgrade.
Why 30 seconds is not always enough
Stopping a VM is an ACPI powerdown: the guest closes its applications, flushes its journals, then hands
back control. Linux does that in a few seconds. Windows applies pending updates during shutdown — the "Do
not turn off your computer" screen — which routinely exceeds 30 s, and at 30 s KubeVirt destroys the
domain. Every stop of a machine with queued updates is then an unclean shutdown of the guest filesystem.
And it is not limited to a user pressing stop. The same field bounds every shutdown path: a node
drain, an eviction, and a workload update rolled out by KubeVirt itself (
evictionStrategy: LiveMigrate,workloadUpdateMethods: [LiveMigrate, Evict]on the KubeVirt CR). A cluster upgrade therefore hard-killsthose guests, and no dashboard button is involved to work around it. That is also why the value belongs in
the CR rather than in a
stopsubresource call: only the CR is honoured by the paths nobody triggers.What this changes
packages/apps/vm-instance/values.yaml— annotated@param {int} terminationGracePeriodSeconds, default30, with the Windows case spelled out in the comment.packages/apps/vm-instance/templates/vm.yaml— renders the value instead of the constant.make generateand committed:values.schema.json,README.md,api/apps/v1alpha1/vminstance/types.go, andpackages/system/vm-instance-rd/cozyrds/vm-instance.yaml.Testing
helm templateon both paths — value absent renders30,--set terminationGracePeriodSeconds=300renders
300.No
helm unittestsuite is added fortemplates/vm.yaml: itsinstanceTypeand itsdisksare gated bycluster
lookupcalls, so the template cannot render without a live cluster. That is the same reason theexisting suite in this package scopes itself to
templates/vm-update-hook.yaml, and says so in its header.Screenshots
N/A — no UI change.
Downstream repositories
Downstream: walked the trigger map against the diff, file by file.
cozystack/terraform-provider-cozystack— affected: this adds a field to an app'svalues.schema.json, and the provider modelsvm-instanceconfig by hand (schema, model, expand/flattenpair) in both the resource and the data source. Follow-up issue: cozystack/terraform-provider-cozystack#36
(searched first: [core] Update fluxcd and kubeapps #35 exists but is scoped to
disks[].hotpluggable, a different field).cozystack/website— covered by the release bot:vm-instanceis in theVMSlist of the websiteMakefile, and its reference page is regenerated from this package'sREADME.md, whichmake generateupdated here.
ansible-cozystack,ccp,talm,cozyhr,cozy-proxy,external-apps-example,examples,cozystack-telemetry-server— no trigger: nothing underhack/, no package layout, namespace, CRD,installer value, variant, node prerequisite or release-prep change.
Release note