[PATCH 0/3] RFC: stop using netcf for the interface driver backend
This is a followup to my message in December suggesting that we deprecate use of the netcf package: https://www.redhat.com/archives/libvir-list/2020-December/msg00183.html (or Message-Id: <4889202b-734c-4d0f-472c-d86894319878@redhat.com> for those of you who keep a local archive) Each of these patches takes a baby step in the direction of removing netcf from libvirt: 1) makes it possible to explicitly disable netcf in a build without uninstalling netcf-devel. 2) makes netcf=disabled the default for all normal builds 3) switches all RHEL/Fedora/CentOS rpm builds to disable netcf, and no longer require that netcf and netcf-devel be installed. I purposefully didn't include a patch to completely remove all traces of netcf; we can leave it disabled for awhile before taking that step. Much of the basic functionality provided by the netcf backend to the interface driver is also provided by the udev backend which is used when netcf isn't available. Some differences: 1) the udev backend can't make any modifications to host interface configuration. It is only useful for reading a list of host network devices, and querying some basic status of those interfaces (*not* the contents of configuration files, but how the interface is currently setup) 2) With the netcf backend, "virsh iface-list --all" shows a list of those interfaces that have an ifcfg-blah file in /etc/sysconfig/network-scripts. With the udev backend, the same command shows a list of all network devices currently on the host, as provided by the udev function udev_enumerate_add_match_subsystem(enumerate, "net") (with some exceptions - see udeevGetDevices()). This means that, for example, the bridge devices created by libvirt for virtual networks (virbrX) will show in the list when using udev. 3) The udev backend doesn't fill in IP address info about the devices in the output of "virsh iface-dumpxml" (netcf does). 4) The udev driver *does* go to the trouble of filling in the MTU of every interface, even when it is the default value of 1500. In the end, all the information it reports is correct (well, item (2) is debatable - it depends on what you would use the info for), but it is different, which is why I'm sending this as an RFC - I'd appreciate if people can build and try running something like this script with existing libvirt as well as with the patches applied: http://people.redhat.com/lstump/ncfresults and compare the before/after. The real question is whether or not this difference really "makes a difference" to anyone. My guess/hope is that the answer to this is "no". As far as I know, there aren't any management applications that use the virInterface API (even virt-manager has stopped) so it's probably only humans using virsh that encounter it. (If it's considered important, we could 1) filter out the bridge devices used by libvirt virtual networks, and 2) copy the netcf code that fills in IP address info. I don't want to spend time doing that if nobody's going to use it anyway, though.) Laine Stump (3): build: support explicitly disabling netcf build: make netcf=disabled the default rpm: disable netcf for the interface driver on RHEL/Fedora/CentOS libvirt.spec.in | 8 ++------ meson.build | 9 ++++++--- 2 files changed, 8 insertions(+), 9 deletions(-) -- 2.29.2
placing "-Dnetcf=disabled" on the meson commandline was ignored, meaning that even with that option the build would get WITH_NETCF if the netcf-devel package was found - the only way to disable it was to uninstall netcf-devel. This patch adds the small bit of logic to check the netcf meson commandline option (in addition to whether netcf-devel is installed) before defining WITH_NETCF. Signed-off-by: Laine Stump <laine@redhat.com> --- meson.build | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/meson.build b/meson.build index d8a63baac9..bfdffb16d2 100644 --- a/meson.build +++ b/meson.build @@ -1155,8 +1155,10 @@ libm_dep = cc.find_library('m', required : false) netcf_version = '0.1.8' netcf_dep = dependency('netcf', version: '>=' + netcf_version, required: get_option('netcf')) -if netcf_dep.found() - conf.set('WITH_NETCF', 1) +if not get_option('netcf').disabled() + if netcf_dep.found() + conf.set('WITH_NETCF', 1) + endif endif have_gnu_gettext_tools = false @@ -1550,7 +1552,7 @@ elif get_option('driver_hyperv').enabled() error('openwsman is required for the Hyper-V driver') endif -if not get_option('driver_interface').disabled() and conf.has('WITH_LIBVIRTD') and (udev_dep.found() or netcf_dep.found()) +if not get_option('driver_interface').disabled() and conf.has('WITH_LIBVIRTD') and (udev_dep.found() or conf.has('WITH_NETCF')) conf.set('WITH_INTERFACE', 1) elif get_option('driver_interface').enabled() error('Requested the Interface driver without netcf or udev and libvirtd support') @@ -2355,7 +2357,7 @@ libs_summary = { 'libssh': libssh_dep.found(), 'libssh2': libssh2_dep.found(), 'libutil': libutil_dep.found(), - 'netcf': netcf_dep.found(), + 'netcf': conf.has('WITH_NETCF'), 'NLS': have_gnu_gettext_tools, 'numactl': numactl_dep.found(), 'openwsman': openwsman_dep.found(), -- 2.29.2
Even after the previous patch, in order to build without netcf you would need to add "-Dnetcf=disabled" to the meson commandline (or uninstall netcf-devel). This patch makes -Dnetcf=disabled the default. (Without this change, a lot of people would just blindly continue building with netcf enabled.) Signed-off-by: Laine Stump <laine@redhat.com> --- meson.build | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/meson.build b/meson.build index bfdffb16d2..4cddb95bad 100644 --- a/meson.build +++ b/meson.build @@ -1155,10 +1155,11 @@ libm_dep = cc.find_library('m', required : false) netcf_version = '0.1.8' netcf_dep = dependency('netcf', version: '>=' + netcf_version, required: get_option('netcf')) -if not get_option('netcf').disabled() - if netcf_dep.found() - conf.set('WITH_NETCF', 1) +if get_option('netcf').enabled() + if not netcf_dep.found() + error('You must install the netcf development package to build with netcf') endif + conf.set('WITH_NETCF', 1) endif have_gnu_gettext_tools = false -- 2.29.2
On Fri, Jan 22, 2021 at 03:01:56AM -0500, Laine Stump wrote:
Even after the previous patch, in order to build without netcf you would need to add "-Dnetcf=disabled" to the meson commandline (or uninstall netcf-devel). This patch makes -Dnetcf=disabled the default. (Without this change, a lot of people would just blindly continue building with netcf enabled.)
Signed-off-by: Laine Stump <laine@redhat.com> --- meson.build | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
I don't much like this idea. I consider it a bug if I have installed the -devel package for a pre-requisite and it isn't then detected. If a distro no longer wants to support netcf why not just retire the netcf package from that distro version(s), that way users won't have it installed in the first place ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 1/22/21 4:51 AM, Daniel P. Berrangé wrote:
Even after the previous patch, in order to build without netcf you would need to add "-Dnetcf=disabled" to the meson commandline (or uninstall netcf-devel). This patch makes -Dnetcf=disabled the default. (Without this change, a lot of people would just blindly continue building with netcf enabled.)
Signed-off-by: Laine Stump <laine@redhat.com> --- meson.build | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) I don't much like this idea. I consider it a bug if I have installed the -devel package for a pre-requisite and it isn't
On Fri, Jan 22, 2021 at 03:01:56AM -0500, Laine Stump wrote: then detected.
If a distro no longer wants to support netcf why not just retire the netcf package from that distro version(s), that way users won't have it installed in the first place ?
Yeah, that's what patch 3 does. I added this patch in later as a way of preventing developers (and people who routinely build their own packages from upstream without using the "rpm" target) from naively continuing to use a setup that is deprecated (and then wondering why it behaves differently when the build locally). I'm okay leaving out this patch and just recommending that everyone do their own builds, though.
Since libvirt.spec explicitly adds -Dnetcf=enabled to the meson commandline, just setting the default in the meson.build file won't have any effect for rpm builds. This patch changes the meson commandline in the spec file to -Dnetcf=disabled and removes the associated BuildRequires: and Requires: (I debated whether or not to do that conditionally only for (fedora > 33 || rhel > 8) but haven't done that for now (still may if it's considered important; for Fedora probably not important (since we never rebase after release), for RHEL we might want to preserve current behavior for RHEL8 even through a rebase though) Signed-off-by: Laine Stump <laine@redhat.com> --- libvirt.spec.in | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index b5892987cf..ec82839e10 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -359,7 +359,6 @@ BuildRequires: fuse-devel >= 2.8.6 BuildRequires: libssh2-devel >= 1.3.0 %endif -BuildRequires: netcf-devel >= 0.2.2 %if %{with_esx} BuildRequires: libcurl-devel %endif @@ -528,13 +527,10 @@ capabilities. Summary: Interface driver plugin for the libvirtd daemon Requires: libvirt-daemon = %{version}-%{release} Requires: libvirt-libs = %{version}-%{release} -Requires: netcf-libs >= 0.2.2 %description daemon-driver-interface The interface driver plugin for the libvirtd daemon, providing -an implementation of the network interface APIs using the -netcf library - +an implementation of the network interface APIs using udev. %package daemon-driver-secret Summary: Secret driver plugin for the libvirtd daemon @@ -1170,7 +1166,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/%{name}.spec) %{?arg_numad} \ -Dcapng=enabled \ %{?arg_fuse} \ - -Dnetcf=enabled \ + -Dnetcf=disabled \ -Dselinux=enabled \ %{?arg_selinux_mount} \ -Dapparmor=disabled \ -- 2.29.2
On Fri, Jan 22, 2021 at 03:01:57AM -0500, Laine Stump wrote:
Since libvirt.spec explicitly adds -Dnetcf=enabled to the meson commandline, just setting the default in the meson.build file won't have any effect for rpm builds. This patch changes the meson commandline in the spec file to -Dnetcf=disabled and removes the associated BuildRequires: and Requires:
(I debated whether or not to do that conditionally only for (fedora > 33 || rhel > 8) but haven't done that for now (still may if it's considered important; for Fedora probably not important (since we never rebase after release), for RHEL we might want to preserve current behavior for RHEL8 even through a rebase though)
Yeah, our normal policy is that we don't disable stuff that was already present. So changing this should be conditional on newer distros only. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 1/22/21 4:49 AM, Daniel P. Berrangé wrote:
On Fri, Jan 22, 2021 at 03:01:57AM -0500, Laine Stump wrote:
Since libvirt.spec explicitly adds -Dnetcf=enabled to the meson commandline, just setting the default in the meson.build file won't have any effect for rpm builds. This patch changes the meson commandline in the spec file to -Dnetcf=disabled and removes the associated BuildRequires: and Requires:
(I debated whether or not to do that conditionally only for (fedora > 33 || rhel > 8) but haven't done that for now (still may if it's considered important; for Fedora probably not important (since we never rebase after release), for RHEL we might want to preserve current behavior for RHEL8 even through a rebase though) Yeah, our normal policy is that we don't disable stuff that was already present. So changing this should be conditional on newer distros only.
Okay, I'll make it more elaborate (with its own variable ala firewalld_zones) and repost. Speaking of this - I just noticed that Fedora will be branching for F34 prior to the next libvirt release, and I was hoping I could get this into the next Fedora (wish I'd been paying closer attention to the important dates :-( - I really need those entered in the same calendar that reminds me of my doctor appointments and meetings).
participants (2)
-
Daniel P. Berrangé -
Laine Stump