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

Prefer resolvectl over systemd-resolve #2385

Closed
wants to merge 1 commit into from

Conversation

mbiebl
Copy link
Contributor

@mbiebl mbiebl commented Jan 26, 2021

The latter is a deprecated compat symlink.

See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=979264

@mbiebl
Copy link
Contributor Author

mbiebl commented Jan 26, 2021

From the downstream bug report at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=979264

Hi,

systemd-resolve has been replaced by the resolvectl tool in systemd v239
(i.e. is available since buster). This is from the systemd release
notes:

    * The systemd-resolve tool has been renamed to resolvectl (it also
      remains available under the old name, for compatibility), and its
      interface is now verb-based, similar in style to the other <xyz>ctl
      tools, such as systemctl or loginctl.

systemd-resolve is nowadays merely a symlink pointing at resolvectl and
we'd like to get rid of this compat symlink at some point.

Your package uses the old name like this:

$ grep -E "systemd-resolve\b" -R
sos/plugins/systemd.py:            "systemd-resolve --status",
sos/plugins/systemd.py:            "systemd-resolve --statistics",

Attached is a patch which uses resolvectl instead.
Please review and consider applying it in your next upload.
It would be great if you can make this upload before the bullseye
release, so we can safely drop the symlink in bullseye+1.

@slashdd
Copy link

slashdd commented Jan 26, 2021

c2e84cab3a resolvectl: rename systemd-resolve to resolvectl

$ git describe --contains c2e84cab3a
v239~386

resolvectl has been first introduced in v239 and onwards in systemd.

Wearing my ubuntu hat. Ubuntu has systemd version lt and gt than 239.

 systemd | 229-4ubuntu21.27 | xenial-security | source, amd64, arm64, armhf, i386, powerpc, ppc64el, s390x
 systemd | 229-4ubuntu21.29 | xenial-updates  | source, amd64, arm64, armhf, i386, powerpc, ppc64el, s390x
 systemd | 237-3ubuntu10    | bionic          | source, amd64, arm64, armhf, i386, ppc64el, s390x
 systemd | 237-3ubuntu10.38 | bionic-security | source, amd64, arm64, armhf, i386, ppc64el, s390x
 systemd | 237-3ubuntu10.44 | bionic-updates  | source, amd64, arm64, armhf, i386, ppc64el, s390x
 systemd | 245.4-4ubuntu3   | focal           | source, amd64, arm64, armhf, i386, ppc64el, riscv64, s390x
 systemd | 245.4-4ubuntu3.4 | focal-updates   | source, amd64, arm64, armhf, i386, ppc64el, riscv64, s390x
 systemd | 246.6-1ubuntu1   | groovy          | source, amd64, arm64, armhf, i386, ppc64el, riscv64, s390x
 systemd | 246.6-1ubuntu1.1 | groovy-updates  | source, amd64, arm64, armhf, i386, ppc64el, riscv64, s390x
 systemd | 247.1-4ubuntu1   | hirsute         | source, amd64, arm64, armhf, i386, ppc64el, riscv64, s390x

While Debian look to be gt v239:

systemd    | 241-5~bpo9+1    | stretch-backports       | source, amd64, arm64, armel, armhf, i386, mips, mips64el, mipsel, ppc64el, s390x
systemd    | 241-5~bpo9+1    | stretch-backports-debug | source
systemd    | 241-7~deb10u5   | stable                  | source, amd64, arm64, armel, armhf, i386, mips, mips64el, mipsel, ppc64el, s390x
systemd    | 241-7~deb10u5   | stable-debug            | source
systemd    | 247.2-1~bpo10+1 | buster-backports        | source, mips64el, mipsel
systemd    | 247.2-1~bpo10+1 | buster-backports-debug  | source
systemd    | 247.2-5~bpo10+1 | buildd-buster-backports | source, mipsel
systemd    | 247.2-5~bpo10+1 | buster-backports        | source, amd64, arm64, armel, armhf, i386, mips, mipsel, ppc64el, s390x
systemd    | 247.2-5~bpo10+1 | buster-backports-debug  | source
systemd    | 247.2-5         | testing                 | source, amd64, arm64, armel, armhf, i386, mips64el, mipsel, ppc64el, s390x
systemd    | 247.2-5         | unstable                | source, amd64, arm64, armel, armhf, i386, mips64el, mipsel, ppc64el, s390x
systemd    | 247.2-5         | unstable-debug          | source

Since sosreport is fault tolerant and move on if the command doesn't work nor exist.
Could you implement a way that the systemd.py is backward compatible with both approaches ?

@mbiebl
Copy link
Contributor Author

mbiebl commented Jan 26, 2021

Are you planning on updating sosreport for those old Ubuntu releases?
https://packages.ubuntu.com/search?keywords=sosreport&searchon=names&suite=all&section=all doesn't look like it.

@slashdd
Copy link

slashdd commented Jan 26, 2021

  • Xenial/16.04LTS
    Will EOL shortly, so not much of a problem here.

  • Bionic/18.04LTS (Long Term Support) will roughly be supported for ~10 years.
    I checked and 'resolvectl' hasn't had been backported in Bionic's systemd.
    It is unlikely that systemd pkg in Bionic will bump version short-term.
    I will definitely update 'sosreport' in Bionic short term.

@slashdd
Copy link

slashdd commented Jan 26, 2021

I can carry a patch if needed for Bionic, but I'm afraid other distros might have the same constraint.
I'll be curious to hear what RHEL/CENTOS folks have to say. If we only do an exception for Bionic, forget about it, and I'll carry a UBUNTU SAUCE patch in Bionic when I'll bump the version.

@mbiebl
Copy link
Contributor Author

mbiebl commented Jan 26, 2021

what about something like this?


diff --git a/sos/report/plugins/systemd.py b/sos/report/plugins/systemd.py
index 615b6aa5..01ca20f4 100644
--- a/sos/report/plugins/systemd.py
+++ b/sos/report/plugins/systemd.py
@@ -9,6 +9,7 @@
 # See the LICENSE file in the source distribution for further information.
 
 from sos.report.plugins import Plugin, IndependentPlugin, SoSPredicate
+import shutil
 
 
 class Systemd(Plugin, IndependentPlugin):
@@ -47,11 +48,12 @@ class Systemd(Plugin, IndependentPlugin):
             "timedatectl"
         ])
 
+        RESOLVECTL = shutil.which('resolvectl') or shutil.which('systemd-resolve')
         # resolvectl command starts systemd-resolved service if that
         # is not running, so gate the commands by this predicate
         self.add_cmd_output([
-            "resolvectl status",
-            "resolvectl statistics",
+            RESOLVECTL + " status",
+            RESOLVECTL + " statistics",
         ], pred=SoSPredicate(self, services=["systemd-resolved"]))
 

@TurboTurtle
Copy link
Member

TurboTurtle commented Jan 26, 2021

I'll be curious to hear what RHEL/CENTOS folks have to say. If we only do an exception for Bionic, forget about it, and I'll carry a UBUNTU SAUCE patch in Bionic when I'll bump the version.

Same boat, need to maintain the deprecated symlink.

what about something like this?

    RESOLVECTL = shutil.which('resolvectl') or shutil.which('systemd-resolve')

This, but not using shutil in the plugin directly. Instead, something like this using the helpers within Plugin:

resolvectl = 'resolvectl' if self.is_executable('resolvectl') else 'systemd-resolve'

[...]

             "%s status" % resolvectl,
             "%s statistics" % resolvectl,
         ], pred=SoSPredicate(self, services=["systemd-resolved"]))

On that note, the SoSPredicate here will block collection if the systemd-resolved service is not running. Is the service name changing along with the binary, or is it just the binary?

@mbiebl
Copy link
Contributor Author

mbiebl commented Jan 26, 2021

The service name was not changed, only the /usr/bin/systemd-resolve binary name.

@mbiebl
Copy link
Contributor Author

mbiebl commented Jan 26, 2021

Updated as per @TurboTurtle 's suggestions.

@TurboTurtle
Copy link
Member

Oh, my fault there. For some reason I always forget that is_executable is in utilities and not Plugin.

@mbiebl mbiebl force-pushed the resolvectl branch 2 times, most recently from a557940 to a8caa40 Compare January 26, 2021 17:03
The latter is a deprecated compat symlink.

See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=979264

Signed-off-by: Michael Biebl <biebl@debian.org>
@slashdd
Copy link

slashdd commented Jan 26, 2021

The PR mentioned This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository. Preventing me to approve it, but I'm +1 with the most recent commit 110757d

@mbiebl
Copy link
Contributor Author

mbiebl commented Jan 26, 2021

I'm sorry it took me a few tries. But as said, my Python is rusty and I'm not really familiar with sosreport. That said, thanks for the help so far.

@mbiebl mbiebl changed the title Use resolvectl instead of systemd-resolve Prefer resolvectl over systemd-resolve Jan 26, 2021
Copy link

@slashdd slashdd left a comment

Choose a reason for hiding this comment

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

lgtm. Tested and working as expected with systemd lt & gt v239 on Ubuntu.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants