-
Notifications
You must be signed in to change notification settings - Fork 270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core: use manged_path for running VMs with MBS devices #104
Changes from all commits
9b7a408
c547bfb
052477c
c7a28af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,7 +142,9 @@ public VDSReturnValue attachManagedBlockStorageDisk(ManagedBlockStorageDisk disk | |
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change makes sense - but is required only to allow LSM which is not supported yet. Until we do this change, all disks will use blank uuid. Adding the sd id is likely There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the vdsm PR included these changes so I tested them all together, I can separate it now since it's merged in vdsm |
||
disk.setConnectionInfo(returnValue.getActionReturnValue()); | ||
AttachManagedBlockStorageVolumeVDSCommandParameters params = | ||
new AttachManagedBlockStorageVolumeVDSCommandParameters(vds, returnValue.getActionReturnValue()); | ||
new AttachManagedBlockStorageVolumeVDSCommandParameters(vds, | ||
returnValue.getActionReturnValue(), | ||
disk.getStorageIds().get(0)); | ||
params.setVolumeId(disk.getImageId()); | ||
return resourceManager.runVdsCommand(VDSCommandType.AttachManagedBlockStorageVolume, params); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2437,21 +2437,22 @@ private void writeDiskSource(VmDevice device, Disk disk, String dev, DiskVmEleme | |
case MANAGED_BLOCK_STORAGE: | ||
ManagedBlockStorageDisk managedBlockStorageDisk = (ManagedBlockStorageDisk) disk; | ||
Map<String, String> metadata = new HashMap<>(); | ||
String path = (String) managedBlockStorageDisk.getDevice().get(DeviceInfoReturn.PATH); | ||
String path; | ||
if (Version.v4_7.lessOrEquals(vm.getCompatibilityVersion())) { | ||
path = (String) managedBlockStorageDisk.getDevice().get(DeviceInfoReturn.MANAGED_PATH); | ||
} else { | ||
path = (String) managedBlockStorageDisk.getDevice().get(DeviceInfoReturn.PATH); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this work for running vm when we upgrade the cluster version while the vm is running? The vm will start using "path". After the upgrade, if we migrate the vm, we will use "managed_path", and the migration will likely fail. Can we always use "managed_path"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about older cluster levels, if we do live migration in a 4.6 cluster for example? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Migration to/from 4.6 cluster will not work with managed_path. But since this is tech preview we don't need to care about it. If we want to avoid this issue we can use managed_path only for lightos, and continue to use path for rbd and mpath. But this means we carry to ways to work with mbs storage, which is bad. I this to break this now and not later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now it is clear, we use manage_path for cluster level 4.7, and path for older cluster levels since vdsm in older versions did not support managed_path. |
||
|
||
if (managedBlockStorageDisk.getCinderVolumeDriver() == CinderVolumeDriver.RBD) { | ||
// For rbd we need to pass the entire path since we rely on more than a single | ||
// variable e.g: /dev/rbd/<pool-name>/<vol-name> | ||
metadata = Collections.singletonMap("RBD", path); | ||
metadata.put("RBD", path); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So before we started with empty map, and then replaced it with Collections.singletonMap? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unfortunately it is not a cleanup, singletonMap was incorrect because it's immutable which would prevent setting metadata.put("managed", "true"); later on |
||
} else if (managedBlockStorageDisk.getCinderVolumeDriver() == CinderVolumeDriver.BLOCK) { | ||
Map<String, Object> attachment = | ||
(Map<String, Object>) managedBlockStorageDisk.getDevice().get(DeviceInfoReturn.ATTACHMENT); | ||
metadata = Map.of( | ||
"GUID", (String)attachment.get(DeviceInfoReturn.SCSI_WWN), | ||
"managed", "true" | ||
); | ||
metadata.put("GUID", (String)attachment.get(DeviceInfoReturn.SCSI_WWN)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is also nicer, but is this require to get this change merged? |
||
} | ||
|
||
metadata.put("managed", "true"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this the only required change? |
||
writer.writeAttributeString("dev", path); | ||
diskMetadata.put(dev, metadata); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is similar to the previous commit adding sd_id, no?