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.

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

Virt cdrom fix #57479

Merged
merged 3 commits into from
Jul 31, 2020
Merged

Virt cdrom fix #57479

merged 3 commits into from
Jul 31, 2020

Conversation

cbosdo
Copy link
Contributor

@cbosdo cbosdo commented May 27, 2020

What does this PR do?

This issue fixes some issues with the handling of CDROM images attaching and changing on VM created or updated by virt.init() and virt.update() functions.

What issues does this PR fix or reference?

Fixes: #57477

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@cbosdo cbosdo requested a review from a team as a code owner May 27, 2020 17:21
@ghost ghost requested review from twangboy and removed request for a team May 27, 2020 17:21
twangboy
twangboy previously approved these changes May 27, 2020
@twangboy twangboy added this to the Approved milestone May 27, 2020
waynew
waynew previously approved these changes May 27, 2020
@waynew
Copy link
Contributor

waynew commented May 27, 2020

Pretty sure this will get merged after the code thaw 🧊 🌞

@cbosdo
Copy link
Contributor Author

cbosdo commented May 27, 2020

Pretty sure this will get merged after the code thaw ice_cube sun_with_face

you mean it's likely not to hit Sodium?

Akm0d
Akm0d previously approved these changes May 27, 2020
@cbosdo
Copy link
Contributor Author

cbosdo commented Jun 9, 2020

Rebased on master, added more tests and fixed an uncovered bug in the target name generation code

@cbosdo
Copy link
Contributor Author

cbosdo commented Jul 10, 2020

rebased on master

@sagetherage sagetherage added Bug broken, incorrect, or confusing behavior Magnesium Mg release after Na prior to Al severity-high 2nd top severity, seen by most users, causes major problems labels Jul 22, 2020
@cbosdo
Copy link
Contributor Author

cbosdo commented Jul 23, 2020

Updated on latest master

"{0}{1}".format(prefix, string.ascii_lowercase[i])
for i in range(disks_count)
if "{0}{1}".format(prefix, string.ascii_lowercase[i]) not in targets
][0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good but logically it should be more effective if we'll not build the entire list to just get the first item. Maybe something like this?

for i in range(disks_count):
    ret = "{0}{1}".format(prefix, string.ascii_lowercase[i])
    if ret not in targets:
        return ret

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, I'll change that then

@cbosdo
Copy link
Contributor Author

cbosdo commented Jul 24, 2020

@DmitryKuzmenko Just included your suggestion

DmitryKuzmenko
DmitryKuzmenko previously approved these changes Jul 24, 2020
Akm0d
Akm0d previously approved these changes Jul 29, 2020
cdrom sources can't be of format qcow2. Force raw as the default if
needed when creating VM with source_file set for the cdrom.
When a DVD device on a VM has a remote source, virt.update needs to be
able to handle detaching it and attaching a file image live.
@cbosdo
Copy link
Contributor Author

cbosdo commented Jul 31, 2020

rebased on master

@dwoz dwoz merged commit ebab868 into saltstack:master Jul 31, 2020
@cbosdo cbosdo deleted the virt-cdrom-fix branch August 1, 2020 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Magnesium Mg release after Na prior to Al severity-high 2nd top severity, seen by most users, causes major problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

virt.init doesn't generate proper disk target devices
8 participants