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

Introduce virt-xml into disk APIs #394

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Introduce virt-xml into disk APIs #394

wants to merge 2 commits into from

Conversation

skobyda
Copy link
Contributor

@skobyda skobyda commented Sep 24, 2021

No description provided.

@KKoukiou
Copy link
Collaborator

Fixed the filesystem, disks still broken: https://logs.cockpit-project.org/logs/pull-394-20210924-130035-1e3d9861-fedora-34/log.html

@skobyda skobyda removed the no-test label Sep 30, 2021
@skobyda skobyda added blocked and removed needs-work labels Oct 8, 2021
@skobyda
Copy link
Contributor Author

skobyda commented Oct 8, 2021 •
edited
Loading

This is currently blocked on virt-manager/virt-manager#311 , as it fails https://logs.cockpit-project.org/logs/pull-394-20211004-110348-b16aad72-fedora-34/log.html#21-2

@skobyda skobyda changed the title Introduce virt-xml into disk and filesystem's attach/detach APIs Introduce virt-xml into disk APIs Oct 15, 2021
@skobyda skobyda force-pushed the virtXmlDisksFilesystems branch 2 times, most recently from 79830af to 0138180 Compare October 15, 2021 12:54
@martinpitt
Copy link
Member

The fix landed in upstream virt-manager 26 days ago, but not released yet AFAICS.

@martinpitt martinpitt marked this pull request as draft February 14, 2022 07:19
@skobyda
Copy link
Contributor Author

skobyda commented Apr 7, 2022

There is already a release in fedora 36 and ubuntu 2204. I can get going with this PR :)

@skobyda skobyda removed the blocked label Sep 7, 2022
@skobyda skobyda marked this pull request as ready for review September 7, 2022 08:54
@skobyda
Copy link
Contributor Author

skobyda commented Sep 7, 2022

This now passes on all distros where virt-xml (virt-manager package) is >= 4.0 . I wonder if I should just keep this PR blocked until it's released in the other distros too, or just write an exception/antipattern for it

martinpitt
martinpitt requested changes Sep 8, 2022
View reviewed changes
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

I see that using virt-xml is probably nicer in the long run, as it's a bit more abstract than directly modifying the XML. But as long as this still breaks on so many supported OSes, I feel that we shouldn't land this -- this would appear as a regression. So either we need to support both code paths for now (ugh!), or just shelve this for a while?

src/libvirtApi/domain.js Outdated Show resolved Hide resolved
src/libvirt-xml-parse.js Outdated Show resolved Hide resolved
src/libvirt-xml-parse.js Outdated Show resolved Hide resolved
@skobyda skobyda force-pushed the virtXmlDisksFilesystems branch 4 times, most recently from fc84e8c to 5ee713d Compare January 23, 2023 10:55
@KKoukiou
Copy link
Collaborator

@skobyda this looks still blocked at least for rhel-8-8, as we still update that, but seems like virt-xml is not updated there.

@skobyda
Copy link
Contributor Author

skobyda commented Feb 2, 2023

@skobyda this looks still blocked at least for rhel-8-8, as we still update that, but seems like virt-xml is not updated there.

Yeah I updated this PR last week to see where it still continues to fail. I'd not try to push this until every distro gets new version of virt-xml

@KKoukiou KKoukiou removed their request for review May 24, 2023 07:06
@KKoukiou
Copy link
Collaborator

This is probably unblocked by now?

@skobyda
Copy link
Contributor Author

skobyda commented Oct 13, 2023

This is probably unblocked by now?

No idea, let's re-trigger

cockpituous
cockpituous reviewed Oct 13, 2023
View reviewed changes
src/libvirtApi/domain.js
Comment on lines +1126 to +1127
addressType = "pci";
if (busType === "usb" || busType === "scsi" || busType === "sata") {
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 added lines are not executed by any test. Details

src/libvirtApi/domain.js
// https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_validate.c
addressBus = 0;
if (busType === "usb")
addressType = "usb";
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test. Details

@skobyda
Copy link
Contributor Author

skobyda commented Oct 13, 2023

This is probably unblocked by now?

Nope, old version of virt-xml is still present on centos-8-stream and rhel-8-*, so this PR will not get merged until we stop supporting those distros

@mvollmer
Copy link
Member

@martinpitt, @KKoukiou, what's the point of using virt-xml? Does it augment the XML with its own knowledge of best defaults? Otherwise this looks like a step back to me, in the JSON > XML > CSV hierachy. (I think we need to worry about quoting when constructing the virt-xml invocations, for example.)

@mvollmer
Copy link
Member

Please reopen if this is still wanted.

@mvollmer mvollmer closed this May 14, 2024
@KKoukiou
Copy link
Collaborator

@mvollmer I am severely out of context on this repo but the virt-xml direction was desired as it picks up correct defaults when attaching devices and removes a lot of custom logic from our side. I dont have examples right now handy.

@mvollmer
Copy link
Member

but the virt-xml direction was desired as it picks up correct defaults when attaching devices and removes a lot of custom logic from our side.

Ok, convinced.

@mvollmer mvollmer reopened this May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cockpituous cockpituous cockpituous left review comments

@martinpitt martinpitt martinpitt requested changes

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.
5 participants
@skobyda @KKoukiou @martinpitt @mvollmer @cockpituous