weizhouapache commented on PR #14426: URL: https://github.com/apache/cloudstack/pull/14426#issuecomment-6267713416
> Went through this by hand first and then had Claude Fable dig through the paths I wasn't sure about. Really great work, we've wanted this for years! > > One thing I think is blocking: in migrateRBDVolume everything after the pivot can still fail the migration. The delete of the source volume in the finally isn't guarded and pool.refresh() is inside the try. If one of those throws the agent reports a failure, mgmt server keeps the volume on the old pool, but qemu is already writing to the new image. Next stop/start the instance boots from the old image or doesn't start at all. Thats your 5th run failure I think. Once the PIVOT returned the migration is done, delete and refresh should just log and not touch the answer anymore. Was the source image still there after that failed run btw? > addressed in https://github.com/apache/cloudstack/pull/14426/commits/abdaa03f0e01dec1785d43695da6dd1a5a723592 > The LibvirtStorageVolumeXMLParser fix is correct (args were swapped) but it changes format detection for every volume on every libvirt pool, that fallback has been in effect since forever. I'd put that in a seperate PR with its own testing. LXC .tar templates for example get probed as raw now. > reverted in https://github.com/apache/cloudstack/pull/14426/commits/dac5d81dbe0707ca2ed0133a88ccc06eebb926bf and fixed an issue in NFS->RBD->NFS migration: https://github.com/apache/cloudstack/pull/14426/commits/c8b302d6a30c81274b3c7760eb02699de2e4bc77 > Agree with dropping NFS/SMP/local live migration from this PR. The server gate, isFileBasedPool in the wrapper, smoke tests and the migrateRBDVolume name should then go together. > addressed in https://github.com/apache/cloudstack/pull/14426/commits/3a1a22c94453bf0caaa5c5399f7a64b94251f95d > Checked and fine: the encrypted copy in copyPhysicalDisk is only reached from the primary to primary copy with both passphrases set, templates and snapshots are not affected. Endpoint selector change only hits running instances. > > Ran the unit tests from the description on the branch with JDK 17, 238 tests, all pass. > > I don't have a Ceph test env at the moment. CCC is coming up in November, can you give me a demo there so we can verify this together? Would love to see it live. -- 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]
