Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Skip disk profiles validation on memory disk #594

Merged
merged 1 commit into from
Aug 23, 2022
Merged

Skip disk profiles validation on memory disk #594

merged 1 commit into from
Aug 23, 2022

Conversation

liranr23
Copy link
Member

When we create the memory or metadata disks they are a result of live
snapshot with memory or hibernation command. In some cases the user can
own the VM, including the option to execute the above command but he may
not have the disk profile permission to add those disks.

This patch will skip the permission validation in those cases, allowing
a user that owns the VM and capable of executing the command to not fail
on the underlying addDisk command.

Change-Id: Ib04048d652c8d98df84ffccc4babd71730b218dc
Bug-Url: https://bugzilla.redhat.com/1565183
Signed-off-by: Liran Rotenberg lrotenbe@redhat.com

@liranr23 liranr23 added the virt label Aug 17, 2022
@liranr23 liranr23 force-pushed the skip_disk_ownership branch 2 times, most recently from 16c2fba to 19cda9d Compare August 17, 2022 14:00
@liranr23
Copy link
Member Author

/ost

ahadas
ahadas approved these changes Aug 18, 2022
View reviewed changes
Copy link
Member

@ahadas ahadas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bennyz the commit message is focused on a concrete bug but it's actually a broader change of not setting disk profiles to memory dump/metadata disks. this looks like a better approach to me since those are not really virtual disks. what do you think?

@ahadas ahadas self-requested a review August 18, 2022 07:12
ahadas
ahadas reviewed Aug 18, 2022
View reviewed changes
Copy link
Member

@ahadas ahadas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, minor comment inside

.../manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/DiskProfileHelper.java Outdated Show resolved Hide resolved
@ahadas ahadas self-requested a review August 21, 2022 07:32
ahadas
ahadas reviewed Aug 21, 2022
View reviewed changes
Copy link
Member

@ahadas ahadas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, please update the commit message to better reflect the change

@liranr23
Copy link
Member Author

/ost

smelamud
smelamud approved these changes Aug 21, 2022
View reviewed changes
Copy link
Member

@smelamud smelamud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo in the commit message ("virutal disks").

@liranr23
Copy link
Member Author

There is a typo in the commit message ("virutal disks").

thanks, updated.

@ahadas
Copy link
Member

ahadas commented Aug 22, 2022 •
edited
Loading

There is a typo in the commit message ("virutal disks").

thanks, updated.

also the following doesn't describe accurately what this PR does now:
This patch will skip the permission validation in those cases, allowing a user to create attach disks for disks that are not a real virtual disks.

@liranr23
Copy link
Member Author

also the following doesn't describe accurately what this PR does now: This patch will skip the permission validation in those cases, allowing a user to create attach disks for disks that are not a real virtual disks.

changed.

ahadas
ahadas approved these changes Aug 22, 2022
View reviewed changes
Copy link
Member

@ahadas ahadas left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch will skip setting the profile in those cases, allowing a user to create attach disks for disks that are not a real virtual disks.
could have been phrased better (e.g., This patch skips setting the profile in these cases, allowing the user to create those disks even when the user doesn't have a permission to set them with a disk profile) but let's not block it on phrasing, it's close enough

@liranr23 @ahadas
core: skip disk profiles validation on memory disk
9b4bf2c 
When we create the memory or metadata disks they are a result of other
command. These disks are not act as virtual disks. In some cases the
user can have permission to execute these command, but he may not have
the disk profile permission to add those disks.

This patch will skip setting the profile in those cases, allowing a user
to create attach disks for disks that are not a real virtual disks.

Change-Id: Ib04048d652c8d98df84ffccc4babd71730b218dc
Bug-Url: https://bugzilla.redhat.com/1565183
Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>
@ahadas
Copy link
Member

ahadas commented Aug 23, 2022

/ost

@ahadas ahadas merged commit 27dc8ee into oVirt:master Aug 23, 2022
@liranr23 liranr23 deleted the skip_disk_ownership branch August 23, 2022 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smelamud smelamud smelamud approved these changes

@ahadas ahadas ahadas approved these changes

@bennyz bennyz Awaiting requested review from bennyz bennyz is a code owner

@ljelinkova ljelinkova Awaiting requested review from ljelinkova

@mz-pdm mz-pdm Awaiting requested review from mz-pdm

@emesika emesika Awaiting requested review from emesika emesika is a code owner

@mwperina mwperina Awaiting requested review from mwperina mwperina is a code owner

@michalskrivanek michalskrivanek Awaiting requested review from michalskrivanek michalskrivanek is a code owner

@oliel oliel Awaiting requested review from oliel

@sgratch sgratch Awaiting requested review from sgratch sgratch is a code owner

@didib didib Awaiting requested review from didib didib is a code owner

Assignees
No one assigned
Labels
virt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@liranr23 @ahadas @smelamud