Re: [PATCH v10 0/3] maintenance: add support for systemd timers on Linux

From: "Lénaïc Huard" <lenaic@lhuard.fr>
To: "Derrick Stolee" <stolee@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Derrick Stolee" <dstolee@microsoft.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"Felipe Contreras" <felipe.contreras@gmail.com>,
	"Phillip Wood" <phillip.wood123@gmail.com>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Bagas Sanjaya" <bagasdotme@gmail.com>,
	"brian m . carlson" <sandals@crustytoothpaste.net>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Jeff King" <peff@peff.net>,
	"Ramsay Jones" <ramsay@ramsayjones.plus.com>
Subject: Re: [PATCH v10 0/3] maintenance: add support for systemd timers on Linux
Date: Mon, 27 Sep 2021 23:44:04 +0200	[thread overview]
Message-ID: <7292569.mAQTRl3RYs@coruscant.lhuard.fr> (raw)
In-Reply-To: <874ka618n4.fsf@evledraar.gmail.com>

Le lundi 27 septembre 2021, 14:50:28 CEST Ævar Arnfjörð Bjarmason a écrit :
> On Thu, Sep 09 2021, Derrick Stolee wrote:
> > On 9/9/2021 1:52 AM, Lénaïc Huard wrote:
> >> Le mercredi 8 septembre 2021, 13:44:26 CEST Derrick Stolee a écrit :
> >>> On 9/7/2021 12:48 PM, Derrick Stolee wrote:
> >>>> On 9/4/2021 4:54 PM, Lénaïc Huard wrote:
> >>>>> Hello,
> >>>>> 
> >>>>> Please find hereafter my updated patchset to add support for systemd
> >>>>> timers on Linux for the `git maintenance start` command.
> >>>>> 
> >>>>> The only changes compared to the previous version are fixes for the
> >>>>> two typos in a comment that Ramsay Jones pointed out [1]
> >>>>> 
> >>>>> [1]
> >>>>> https://lore.kernel.org/git/51246c10-fe0b-b8e5-cdc3-54bdc6c8054e@ramsa
> >>>>> yj
> >>>>> ones.plus.com/>
> >>>> 
> >>>> The changes in the most recent two versions look good to me.
> >>> 
> >>> I recently tested the 'seen' branch for an unrelated reason, but found
> >>> that the t7900-maintenance.sh test failed for me. It was during test 34,
> >>> 
> >>> 'start and stop Linux/systemd maintenance' with the following issue:
> >>>   + systemd-analyze verify systemd/user/git-maintenance@.service
> >>>   Failed to create
> >>>   /user.slice/user-1000.slice/session-44.scope/init.scope
> >>> 
> >>> control group: Permission denied Failed to initialize manager:
> >>> Permission
> >>> denied
> >>> 
> >>> Now, this test has the prereq SYSTEMD_ANALYZE, but for some reason this
> >>> later command fails for permission issues. I'm running Ubuntu, if that
> >>> helps.
> >> 
> >> Thank you for the feedback.
> >> 
> >> Could you please share which version of Ubuntu and which version of
> >> systemd
> >> you are using ?
> >> 
> >> I’ve just tried to start an Ubuntu Impish 21.10 which uses systemd
> >> 248.3-1ubuntu3 and to test the `seen` git branch.
> >> 
> >> All tests of `t/t7900-maintenance.sh` passed including the one which is
> >> failing for you.
> >> 
> >> As `systemd-analyse verify` should only check a unit file validity [1], I
> >> wouldn’t expect it to fail on a cgroup manipulation.
> >> 
> >> [1]
> >> https://www.freedesktop.org/software/systemd/man/systemd-analyze.html#sy
> >> stemd-analyze%20verify%20FILE...
> >> 
> >> I tried to run
> >> systemd-analyze verify /etc/systemd/system/sshd.service
> >> and it didn’t produce the error you mentioned but if I `strace` it, I can
> >> find:
> >> 
> >> mkdir("/sys/fs/cgroup/unified/user.slice/user-1000.slice/session-3.scope/
> >> init.scope", 0755) = -1 EACCES (Permission denied)
> >> 
> >> This makes me think your version of systemd is wrongly considering this
> >> cgroup directory failure as fatal.
> >> I’d like to know more precisely which versions are affected.
> >> 
> >  I am on Ubuntu 18.04.
> > 
> > $ systemd --version
> > systemd 237
> > +PAM +AUDIT +SELINUX +IMA +APPARMOR +SMACK +SYSVINIT +UTMP
> > +LIBCRYPTSETUP +GCRYPT +GNUTLS +ACL +XZ +LZ4 +SECCOMP +BLKID +ELFUTILS
> > +KMOD -IDN2 +IDN -PCRE2 default-hierarchy=hybrid
> > 
> > I tried upgrading with apt-get, but that did not get me a new
> > version.
> 
> It seems this discussion has gone stale, but this is still broken on
> some systems. This is gcc135 on the GCC Farm, which passes the prereq
> this commit adds:
> 
>     $ systemd-analyze verify systemd/user/git-maintenance@.service
>     Failed to open /dev/tty0: Permission denied
>     Failed to load systemd/user/git-maintenance@.service: Invalid argument
> 
> I don't know the systemd specifics involved, but this seems like a
> rather straightforward problem of assuming permissions that aren't
> universal. I.e. let's try to do that in the prereq instead?
> 
> OS details, if they matter:
> 
>     [avar@gcc135 t]$ systemd-analyze --version
>     systemd 219
>     +PAM +AUDIT +SELINUX +IMA -APPARMOR +SMACK +SYSVINIT +UTMP
> +LIBCRYPTSETUP +GCRYPT +GNUTLS +ACL +XZ +LZ4 -SECCOMP +BLKID +ELFUTILS
> +KMOD +IDN [avar@gcc135 t]$ cat /etc/centos-release
>     CentOS Linux release 7.9.2009 (AltArch)

Hello,

I’ve just submitted a new patch to change the way the prerequisite is 
implemented:

https://lore.kernel.org/git/20210927213016.21714-2-lenaic@lhuard.fr/

I’m not sure what was going wrong with old versions of systemd but with this 
new patch, the permission errors raised by `systemd-analyze` should now make 
the prerequisite fail instead of making the test fail.