-
Notifications
You must be signed in to change notification settings - Fork 261
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: fix copying from MBS to image domain #349
Conversation
There is no need to reconstruct the sourceDisk as ManagedBlockDisk as it is already of this type if the source domain is a Managed Block Storage Domain, and it will fail later on as the conversion method createManagedBlockDiskFromDiskImage assigns the target's id to the constructed object which is incorrect, as this is the source disk. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
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.
can you please verify this with copy a template disk from/to a Managed Block Storage domain?
that was the original fix (https://bugzilla.redhat.com/2006745)
ack, works |
@@ -167,7 +167,7 @@ protected void createVolumes() { | |||
// Attach source to host (if source is Managed Block Storage) | |||
// TODO: handle failures | |||
if (sourceDomainType == StorageDomainType.ManagedBlockStorage) { | |||
String sourcePath = attachVolume(createManagedBlockDiskFromDiskImage(sourceDisk)); |
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.
so how about renaming createManagedBlockDiskFromDiskImage
to something like createDestinationManagedBlockDiskFromSourceDiskImage
? a bit longer but would make it more clear that it's not an equivalent to casting..
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.
or maybe toDestinationManagedBlockDisk(DiskImage sourceImage)
?
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.
I agree although I am not sure about the naming, maybe a better way would be to have createManagedBlockDiskFromDiskImage
do the casting if object is already a ManagedBlockDisk
but that could be confusing too
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.
yeah..
There is no need to reconstruct the
sourceDisk
asManagedBlockDisk
as it is already of this type if the source domain is a Managed Block Storage Domain, and it will fail later on as the conversion methodcreateManagedBlockDiskFromDiskImage
assigns the target's id to the constructed object which is incorrect, as this is the source disk.