-
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
Use shared lock on disks for dependent VMs #178
Conversation
b78fb50
to
dcd3b39
Compare
9f07f39
to
7b42770
Compare
The check to see if a template image is being used is pretty complicated, extract it to a method, isTemplateBeingUsed(). Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
7b42770
to
945fe00
Compare
/ost |
945fe00
to
724c9e1
Compare
/ost |
seems to be failing |
@michalskrivanek |
in 945fe00 so now it's good! |
|
||
if (!Guid.isNullOrEmpty(getParameters().getImageId()) || | ||
getDiskImage() != null && | ||
!isThinTemplate(getDiskImage())) { |
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 now if we download a disk of a vm that is set with thin-provisioning, we won't lock the vm? something is suspicious here
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... it's probably not needed
if (getDiskImage() != null) { | ||
|
||
// If the disk is a thin template overlay we only lock with a shared lock | ||
if (getDiskImage() != null && !isThinTemplate(getDiskImage())) { |
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.
what's the reason for not locking cloned disks also with shared locks?
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.
cloned disks have a different id, no? they should be affected by this
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.
yes, we generate a different ID from them but regardless of this issue - if all we care of is that the disk is not removed or moved or converted (and such) then we don't have to create an exclusive lock for the disks at all
another way to think about it is to say exclusive locks = write locks and shared locks = read locks (although it's not always true with our engine locks) so if all we do when downloading a disk is read them, then a shared lock seems be more appropriate
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.
ah ok, I thought the comment was about an issue... I definitely agree there is no reason to use an exclusive lock here
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.
actually, there is one reason - this command is used for upload too :)
6607a2a
to
b5f9d38
Compare
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.
looks good besides that change to the vm-lock which seems redundant
if (!Guid.isNullOrEmpty(getParameters().getImageId()) && getDiskImage() != null) { | ||
|
||
if (!Guid.isNullOrEmpty(getParameters().getImageId()) || | ||
getDiskImage() != null) { |
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.
do we need this change? (we have to check getDiskImage() != null
, otherwise we'll get NPE in the following line)
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.
nope, leftover from previous change
When downloading a disk that is an overlay of a template, take only a shared lock on the disk and skip the locked disk validation, this would allow downloading disks of thin VMs without failing on the disk being locked. Bug-Url: Bug-Url: https://bugzilla.redhat.com/1879391 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
b5f9d38
to
4c3ab00
Compare
/ost |
When downloading a disk that is an overlay of a template, take only a shared lock on the disk and skip the locked disk validation, this would allow downloading disks of thin VMs without failing on the disk being locked