Repository navigation
Conversation
An in-place restore of a KVM instance with more than one data disk can write a backed up disk into a different volume than the one it came from. NASBackupProvider.restoreVMBackup pairs the backed up volumes with the instance's current volumes by position in two lists that are each sorted by device id, and LibvirtRestoreBackupCommandWrapper.restoreVolumesOfExistingVM consumes those lists index by index. The volume UUID is carried along but only used to build error messages. That pairing is only correct while the instance's device ids still match the ones recorded in the backup, and the restore itself breaks that: on completion KVMGuru.importVirtualMachineFromBackup re-attaches every data disk with getNextAvailableDeviceId(). The volumes are never detached first, so their current ids still count as in use and the helper cannot return the id a volume already holds - every data disk is shifted by one slot on each restore. The root disk is unaffected because it is always re-attached at device id 0. A second restore therefore maps the backup files onto the wrong volumes. With equally sized disks the contents are silently exchanged and the API reports success. With disks of different sizes the first volume is overwritten and the restore then fails converting an image into a smaller volume, leaving that volume half rewritten with no rollback. Three changes: - KVMGuru: re-attach data disks with the device id recorded in the backup, falling back to the next free id only when the backup has none. - LibvirtRestoreBackupCommandWrapper: select the target volume by UUID instead of by list position, and fail explicitly when a backed up volume is no longer attached to the instance. - NASBackupProvider: derive the UUID list and the backup file list from the same sorted collection so the two cannot drift apart. The create-instance-from-backup path (restoreVolumesOfDestroyedVMs) is left alone: it provisions new volumes with new UUIDs, so it has nothing to match on.
LibvirtRestoreBackupCommandWrapperTest: restoring an instance whose volumes arrive in a different order than the backed up volumes must still write each backup into the volume it was taken from, and must fail explicitly when a backed up volume is no longer attached to the instance. KVMGuruTest: importing an instance from a backup must re-attach its data disks on the device ids recorded in the backup rather than allocating new ones.
calvix
force-pushed
the
fix/nas-backup-restore-volume-mapping
branch
from
August 19, 2026 12:00
8f011e1 to
4722bd4
Compare
… uuid Matching backed up volumes to the instance's volumes by uuid breaks creating an instance from a backup: that instance gets brand new volumes, so none of the uuids recorded in the backup can ever match and the restore always failed with the volume 'is not attached to the instance any more' error. When none of the recorded uuids match, the volumes are new and the device id ordering both lists already carry is the only mapping available, so fall back to it. When some of them do match, a missing one really is a detached volume and is still rejected, which is what the uuid matching was added for. The fallback also refuses to run if the two lists differ in length.
…e#13779) * Add NULL check during restoreVM operation when host is removed (apache#571) * Add NULL check during restore VM operation when host is not available/removed * fix line ending pre commit failure * update logging with details of removed host and vm --------- Co-authored-by: Sachin R Doddaguni <s_rudrappadoddagu@apple.com> (cherry picked from commit 1dbda12ccdca1aaf86ef8f9b185986c9d22b567e) * Handle null host in VM restore to prevent NPE on deleted host records * Fix build * Fix unit test --------- Co-authored-by: Sachin R <32716246+sachindoddaguni@users.noreply.github.com> Co-authored-by: Sachin R Doddaguni <s_rudrappadoddagu@apple.com> Co-authored-by: mprokopchuk <mprokopchuk@apple.com>
…ues (apache#12419) Co-authored-by: dahn <daan@onecht.net>
…s list (apache#13931) Co-authored-by: mprokopchuk <mprokopchuk@apple.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…#13502) Co-authored-by: dahn <daan@onecht.net>
…VM (apache#13558) Co-authored-by: Sachin R <32716246+sachindoddaguni@users.noreply.github.com> Co-authored-by: dahn <daan@onecht.net>
Co-authored-by: Daan Hoogland <dahn@apache.org> Co-authored-by: Suresh Kumar Anaparti <sureshkumar.anaparti@gmail.com>
…3144) Co-authored-by: toni.zamparetti <toni.zamparetti@scclouds.com.br>
…etworks with dedicated vlan (apache#13379) Co-authored-by: Pearl Dsilva <pearl1954@gmail.com>
…che#14022) setVpcLimit ignored its parameter and copied the networkLimit field into vpcLimit (this.vpcLimit = networkLimit), so a projects reported vpclimit always mirrored its networklimit instead of the real VPC limit (and would be null if setVpcLimit ran before setNetworkLimit). The sibling setters all assign their own parameter. Assign the vpcLimit parameter. Adds a regression test asserting setVpcLimit stores its own value and leaves networkLimit untouched.
Change input type for code entry field Replaced a-input-password with a-input for better user experience.
…pache#13611) When createVolumeAsync fails, createVolumeCallback resets the volume's pool_id only when volume.getPodId() != null. Zone-wide primary storage pools have no pod, so such a volume keeps a stale pool_id while it is reverted to Allocated. On the next create/attach, findStoragePool then returns no suitable pool because storagePoolCompatibleWithVolumePool rejects the volume (its state is not Ready), and the operation fails with "Unable to find suitable primary storage" even though the pool has plenty of capacity. Guard the reset on the field that is actually being cleared (getPoolId() != null) instead of getPodId(), so it also applies to zone-wide (and local) storage. The same guard is fixed in destroyAndReallocateManagedVolume. ensureVolumeIsExpungeReady is left unchanged as it legitimately clears pod_id. Regression from apache#10757.
…emoval race (apache#13700) removeNicFromVmThroughJobQueue looked up pending work jobs by (vmType, vmId, commandName) only, so the nic was not part of the dedup key. A second removeNicFromVirtualMachine request for a different nic on the same vm matched the first still-pending VmWorkRemoveNicFromVm job and joined it instead of submitting its own. That job removes only the nic it was created for, yet both callers wait on the same job id and both receive its success, leaving the second nic silently attached while its API call reports success. Make the nic uuid part of the lookup key, mirroring addVmToNetworkThroughJobQueue which was fixed the same way in apache#5658: - look up pending jobs with the 4-arg listPendingWorkJobs(Instance, vmId, cmd, nic.getUuid()) - fail fast with CloudRuntimeException if more than one job matches - stamp new jobs with setSecondaryObjectIdentifier(nic.getUuid()) before submitting Genuine duplicates, two requests for the same nic, still dedup as before. Adds three regression tests to VirtualMachineManagerImplTest covering the cross-nic race, same-nic dedup, and the multiple-pending-jobs guard. Fixes: apache#13699 Generated-by: Claude Code (Anthropic) Signed-off-by: Ayush Sinha <ayushsinha3199@gmail.com>
…pache#13835) * kvm: fix RBD exclusive-lock leak that breaks revertSnapshot on Ceph takeRbdVolumeSnapshotOfStoppedVm() called image.snapCreate(snapshotName) twice. The first call creates the RBD snapshot, the second one always throws RbdException ("Failed to create snapshot <uuid>") because the snapshot already exists. The duplicate is a merge artifact: 30d3066 ("Merge branch '4.20' into 4.22") resolved a conflict by keeping the call from both sides - each parent had exactly one. Because there was no finally block, that exception skipped rbd.close(image) and r.ioCtxDestroy(io), so the agent kept the image open and held its RBD exclusive-lock indefinitely. The exception is only logged, so the snapshot job still reported success and the fault stayed invisible. Consequences observed on a KVM + Ceph/RBD cluster: - revertSnapshot fails with "com.ceph.rbd.RbdException: Failed to rollback snapshot <uuid>". librbd returns EROFS because a live peer holds the exclusive-lock; 'rbd snap rollback' only succeeds once that client dies and librbd can break the lock, which makes the failure look intermittent. - getRbdSnapshotSize() is never reached, so every snapshot is reported with physical size 0 when snapshot.backup.to.secondary is false. - The leaked watchers keep the image busy, so 'rbd rm' fails and the volume cannot be expunged - it stays stuck in state Destroy. Note the method also runs for RUNNING VMs: createSnapshot() branches on "RUNNING && !primaryPool.isExternalSnapshot()", and RBD is an external-snapshot pool, so every RBD volume snapshot took this path. Remove the duplicated call and move the image/IO-context cleanup into a finally block so the lock is released even if the snapshot itself fails. * kvm: release RBD handles on every path when cloning a volume from a snapshot createRBDvolumeFromRBDSnapshot() closed the source image, the cloned image and the RADOS IO context only on the success path, and called snapUnprotect() only there too. Two paths escaped that cleanup: - the early "Could not find snapshot ... on RBD" return, and - any RadosException/RbdException from clone(), resize() or flatten(), which is caught and turned into a null disk. Both leave the images open, so this client keeps the RBD exclusive-lock. That later makes 'rbd snap rollback' (revertSnapshot) fail with EROFS from another host, and keeps the image busy so 'rbd rm' cannot remove it - the volume then stays stuck in state Destroy. The failure paths after snapProtect() are worse: the snapshot stays protected, and a protected snapshot can be deleted neither on its own nor together with its volume. Move the cleanup into a finally block, tracking whether the snapshot was actually protected so it is unprotected exactly when it needs to be. Failures during cleanup are logged and never mask the original outcome; a failed snapUnprotect is logged at ERROR since it needs manual intervention. This is the same class of defect as the leak fixed in takeRbdVolumeSnapshotOfStoppedVm(); no behaviour changes on the success path. * kvm: add regression tests for the RBD snapshot handle leak Two tests around takeRbdVolumeSnapshotOfStoppedVm, using the MockedConstruction pattern already used in this test class (the Rbd instance is created inside the method under test, so it cannot be injected): - createsSnapshotExactlyOnce guards the duplicated snapCreate call from coming back, and checks the image and IO context are released. - releasesHandlesWhenSnapshotFails makes snapCreate throw and asserts the image is still closed and the IO context destroyed, so a future failure cannot leak the RBD exclusive-lock again. takeRbdVolumeSnapshotOfStoppedVm, radosConnect and getRbdSnapshotSize widened from private to protected so the test can stub the Ceph interactions. * kvm: extract RBD handle teardown into null-safe helper methods Address review feedback: the nested try-catch teardown blocks in the finally clauses of takeRbdVolumeSnapshotOfStoppedVm and createRBDvolumeFromRBDSnapshot are extracted into two reusable, null-safe helpers that log but never throw: - closeRbdImage (3 call sites) - destroyRadosIoCtx (2 call sites) No behavior change. * kvm: extract RBD snapshot unprotect into a helper method Follow-up to the teardown helpers: the snapUnprotect block in the finally clause of createRBDvolumeFromRBDSnapshot moves into a never-throwing unprotectRbdSnapshot helper, so the finally clause is now free of inline try-catch constructions entirely. No behavior change. --------- Co-authored-by: calvix <7136358+calvix@users.noreply.github.com>
… on RBD (apache#13361) RBD erasure-coded pool support (apache#9808) added handling of the rbd_default_data_pool storage-pool detail to RBDStringBuilder (qemu-img path) and to createPhysicalDisk (blank volumes), but not to createDiskFromTemplateOnRBD. As a result, ROOT volumes created from a template via rados-java rbd.clone()/rbd.create() are created without a data pool: all of their data objects land in the (replicated) metadata pool instead of the erasure-coded data pool, defeating the point of EC and consuming ~3x raw space. Set rbd_default_data_pool on the Rados connection (before connect) in both the same-cluster clone/copy branch and the cross-cluster copy branch of createDiskFromTemplateOnRBD, using the destination pool's detail. librbd then honors it as the default data pool when the new image is created, so template-derived volumes get data_pool set, the same way blank volumes already do.
…storeBackupCommandWrapper (apache#14006) * kvm: detect mount failures and honour the configured timeouts on backup restore Script.executeCommand returns null when the command fails, it does not throw, so the try/catch around the mount and umount of the backup repository could never fire and the return value was discarded. A repository that fails to mount was therefore treated as mounted, and the restore carried on against an empty directory until it failed later with a misleading "backup file not found". A failed umount was ignored the same way, leaking the mount. Both now go through executeCommandForExitValue and check the exit value. The same refactor also dropped the timeouts. mountTimeout was still passed into mountBackupDirectory but never used, and the rsync of the volume lost the command timeout, so both fell back to the one hour default in Script instead of the configured values. An unresponsive repository could hold a restore up for an hour rather than failing after nas.backup.restore.mount.timeout seconds. * kvm: clean up after a failed mount and bound the unmount of a backup repository The directory created for the mount is removed by the caller in a finally block, but that block is only reached once the mount has succeeded, so a repository that cannot be mounted left an empty directory behind on every attempt. It is now removed before the failure is reported, best effort and logged if it cannot be. The unmount ran without a timeout and so fell back to the one hour default in Script. Unmounting a repository that has become unreachable blocks as easily as mounting one, and this runs in the cleanup path of a restore, so it is now bounded by the configured mount timeout like the mount itself.
Conflicts: - LibvirtRestoreBackupCommandWrapperTest.java: apache#14006 appended three mount/unmount timeout tests at the end of the class, where the volume mapping tests end too; kept both.
…eouts apache#14006 runs the mount, the umount and the rsync of a restore through Script.executeCommandForExitValue(long, String...) so that they honour the configured timeouts. The volume mapping tests still stubbed and verified the overloads without a timeout. The two tests asserting where a backup is written therefore failed, and the checks that a backup is never written into the wrong volume passed without verifying anything, as nothing called that overload any more. Stub and verify the timeout variants instead, as apache#14006 does for the existing tests.
…he#13877) * feat(backup): incremental NAS backup support for KVM (apache#13074) Implements incremental backup support for the NAS backup provider on KVM, using QEMU dirty bitmaps and libvirt's backup-begin API. RFC: apache#12899. For large VMs this reduces daily backup storage 80–95% and shortens backup windows from hours to minutes (e.g. a 500 GB VM with moderate writes goes from ~500 GB/day to ~5–15 GB/day after the initial full backup). Signed-off-by: James Peru <jmsperu@gmail.com> Co-authored-by: jmsperu <jmsperu@users.noreply.github.com> Co-authored-by: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> * fix checkstyle * Incremental nas backup fixes (apache#13571) * Avoid concurrent deletes modifying the same backup chain in parallel - Backup deletes are serialized by using a per-VM GlobalLock Backup delete checks if it has a live dependent backup not just a live immediate child. * Fix test failure in NasBackupProviderTest * backup: drop unused backupStatus search condition from listBackups The backupStatus condition was carried over from main, where apache#13254 added a status filter to listBackups. 4.22 has neither ListBackupsCmd.getBackupStatus() nor a setParameters call for it, so the condition is never populated and SearchCriteria.getWhereClause() skips it. Dead code on this branch; the statusNeq clause that hides tombstoned chain members is the one in use. * kvm: execute "qemu-img convert" by Script.executeCommandForExitValue * Add missing return in replaceVolumeWithBackup * make NASBackupFullEvery use NasBackupIncrementalEnabled as the parent * Address review comments: null/empty checks and restore answer guard --------- Signed-off-by: James Peru <jmsperu@gmail.com> Co-authored-by: James Peru Mmbono <jmsperu@gmail.com> Co-authored-by: jmsperu <jmsperu@users.noreply.github.com>
findByID will fail for dummy template as it is stored in the deleted state.
…ion (apache#14162) * kvm: allow importVm importsource=shared from an RBD pool The CheckVolumeCommand wrapper on the KVM agent only accepted file based pools, so importing a root disk straight from Ceph failed on the agent with "Unsupported Storage Pool" and surfaced as "Disk not found or is invalid" on the management server. Add RBD to the supported pool types, take the virtual size from the disk libvirt resolved (qemu-img cannot open a bare RBD image name), skip the QCOW2 header check for raw RBD images, and build the rbd: URI when running qemu-img info, the same way LibvirtGetVolumesOnStorageCommandWrapper already does for listVolumesForImport. * kvm: record the real image format on an imported volume importVolume and updateImportedVolume both stamped the cluster default format for the hypervisor, so a volume imported from an RBD pool was recorded as QCOW2 while a natively deployed volume on the same pool is RAW. This affected both entry points: importVm importsource=shared for a root disk, and importVolume for a data disk. Pass the format the hypervisor reported for the existing image, from the check answer for a root disk and from the volume listed on the pool for a data disk, and fall back to the hypervisor default only when no format is reported. This also corrects a raw image imported from a file based pool. * vm import: plan the instance inside the pod and cluster of the requested pool importKVMInstanceFromDiskImage planned with the pod and cluster unset, so the planner was free to pick any host in the zone by capacity. When it picked a host in a cluster that cannot see the pool the caller named, the volume check ran against whichever pool that cluster does have, and the import failed with "Disk not found or is invalid" although the image was fine. Take the pod and cluster from the pool the caller passed, the same way importVolume already derives its host from the pool's scope * verify qemu is able to read the rbd image since check can not be done on a raw file * use computed volumeDetails in success path as well. * ui: do not call the imported disk a QCOW2 image The import wizard for local and shared storage described the disk as a QCOW2 image. An image on an RBD pool is raw, and a raw image on a file based pool can be imported as well, so the wording is wrong for both. Call it a disk image instead. Only the English strings are changed; the other locales come from Transifex.
The incremental NAS backup backport (apache#13877) makes a restore first ask qemu-img whether the backup references a parent, and flattens the chain with qemu-img convert when it does; only a backup without a backing file is rsynced. The volume mapping tests stubbed runSimpleBashScriptForExitValue to return 0 for every command, so the probe reported a backing chain, the restore took the convert path, and the rsync invocations these tests assert on never happened. Answer the probe with "no backing chain", as the existing restore tests do, and keep 0 for the other checks.
* There is a set of TO classes with renamed fields, which makes impossible to correctly communicate between Management Servers and Agents * fix logger * Apply suggestion from Daman * Apply suggestion from Daman 2 * Fix nested TO renaming, log redaction and cleanup in compat TypeAdaptors AbstractTOAdaptor built its own private Gson to run the pre-rename serialization step through, which meant it never honoured the LoggingExclusionStrategy the enclosing Gson (GsonHelper's logging instance) was configured with, so fields marked @loglevel(Off) (e.g. VirtualMachineTO.vncPassword) leaked in plaintext when logged. It also had no adapters for the sibling compat TOs, so nested TOs (disks/nics inside a VirtualMachineTO, or a VirtualMachineTO inside a MigrateCommand) kept their new field names instead of being renamed for backward compatibility with older Agents. AbstractTOAdaptor no longer owns a Gson at all: it takes one via initGson(), mirroring the existing InterfaceTypeAdaptor pattern. GsonHelper.setDefaultGsonConfig now wires each compat adaptor's delegate Gson incrementally off the same builder, snapshotting it via builder.create() right before each adaptor registers itself, so every adaptor's delegate carries its sibling adaptors (for correct nested renaming) without ever routing back into itself and recursing forever. NetworkTO is now registered via registerTypeHierarchyAdapter since VirtualMachineTO.nics is declared as NicTO[] (a NetworkTO subclass) and was never matched by the previous exact-type registration. This also removes AbstractTOAdaptor's now-unused loggerBuilder/LOGGER and its duplicate copy of GsonHelper.setDefaultGsonConfig, and replaces a dead null check (getAsJsonObject() never returns null) with a real isJsonObject() check. Added RequestTest#testCompatFieldRenamingNestedTOs covering a StartCommand and a MigrateCommand with nested disks/nics, asserting old field names appear at every nesting level on the wire and that vncPassword never appears in the logging serialization. * fix logger * fix build error RequestTest * fix unit test failures RequestTest.java --------- Co-authored-by: mprokopchuk <mprokopchuk@gmail.com> Co-authored-by: Daman Arora <damans227@gmail.com>
…apache#14066) Device-mapper doubles every dash in both the volume group and the logical volume name. getSnapshotPath only escaped the resource and snapshot name, so on a VG like "linstor_pool-lvm-thin" the computed /dev/mapper path did not exist and backing up a snapshot to secondary storage failed with "qemu-img: Could not open". Fixes apache#14011
Co-authored-by: mprokopchuk <mprokopchuk@apple.com>
…kubectl -o name/jsonpath (apache#11968)
…eUtil (apache#13809) * Improve date/timestamp handling performance in GenericDaoBase and DateUtil * address review comment
…tation() (apache#13427) * Fix overly broad classpath scanning in ReflectUtil.getClassesWithAnnotation() Production logs show a recurring WARN at startup: "could not get type for name org.conscrypt.AllocatedBuffer" (org.reflections.ReflectionsException / ClassNotFoundException) Root cause: ReflectUtil.getClassesWithAnnotation() uses ClasspathHelper.forPackage() to collect URLs to scan, but this returns all classpath entries (JARs/directories), not just those containing the target package. The Reflections library then scans every .class file in every JAR — including Netty's ConscryptAlpnSslEngine$BufferAdapter which references the optional org.conscrypt.AllocatedBuffer type. During expandSuperTypes(), Reflections tries Class.forName() on that type and fails because Conscrypt is not (and need not be) on the classpath. The fix adds a FilterBuilder that restricts bytecode scanning to only classes whose fully-qualified names match the requested packages. This prevents Reflections from processing unrelated classes (e.g., io.netty.handler.ssl.*), eliminating the warning and reducing startup time by skipping thousands of irrelevant class files. Signed-off-by: Ganesh Maharaj Mahalingam <g.mahalingam@apple.com> * Add missing API command packages to ApiXmlDocWriter's classpath scan list --------- Signed-off-by: Ganesh Maharaj Mahalingam <g.mahalingam@apple.com> Co-authored-by: Ganesh Maharaj Mahalingam <g.mahalingam@apple.com>
…ils of the vms with failed migrations on host maintenance (apache#13492) - Fixes the Constraint error Column 'value' cannot be null
…torage pools (apache#13990) - connect to only storage pools with empty tags when access group is empty Co-authored-by: Sachin R Doddaguni <s_rudrappadoddagu@apple.com>
* Make worker and API thread pool size configurable The AsyncJobManager's API and Worker executor thread pool sizes are currently derived solely from db.cloud.maxActive (maxActive/2 and maxActive*2/3 respectively). This coupling doesn't account for environments with predominantly I/O-bound workers, where threads are held waiting on external responses for extended periods and higher pool sizes may be needed independently of the DB connection count. This adds two global config keys: - api.job.pool.size (default: 50) - work.job.pool.size (default: 50) The actual pool size is Math.max(configured value, db-derived default), preserving existing behavior by default while allowing operators to increase pool sizes when workloads require it. Pool sizes and their derivation are logged at startup for observability. * Replace synthetic pool size tests with real configure() executor assertions The previous unit tests for the api.job.pool.size/work.job.pool.size config keys only re-ran Math.max() on local hardcoded values, or checked ConfigKey plumbing directly, without ever calling AsyncJobManagerImpl.configure() itself. That meant a regression in the actual pool-sizing logic wouldn't have been caught. These tests now invoke configure() directly and assert on the real core pool size of the created _apiJobExecutor/_workerJobExecutor via reflection, with @Before/@after hooks to stub db.properties and reset config/executor state between tests. --------- Co-authored-by: Sachin R <32716246+sachindoddaguni@users.noreply.github.com> Co-authored-by: mprokopchuk <mprokopchuk@apple.com>
The Resource Limits tab derives each label from the resource type name
returned by listResourceLimits:
$t('label.max' + item.resourcetypename.replace('_', ''))
Resource.ResourceType declares backup("backup", 12), so the key looked up
for that row is label.maxbackup, which exists in no locale. en.json has
label.maxbackups, a different string used by the backup schedule form for
how many backups to retain, and label.maxbackupstorage, which is resource
type 13. With fallbackLocale 'en' and silentTranslationWarn enabled, the
miss is silent and vue-i18n renders the key itself, so the row reads
"label.maxbackup".
The same key is used for the tagged-limit validation message, which shows
the raw key for the same reason.
Backup was the only one of the 17 ResourceType values without a label; the
other 16 already resolve.
apache#14188) getPhysicalDisk() asks libvirt for the volume. A volume that was just created by the management server is not in this host's libvirt pool cache, so the lookup misses and getVolume() falls back to refreshing the whole pool. Refreshing an RBD pool opens and stats every image in it, so the cost grows with the number of volumes in the pool and is paid on every VM start. On a pool holding 950 images that is 11.7 seconds added to each start, against 0.005 seconds to list the image names. Look RBD volumes up directly through librbd instead. Creating, cloning, resizing, copying and deleting RBD volumes in this class already use librbd directly; only the lookup went through libvirt. Because this runs on every VM start, the connection is handled more carefully than at the existing call sites: - the rados connection is shut down in a finally, so a client is not left for the finalizer. KVMStoragePoolManager.getPhysicalDisk retries a missing volume 100 times, so leaking here would mean 101 live librados clients for one absent volume. - the image is opened read only, since it is only stat'ed, and so cannot take the exclusive lock. - the image is closed through a helper that does not throw, so a failed close cannot discard a successful lookup or hide the original error. - the cephx key is only set when the pool has a user. librados aborts the process if it is handed a null value.
Backport of apache#13909 to 4.22, targeting 4.22.2. This also creates the 4.22.1.0 to 4.22.2.0 upgrade path, which didn't exist yet: schema-42210to42220.sql, its cleanup companion, the Upgrade42210to42220 class, and the registration in DatabaseUpgradeChecker. The CALLs can't go into schema-42200to42210.sql instead, because every 4.22.1.0 install has already run that file and those are the affected clusters. Co-authored-by: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com>
* NAS backup: keep the schedule id on a failed backup left in Error state * NAS backup: add nas.backup.quiesce.agent.timeout for guest agent freeze and thaw * nasbackup.sh: abort the backup job before cleanup removes its destination * nasbackup.sh: always thaw after a freeze attempt and log freeze failures * nasbackup.sh: confirm a failed thaw against guest-fsfreeze-status before continuing * nasbackup.sh: only end the abort wait on a successful domjobinfo query * nasbackup.sh: retry failed job queries in the backup polling loop * NAS backup: count a backup kept in Error state in the account resource count * NAS backup: alert when cleanup fails after a failed backup
* Netris CloudStack Integration Enhancements * Netris - Support NAT, PF for Secondary IP * Allow PF rules to be correctly created on secondary IP of a VM on netris * unique static nat names on netris by adding public IP - handle backward compatibility --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - Support for L2 networks * Add support for L2 networks in Netris * remove the need to add connectivity to identify netris provider, remove changes to ui and create net offering * create vpc network and use it for l2 networks * dont fail if deletion on netris doesnt succeed * remove connectivity capability * update vpc name when l2 name is updated --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - CKS support Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - Enable Global Routing Flag for NATTED Dual-Stack VPCs * Set globalRouting = true, when network offering is NATTED, dual-stack * update global routing based on routing mode and ip protocol type --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> * Netris - Fix allocate vnets with the correct account ID so that release succeeds on network deletion * Fix allocate vnets with the correct account ID so that release succeeds on network deletion * add fix for vpc tiers as well * move entire vnet cleanup logic to trash from shutdown, so that it works for all network types for netris * cleanup vnets when reservation id is null --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - Add support to include VPN service for NATTED Netris offerings * Add support to include VPN service for NATTED Netris offerings * missing changes of global routing * Remove extra space --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> * Netris - Support Redundant Virtual Routers for Netris VPC networks * Add support for redundant routers for Netris VPC networks * address comment * default null network mode to natted for netris * preventing vpc and ipam creation if already exists --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - Enable autoscaling groups for Netris network offerings * Enable autoscaling groups for Netris network offerings * Fix UI loading for services when network mode changes for external providers and show vm autoscaling always set to true / non-editable for netris when LB is selected i.e, natted mode * Revert removed comment --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> * merge conflict * systemvm: fix VPC VPN issue when network id is bigger than 1000 on Netris * show networks for vnf nic mappings --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> Co-authored-by: Wei Zhou <weizhou@apache.org> * Add a check to prevent re-adding existing ACLs * Netris - Support NAT, PF for Secondary IP * Allow PF rules to be correctly created on secondary IP of a VM on netris * unique static nat names on netris by adding public IP - handle backward compatibility --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - Support for L2 networks * Add support for L2 networks in Netris * remove the need to add connectivity to identify netris provider, remove changes to ui and create net offering * create vpc network and use it for l2 networks * dont fail if deletion on netris doesnt succeed * remove connectivity capability * update vpc name when l2 name is updated --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - CKS support Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - Enable Global Routing Flag for NATTED Dual-Stack VPCs * Set globalRouting = true, when network offering is NATTED, dual-stack * update global routing based on routing mode and ip protocol type --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> * Netris - Fix allocate vnets with the correct account ID so that release succeeds on network deletion * Fix allocate vnets with the correct account ID so that release succeeds on network deletion * add fix for vpc tiers as well * move entire vnet cleanup logic to trash from shutdown, so that it works for all network types for netris * cleanup vnets when reservation id is null --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - Add support to include VPN service for NATTED Netris offerings * Add support to include VPN service for NATTED Netris offerings * missing changes of global routing * Remove extra space --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> * Netris - Support Redundant Virtual Routers for Netris VPC networks * Add support for redundant routers for Netris VPC networks * address comment * default null network mode to natted for netris * preventing vpc and ipam creation if already exists --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - Enable autoscaling groups for Netris network offerings * Enable autoscaling groups for Netris network offerings * Fix UI loading for services when network mode changes for external providers and show vm autoscaling always set to true / non-editable for netris when LB is selected i.e, natted mode * Revert removed comment --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> * merge conflict * systemvm: fix VPC VPN issue when network id is bigger than 1000 on Netris * show networks for vnf nic mappings * Add a check to prevent re-adding existing ACLs * Add a check to prevent re-adding existing ACLs * change message compare statement --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> Co-authored-by: Wei Zhou <weizhou@apache.org> * Add check for vnet name length and cleanup vpc and vnet resources on failure of creation of allocation * Netris - Support NAT, PF for Secondary IP * Allow PF rules to be correctly created on secondary IP of a VM on netris * unique static nat names on netris by adding public IP - handle backward compatibility --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - Support for L2 networks * Add support for L2 networks in Netris * remove the need to add connectivity to identify netris provider, remove changes to ui and create net offering * create vpc network and use it for l2 networks * dont fail if deletion on netris doesnt succeed * remove connectivity capability * update vpc name when l2 name is updated --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - CKS support Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - Enable Global Routing Flag for NATTED Dual-Stack VPCs * Set globalRouting = true, when network offering is NATTED, dual-stack * update global routing based on routing mode and ip protocol type --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> * Netris - Fix allocate vnets with the correct account ID so that release succeeds on network deletion * Fix allocate vnets with the correct account ID so that release succeeds on network deletion * add fix for vpc tiers as well * move entire vnet cleanup logic to trash from shutdown, so that it works for all network types for netris * cleanup vnets when reservation id is null --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - Add support to include VPN service for NATTED Netris offerings * Add support to include VPN service for NATTED Netris offerings * missing changes of global routing * Remove extra space --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> * Netris - Support Redundant Virtual Routers for Netris VPC networks * Add support for redundant routers for Netris VPC networks * address comment * default null network mode to natted for netris * preventing vpc and ipam creation if already exists --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris - Enable autoscaling groups for Netris network offerings * Enable autoscaling groups for Netris network offerings * Fix UI loading for services when network mode changes for external providers and show vm autoscaling always set to true / non-editable for netris when LB is selected i.e, natted mode * Revert removed comment --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> * merge conflict * systemvm: fix VPC VPN issue when network id is bigger than 1000 on Netris * show networks for vnf nic mappings * Add a check to prevent re-adding existing ACLs * Add a check to prevent re-adding existing ACLs * change message compare statement * Add check for vnet name length and cleanup vpc and vnet resources on failure of creation of allocation * add name length validation to update path --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> Co-authored-by: Wei Zhou <weizhou@apache.org> * Add support to the VPC response to display if redundant routers are supported Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Hide VPN sections and prevent enabling it when service isn't enabled in the vpc offering * Hide VPN sections and prevent enabling it when service isn't enabled in the vpc offering * update --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Fix issue with adding external node to a CKS cluster with Netris offering Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Update upgrade path to reclaim vnets of deleted netris networks, improve log and hide add upstream route for Netris routed networks * Update upgrade path to reclaim vnets of deleted netris networks, improve log and hide add upstream route for Netris routed networks * Apply suggestions from code review * hide upstream route banner for netris networks altogether * fix migration of netris VRs and fix reclaim vnet query --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: Nicolas Vazquez <nicovazquez90@gmail.com> * Allocate IP from Netris (Provider) range when IP is not specified during associate * Allocate IP from Netris (Provider) range when IP is not specified during associate * update javadoc and method name --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Fix VRRP gateway IP conflict on redundant VPC routers with external network providers (Netris/NSX) Co-authored-by: Pearl Dsilva <pearl1594@gmail.com> * VPN: Reconfigure static nat and static route on fail over to backup VR - for redundant VPC VRs * VPN: Reconfigure static nat and static route on fail over to backup VR - for redundant VPC VRs * address comment * add stacktrace to the log --------- * Fix issue with deletion of vpn connection for DualStack redundant VPCs * fix precommit ci issue * Add Netris to the range limit check (apache#129) Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> * Netris: address review comments on static NAT, L2 and offering form - Match the legacy static NAT rule only when it was created for the requested public IP, on both create and delete - Roll back the dedicated L2 VPC if vNet creation fails - Hide the L2 guest type for Netris in the add offering form - Add VPN to the external provider service map for Netris only - Fix the L2 offering services javadoc * Update Netris SDK version to 1.0.1 * Move to the correct schema file * systemvm: bind dnsmasq and password server to router IP when external provider owns the gateway --------- Co-authored-by: Pearl Dsilva <pearl1954@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> Co-authored-by: Wei Zhou <weizhou@apache.org>
The agent matched the uuids recorded in the backup against the file name in each volume's path. A volume's path is not its uuid once the volume was migrated to another pool, or when it was created from a snapshot or an upload, so such a volume did not match: the restore of an existing instance failed with "not attached to the instance any more", after the volumes before it had already been overwritten from the backup. NASBackupProvider now picks the volume to restore each backup into by the volume uuid, which it has, and sends the volume list in the order of the backed up volumes. When none of the uuids match (an instance created from the backup) the volumes are taken in device id order as before; when only some match, the restore is refused before anything is sent to the host. The agent pairs the two lists by position again.
…ore-volume-mapping # Conflicts: # plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java
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
20 participants
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.
Description
restoreBackupof a KVM instance with more than one data disk can write abacked up disk into a different volume than the one it was taken from. This happens only on a second or further restore of a VM from a backup that was taken. ie: create backup of VM, restore 1, then restore 2 again.
success: true, but the appearance of the bug is not 100% because the enumeration can randomly be the same as before. I recommend trying instances with 5 or 7 data disks for testing where occurrence is very likely.fails converting the larger image into the smaller volume, and returns an error after the data is already
destroyed, with no rollback
createVMFromBackupandrestoreVolumeFromBackupAndAttachToVMare not affectedReproduced on 4.22.1.0 with KVM + Ceph/RBD primary storage and the NAS backup provider.
At first, this can be a very silent error, especially if the VM uses mount by LABEL/UUID, as the VM will mount the data to the right mount paths, but volume A now contains the data of volume B ( including LABELs) and volume B contains data for volume A.
Logs
case 1 - different disk sizes
The logs show output showing this bug on the VM, with 2 data volumes
diska,diskb- all stored onCeph primary storageand backed up via backup and recovery plugin tocephfsstorage.diskcheckis a simple bash util that prints useful data from the disks on a remote machine and connects via ssh.If the disk sizes are the same, the second restore works but disk can be swapped depending on the scenario how disk are renumbered.
Root cause
NASBackupProvider.restoreVMBackupbuilds two lists the backed up volumes and the instance's currentvolumes, each sorted by device id, and
LibvirtRestoreBackupCommandWrapper.restoreVolumesOfExistingVMconsumes them index by index:
BackupManagerImpl.restoreBackup-importRestoredVM-KVMGuru.importVirtualMachineFromBackupre-attaches every data disk with
getNextAvailableDeviceId(). The volumes are never detached first, sotheir current ids still count as "in use" and the helper can never return the id a volume already holds —
every data disk is shifted by one slot on every restore.
A restore is therefore only correct while the instance's device ids still match the ones recorded in the
backup. The first restore satisfies that and then breaks it for the next one.
Behaviour, restore by restore
The first restore is always correct. It is the restore itself that can plant the fault, so the damage only
appears on second restore.
Restore #1 — succeeds, but renumbers the disks
The device ids recorded in the backup still match the instance, so the two sorted lists line up and every
backup lands in its own volume. Verified on a 2-data-disk instance by reading the volumes directly from
primary storage — each still held its own filesystem.
But on completion
KVMGuru.importVirtualMachineFromBackupre-attaches the data disks withgetNextAvailableDeviceId():t
After the first restore, diskA gets id 4 because ids 1 and 2 are taken; id 3 is cdrom, so the next free id is 4. diskb gets id 1 because the id was just freed.
Restore #2 — the damage, and what it looks like depends on the disk sizes
Sorted by device id the instance is now
[ROOT, demo-disk-b, demo-disk-a]while the backup is[ROOT, demo-disk-a, demo-disk-b], so index 1 and 2 point at each other's volumes.Equal-sized data disks (1 GB + 1 GB) — silent corruption. Both images fit their new targets, so
qemu-imgsucceeds and the API returnssuccess: true. The volume contents are simply exchanged:A guest that mounts by
LABEL=/UUID=still mounts everything at the right paths, because the labelstravel with the filesystems — so nothing looks wrong from inside the instance, while CloudStack's
volume-to-content mapping is now wrong for every per-volume operation (detach, delete, snapshot, resize,
restore single volume).
Differently sized data disks (1 GB + 2 GB), data loss, reported as a failure.
The loop processes the disks in order and only aborts on the second pairing, so the first one has already been written.
The API returns:
and the agent log shows the real cause — the 2 GB backup being written into the 1 GB volume:
State afterwards — the restore is reported as failed, but one volume is already gone, with no rollback:
Both volumes still report
Readyin CloudStack. An operator seeing error 530 would reasonably assumenothing happened.
How to reproduce
Instance with two data disks, a NAS backup offering, KVM primary storage. Use two different sizes to get
the loud failure, if you want to reproduce the disk swap, I recommend using more data disks, as there is a chance that reumbering accdientaly get right and the bug does not occur. So trying like 5 or 7 data disks should most likely show the bug.
Types of changes
How Has This Been Tested?
Unit tests — three added, all of which fail without the patch:
KVMGuruTestLibvirtRestoreBackupCommandWrapperTestEvery pre-existing test in both classes passes either way.
LibvirtRestoreBackupCommandWrapperTest.testRestoreOfExistingVmMapsBackupsToVolumesByUuid— theinstance's volumes arrive in a different order than the backed up volumes; each backup must still be
written into the volume it was taken from. Also asserts with
never()that neither data disk's backup iswritten into the other one.
LibvirtRestoreBackupCommandWrapperTest.testRestoreOfExistingVmFailsWhenBackedUpVolumeIsNoLongerAttachedKVMGuruTest.testImportVirtualMachineFromBackupReinstatesRecordedDeviceIds— a data disk recorded ondevice id 5 must be re-attached on device id 5, and
getNextAvailableDeviceId()must not be consulted.Live testing — a patched build was deployed to a KVM + Ceph/RBD zone (management server and KVM
agents). The same 2-data-disk instance was restored three times in place:
0,1,2on every restore (unpatched:1,2→4,1after the first)rbd exportand from inside the guestcreateVMFromBackupverified unaffected