Drop usage of virtinterfaced

[martinpitt]

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 #1777

[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]

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]

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.

[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"
                    });
                }
            }

[martinpitt]

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]

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]

/**

• 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]

@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]

@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]

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

[martinpitt]

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.

[Nykseli]

I see. That's a fair point

[JanZerebecki]

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.

[martinpitt]

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]

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]

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

[martinpitt]

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

[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]

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]

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]

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