andrijapanicsb opened a new pull request, #14534:
URL: https://github.com/apache/cloudstack/pull/14534
### Description
Fixes #14462
**In short:** KVM leaves two kinds of small files behind that nothing ever
deletes. This PR deletes each one once it is no longer needed, and changes
nothing else.
### Who runs into this
**Migrating VMs from VMware to KVM.** If you convert a VM straight onto NFS
primary storage (the "Force converting to storage pool directly" option), a
small `.xml` file is left next to the VM's disks after every migration. It
stays there even after the VM is deleted.
**Deleting UEFI VMs.** UEFI is the modern boot firmware. Every UEFI VM has a
small file on its KVM host that stores its boot settings, called the NVRAM
file. When the VM is permanently deleted (expunged), that file stays on the
host.
Nothing uses these files afterwards, and nothing cleans them up. After many
migrations or many deleted VMs, an operator has to find and remove them by hand.
### What changes
| Situation | Before | After |
|---|---|---|
| VMware VM migrated straight onto NFS, import succeeds | `.xml` file left
next to the disks | `.xml` file removed, disks untouched |
| Same migration, but the import fails | Existing cleanup removes the disks
and the `.xml` | Unchanged |
| VMware VM migrated through temporary storage | Nothing left behind |
Unchanged |
| Warm (CBT) migration | Nothing left behind | Unchanged |
| UEFI VM deleted permanently, its host is up | NVRAM file left on the host
| NVRAM file deleted |
| UEFI VM deleted permanently, its host is down | Delete succeeds, file left
| Delete still succeeds, file left, warning logged |
| UEFI VM stopped, rebooted, migrated, or destroyed but recoverable | NVRAM
file kept | Unchanged |
| Non-UEFI VMs, system VMs | Nothing to clean | Unchanged |
### What it never does
- It never makes deleting a VM fail or hang. The NVRAM cleanup is best
effort with a short timeout. If the host is unreachable, the VM is still
deleted and only a warning is logged.
- It never deletes the disks of an imported VM. Only the one `.xml` file the
import just read is removed.
- It never deletes the NVRAM file of a VM that is still defined on the host.
- No API, database or UI changes.
### Not covered
- Only the host the VM last ran on is cleaned. If a UEFI VM ran on several
hosts during its life, older copies of its NVRAM file on those other hosts stay.
- Files left behind by VMs deleted before this change are not cleaned up.
- Tested with unit tests only so far, not yet on a live environment (see
below).
<details>
<summary>Technical details</summary>
**Conversion XML**
- With `forceconverttopool=true` on file-based storage,
`LibvirtImportConvertedInstanceCommandWrapper` adopts the converted disks in
place and skipped the cleanup step that the normal path runs.
- It now deletes exactly `<conversion-uuid>.xml` (the file it just parsed),
only after the converted instance was read successfully.
- On failure the XML is deliberately kept: the management server then sends
`CleanupConvertedInstanceDisksCommand`, which reads that XML to find the
converted disks to remove.
- Warm (CBT) cutover was checked and does not leak an XML: its input XML is
a `/tmp` file deleted in a `finally` block, in-place finalization writes no
output XML, and the fallback output directory is deleted once the disks are
moved out.
- Origin: `forceconverttopool`, #11594
(b106d6e190b098ee1e97264952ed59a277e5c360).
**UEFI NVRAM**
- The per-VM NVRAM file is host-local: `guest.nvram.path` + `<vm-uuid>.fd`.
- `KVMGuru.finalizeExpunge` now sends a new `DeleteUefiNvramCommand` to the
VM's current or last host, only for user VMs with the `UEFI` boot detail.
- It uses `easySend` with a 60 second timeout, catches all errors, and still
returns no expunge commands. Returning the command from `finalizeExpunge`
instead would have made expunge fail whenever the last host is down.
- On the host, `LibvirtDeleteUefiNvramCommandWrapper`:
- rejects anything that is not a plain UUID, so no other path can be
targeted;
- does nothing if no NVRAM path is configured;
- keeps the file if a libvirt domain with that UUID is still defined, or
if that cannot be checked;
- otherwise deletes the file, and treats an already-missing file as
success.
- Mixed versions during an upgrade: an older agent answers that the command
is unsupported; the management server logs a warning and the file stays, as
today.
- Origin: per-VM NVRAM files since #3638
(d4b537efa7ed27ef88b735a22493808b61529186), never removed on expunge.
**Files**
- `core/src/main/java/com/cloud/agent/api/DeleteUefiNvramCommand.java` (new)
-
`plugins/hypervisors/kvm/.../wrapper/LibvirtDeleteUefiNvramCommandWrapper.java`
(new)
-
`plugins/hypervisors/kvm/.../wrapper/LibvirtImportConvertedInstanceCommandWrapper.java`
- `server/src/main/java/com/cloud/hypervisor/KVMGuru.java`
- tests for all of the above
</details>
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [x] Minor
- [ ] Trivial
### How Has This Been Tested?
**In short:** all affected unit tests pass, including 19 new ones. No live
test yet.
| What was checked | Result |
|---|---|
| Import after migration straight onto NFS: only the `.xml` goes, the disk
and unrelated files stay | Pass |
| Failed import keeps the `.xml` for the existing cleanup | Pass |
| Host deletes only the expunged VM's own NVRAM file | Pass |
| Host keeps the file while the VM is still defined, or when it cannot check
| Pass |
| Host rejects a malformed VM UUID such as `../...` | Pass |
| Management server only asks for cleanup for UEFI user VMs with a known
host | Pass |
| Deleting a VM still succeeds when its host is down or an error occurs |
Pass |
| Checkstyle | No violations |
| Live run on a lab | Not done yet |
<details>
<summary>Test details</summary>
Run locally with JDK 17:
```
mvn -pl core,server,plugins/hypervisors/kvm -am test \
-Dtest='KVMGuruTest,LibvirtDeleteUefiNvramCommandWrapperTest,LibvirtImportConvertedInstanceCommandWrapperTest'
\
-Dsurefire.failIfNoSpecifiedTests=false -Dexec.skip=true
```
| Test class | Result | New tests |
|---|---|---|
| `LibvirtImportConvertedInstanceCommandWrapperTest` | 16/16 | 3 |
| `LibvirtDeleteUefiNvramCommandWrapperTest` (new) | 10/10 | 10 |
| `KVMGuruTest` | 41/41 | 6 |
`-Dexec.skip=true` skips an unrelated schema shell test that cannot run on
Windows (CRLF line endings).
Planned live checks:
- cold VMware → KVM import into NFS with `forceconverttopool=true`, then
confirm no `<uuid>.xml` is left in the pool;
- expunge of a UEFI VM with its last host up (file gone) and down (expunge
succeeds, warning logged).
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
