Skip to content

feat(virtual-machine): make the guest shutdown grace period configurable - #3991

Closed
lf (lfinmauritius) wants to merge 1 commit into
cozystack:mainfrom
lfinmauritius:feat/vm-instance-termination-grace
Closed

lf (lfinmauritius) wants to merge 1 commit into
cozystack:mainfrom
lfinmauritius:feat/vm-instance-termination-grace

Conversation

@lfinmauritius

@lfinmauritius lf (lfinmauritius) commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

What this PR does

terminationGracePeriodSeconds is hardcoded to 30 in
packages/apps/vm-instance/templates/vm.yaml, so no consumer of the aggregated API can change how long a
guest is given to shut down cleanly. This exposes it as a chart value, default 30 — behaviour unchanged
on 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-kills
those 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 stop subresource call: only the CR is honoured by the paths nobody triggers.

What this changes

  • packages/apps/vm-instance/values.yaml — annotated @param {int} terminationGracePeriodSeconds, default
    30, with the Windows case spelled out in the comment.
  • packages/apps/vm-instance/templates/vm.yaml — renders the value instead of the constant.
  • Generated by make generate and committed: values.schema.json, README.md,
    api/apps/v1alpha1/vminstance/types.go, and packages/system/vm-instance-rd/cozyrds/vm-instance.yaml.

Testing

helm template on both paths — value absent renders 30, --set terminationGracePeriodSeconds=300
renders 300.

No helm unittest suite is added for templates/vm.yaml: its instanceType and its disks are gated by
cluster lookup calls, so the template cannot render without a live cluster. That is the same reason the
existing 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's
    values.schema.json, and the provider models vm-instance config by hand (schema, model, expand/flatten
    pair) 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-instance is in the VMS list of the website
    Makefile, and its reference page is regenerated from this package's README.md, which make generate
    updated here.
  • ansible-cozystack, ccp, talm, cozyhr, cozy-proxy, external-apps-example, examples,
    cozystack-telemetry-server — no trigger: nothing under hack/, no package layout, namespace, CRD,
    installer value, variant, node prerequisite or release-prep change.

Release note

feat(virtual-machine): `terminationGracePeriodSeconds` is now a `vm-instance` value (default 30). It bounds the ACPI powerdown of the guest on every shutdown path — an explicit stop, a node drain, an eviction, a workload update — after which KubeVirt destroys the domain. Raise it for Windows guests, which apply pending updates during shutdown and are otherwise hard-killed at 30 seconds.

`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]>
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added size/M This PR changes 30-99 lines, ignoring generated files area/virtualization Issues or PRs related to virtualization (kubevirt, cdi, vmi, vm-import) kind/feature Categorizes issue or PR as related to a new feature labels 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 -->
@lexfrei

Copy link
Copy Markdown
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/virtualization Issues or PRs related to virtualization (kubevirt, cdi, vmi, vm-import) kind/feature Categorizes issue or PR as related to a new feature size/M This PR changes 30-99 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants