Drop usage of virtinterfaced #1782

martinpitt · 2024-08-26T10:51:41Z

virtinterfaced is being deprecated [1], and at least some distros want
to get rid of the dependency.

Replace it with reading ip --json a instead to get all the interface
names.

[1] https://listman.redhat.com/archives/libvir-list/2020-December/msg00183.html

Goes on top of PR Revert "libvirtApi remove use of interfaceGetAll" #1785 for cleanliness
Confirm with @KKoukiou that this is really legit

martinpitt · 2024-08-26T10:54:30Z

Our bots images still have libvirt-daemon-driver-interface. I locally prepared an image without it, and started the whole test suite. That will take a while, will update this comment/PR status once its done. However, TF runs should already happen without that dep.

martinpitt · 2024-08-26T12:06:46Z

On TF, TestMachinesNetworks.testNetworksCreate fails. I also get that locally, plus a whole lot of other failures which don't look related to virsh iface-list (which is the one thing that "legitimately" breaks here). This also reflects in the tumbleweed failure where this PR removes the "systemctl enable" special case.

Added fix.

test/check-machines-networks

jelly

Not convinced at all that "nothing uses this":

getBootOrderDevices uses vm.interfaces which seems to come from this reducer state if I am reading this correctly:

And used here:

            } else if (boot.type === "network") {
                const dev = ifaces[0];
                if (dev) {
                    ifaces.splice(0, 1);
                    devices.push({
                        device: dev,
                        bootOrder: i + 1, // bootOrder begins at 1
                        type: "network"
                    });
                }
            }

src/libvirtApi/interface.js

martinpitt · 2024-08-26T13:16:02Z

getBootOrderDevices uses vm.interfaces

Sure, but there's no doubt about a VM having a list of network interfaces. virtinterfaced is tracking the host's network interface. As far as I understand the deprecation announcement, the main use case was

functions that list current interfaces and get their current live status (i.e. based on sending/receiving netlink
messages, not on the contents of the host network config files). Even on that front, the one commonly used example that I know know of is virt-manager, which had used virInterface*() to get a list of bridges/ethernets available for guest network connections

... and the only place which does that is that test helper getNetworkDevice(), which I changed to sysfs.

I could of course misunderstand all of this. @KKoukiou do you remember something about this?

jelly · 2024-08-26T13:45:19Z

getBootOrderDevices uses vm.interfaces

Sure, but there's no doubt about a VM having a list of network interfaces. virtinterfaced is tracking the host's network interface. As far as I understand the deprecation announcement, the main use case was

functions that list current interfaces and get their current live status (i.e. based on sending/receiving netlink
messages, not on the contents of the host network config files). Even on that front, the one commonly used example that I know know of is virt-manager, which had used virInterface*() to get a list of bridges/ethernets available for guest network connections

... and the only place which does that is that test helper getNetworkDevice(), which I changed to sysfs.

I could of course misunderstand all of this. @KKoukiou do you remember something about this?

The problem here is that I cannot read :)

vm.interfaces != interfaces. We obtain interfaces here https://github.com/cockpit-project/cockpit-machines/blob/main/src/app.jsx#L250 and pass it down to HostVmList here https://github.com/cockpit-project/cockpit-machines/blob/main/src/app.jsx#L340

And that component does not use interfaces. So kill that please, that should have been the first removal commit imo instead of removing the API call without explaining why it was unused.

Next up remove interfaces from combineReduces and the interfaces function in reducers.js.

jfehlig · 2024-08-26T17:28:07Z

/**

Returns a list of potential physical devices suitable as network devices

by merging all network node devices and interfaces.

@returns {array}
*/

I assume this means host network devices that could be used by VMs, right? Most commonly, that means a bridge type device. For reference, I think this is how virt-manager handles that (see lines 42-110)

https://github.com/virt-manager/virt-manager/blob/main/virtinst/devices/interface.py#L94

Disclaimer: I'm slightly familiar with virt-manager code, but far from an expert :-)

martinpitt · 2024-08-27T04:31:18Z

@jfehlig Thanks for the pointer! But _host_default_bridge() can't be the whole story -- creating networks is in no way restricted to bridges. But I suppose the equivalent functionality is this:

https://github.com/virt-manager/virt-manager/blob/1fef5d8661118a4f00a8689b323d275144b39dbe/virtManager/device/netlist.py#L131

jfehlig · 2024-08-27T17:16:13Z

@jfehlig Thanks for the pointer! But _host_default_bridge() can't be the whole story -- creating networks is in no way restricted to bridges.

The terminology is starting to confuse me. Let's clarify it :-).

https://libvirt.org/html/libvirt-libvirt-network.html
libvirt fully supports managing virtual networks via the virNetwork* set of APIs, backed by virtnetworkd service. There are many apps and users of this functionality. It works great and is well tested across various distros.

https://libvirt.org/html/libvirt-libvirt-interface.html
libvirt (woefully inadequately IMO) supports managing host network interface devices via the virInterface* set of APIs backed by virtinterfaced service. Beyond cockpit-machines, I'm not aware of any users of this functionality. virt-manager removed it long ago. It's definitely not well tested. The only virtinerfaced backend that works across distros is the udev one, which supports just a handful of read-only operations that likely return inconsistent results across distros.

https://libvirt.org/formatdomain.html#network-interfaces
libvirt supports managing virtual interfaces used by VMs via the <interface> XML config. To be useful, these need to be plugged into a network.

Ok, back to the topic. virt-manager can be used to create/manage libvirt virtual networks. It cannot be used to create/manage any host network interface devices. The virt-manager code I referenced is used to find a host network interface of type 'bridge', which will then be displayed as a possible network to attach virtual interface(s) of new VMs. By default, virt-manager prefers connecting virtual interfaces to a bridge, but the list of possible networks will include any active libvirt virtual networks too. If no bridge is found, virt-manager doesn't provide a way to create one. It simply excludes 'bridge device' from the list of possible networks.

https://github.com/virt-manager/virt-manager/blob/1fef5d8661118a4f00a8689b323d275144b39dbe/virtManager/device/netlist.py#L131

IIUC, that code is used to populate the 'Network selection' list I mentioned above. E.g. the list shown on step 5 of 5 of the new VM wizard.

Nykseli · 2024-08-30T08:16:28Z

test/check-machines-networks

-
-    return device
+def getNetworkDefaultRouteDevice(m):
+    routes = json.loads(m.execute("ip --json route show default"))


Is show default needed here? At least on fedora 40 and tumbleweed, the test will pass if you use ip --json route

Yes, but only by happenstance -- as soon as you have more than one route, and the default one isn't the first one, that will fail. I'd rather make it explicit.

I see. That's a fair point

It seems the problem is that there is no default route on those test systems at this point. The previous code did not pick a specific interface, just any first one, not even enabled. On one system here the first virsh command would have picked "lo". Does routing to outside the host actually need to work? If yes, then which destination? As example 127.0.0.1 then ip --json route get 127.0.0.1. If not then maybe hard coding "lo" is fine.

Ah, indeed -- it failed, as I locally tested in an inadequate way.

My main issue is that I have not the slightest idea what this operation in the test actually does -- it creates a libvirt network with a <dev> node, which happens to be "eth0" or "eth1". I assume this creates a bridge which forwards its traffic to the given host device? But then "lo" doesn't make much sense, does it?

So indeed I'll just drop the default here, that'll also pick "eth0". I don't know what exactly this does, but at least it stays compatible 😅

martinpitt · 2024-08-30T09:23:29Z

For the record -- this is way more involved than it initially looked. This was a quick attempt, but it apparently needs someone who actually has a clue about c-machines/libvirt. So it's back in my long queue of "fix someday", or "we need a c-machines maintainer again". 😢

Nykseli · 2024-09-26T09:50:44Z

I looked into this a bit and created a draft for replicating the old behavior with the ip command: #1839

martinpitt · 2024-09-27T15:05:16Z

Thanks @Nykseli ! I merged (conceptually, not git) the two MRs. I started with your commit, fixed up some things (like parsing MAC and Active properties), and added back the packaging updates. I also triggered a run against the tumbleweed refresh.

@jfehlig @KKoukiou does this look sensible to you now?

jelly

Small cleanup request

src/libvirtApi/interface.js

virtinterfaced is being deprecated [1], and at least some distros want to get rid of the dependency. Replace it with reading `ip --json a` instead to get all the interface names. Fixes cockpit-project#1777 [1] https://listman.redhat.com/archives/libvir-list/2020-December/msg00183.html Co-Authored-By: Martin Pitt <[email protected]>

KKoukiou

This replacement is reasonable considering the virtinterfaced is so undermaintained and causes visible crashes.

Let me know if you want me to review the code as well.

cockpituous · 2024-10-01T08:53:23Z

src/libvirtApi/interface.js

    } catch (ex) {
-        console.log('listInactiveInterfaces action for path', objPath, ex.toString());
+        console.warn("Failed to get interfaces with ip command:", ex.toString());


This added line is not executed by any test.

jelly

Can't tell if the debian failure is a flake or not, if not feel free to merge

martinpitt · 2024-10-01T09:21:23Z

Our tests are all green. Seems packit's TF integration is AWOL today -- but I feel comfortable to ignore, the previous version was all green, and the diff was just that little code cleanup.

jfehlig · 2024-10-01T15:57:44Z

Sorry for the late response, but I don't have anything to add anyhow other than thanks a lot for resolving this issue!

martinpitt force-pushed the drop-virtinterfaced branch from cde6d88 to 0e703fe Compare August 26, 2024 12:06

martinpitt marked this pull request as ready for review August 26, 2024 12:42

martinpitt requested review from KKoukiou and jelly August 26, 2024 12:48

martinpitt mentioned this pull request Aug 26, 2024

cockpit-machines still carries libvirt host interface management dependencies #1777

Closed

jelly reviewed Aug 26, 2024

View reviewed changes

test/check-machines-networks Outdated Show resolved Hide resolved

jelly requested changes Aug 26, 2024

View reviewed changes

src/libvirtApi/interface.js Show resolved Hide resolved

martinpitt force-pushed the drop-virtinterfaced branch 4 times, most recently from dc07a76 to 95d35ce Compare August 26, 2024 14:52

This comment was marked as outdated.

Sign in to view

martinpitt marked this pull request as draft August 26, 2024 16:46

This was referenced Aug 26, 2024

libvirtApi remove use of interfaceGetAll #1779

Merged

Revert "libvirtApi remove use of interfaceGetAll" #1785

Merged

martinpitt force-pushed the drop-virtinterfaced branch from 95d35ce to f22e922 Compare August 27, 2024 04:25

martinpitt added blocked and removed blocked labels Aug 27, 2024

Nykseli reviewed Aug 30, 2024

View reviewed changes

Lunarequest mentioned this pull request Sep 23, 2024

opensuse tumebleweed: don't install libvirt-daemon-driver-interface cockpit-project/bots#6904

Closed

1 task

martinpitt mentioned this pull request Sep 24, 2024

images: Drop libvirt-daemon-driver-interface from opensuse-tumbleweed cockpit-project/bots#6909

Merged

2 tasks

Nykseli mentioned this pull request Sep 26, 2024

Replace deprecated virtinterfaced calls with ip command #1839

Closed

martinpitt force-pushed the drop-virtinterfaced branch from f22e922 to 68ec0e9 Compare September 27, 2024 15:00

martinpitt requested review from JanZerebecki, jelly and KKoukiou and removed request for JanZerebecki and KKoukiou September 27, 2024 15:05

martinpitt marked this pull request as ready for review September 27, 2024 15:22

jelly requested changes Oct 1, 2024

View reviewed changes

src/libvirtApi/interface.js Show resolved Hide resolved

martinpitt force-pushed the drop-virtinterfaced branch from 68ec0e9 to b8b3be2 Compare October 1, 2024 08:32

martinpitt requested a review from jelly October 1, 2024 08:32

KKoukiou reviewed Oct 1, 2024

View reviewed changes

cockpituous reviewed Oct 1, 2024

View reviewed changes

jelly approved these changes Oct 1, 2024

View reviewed changes

martinpitt merged commit 11be3fa into cockpit-project:main Oct 1, 2024
24 of 29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop usage of virtinterfaced #1782

Drop usage of virtinterfaced #1782

martinpitt commented Aug 26, 2024 •

edited

Loading

martinpitt commented Aug 26, 2024

martinpitt commented Aug 26, 2024

jelly left a comment

martinpitt commented Aug 26, 2024 •

edited

Loading

jelly commented Aug 26, 2024

This comment was marked as outdated.

This comment was marked as outdated.

jfehlig commented Aug 26, 2024

martinpitt commented Aug 27, 2024

jfehlig commented Aug 27, 2024

Nykseli Aug 30, 2024

martinpitt Aug 30, 2024

Nykseli Aug 30, 2024

JanZerebecki Aug 30, 2024

martinpitt Sep 27, 2024

martinpitt commented Aug 30, 2024

Nykseli commented Sep 26, 2024

martinpitt commented Sep 27, 2024

jelly left a comment

KKoukiou left a comment

cockpituous Oct 1, 2024

jelly left a comment

martinpitt commented Oct 1, 2024

jfehlig commented Oct 1, 2024

Drop usage of virtinterfaced #1782

Drop usage of virtinterfaced #1782

Conversation

martinpitt commented Aug 26, 2024 • edited Loading

martinpitt commented Aug 26, 2024

martinpitt commented Aug 26, 2024

jelly left a comment

Choose a reason for hiding this comment

martinpitt commented Aug 26, 2024 • edited Loading

jelly commented Aug 26, 2024

This comment was marked as outdated.

This comment was marked as outdated.

jfehlig commented Aug 26, 2024

martinpitt commented Aug 27, 2024

jfehlig commented Aug 27, 2024

Nykseli Aug 30, 2024

Choose a reason for hiding this comment

martinpitt Aug 30, 2024

Choose a reason for hiding this comment

Nykseli Aug 30, 2024

Choose a reason for hiding this comment

JanZerebecki Aug 30, 2024

Choose a reason for hiding this comment

martinpitt Sep 27, 2024

Choose a reason for hiding this comment

martinpitt commented Aug 30, 2024

Nykseli commented Sep 26, 2024

martinpitt commented Sep 27, 2024

jelly left a comment

Choose a reason for hiding this comment

KKoukiou left a comment

Choose a reason for hiding this comment

cockpituous Oct 1, 2024

Choose a reason for hiding this comment

jelly left a comment

Choose a reason for hiding this comment

martinpitt commented Oct 1, 2024

jfehlig commented Oct 1, 2024

martinpitt commented Aug 26, 2024 •

edited

Loading

martinpitt commented Aug 26, 2024 •

edited

Loading