gnu: Add rasdaemon.

  • Done
  • quality assurance status badge
Details
2 participants
  • elaexuotee
  • Leo Famulari
Owner
unassigned
Submitted by
elaexuotee
Severity
normal
E
E
elaexuotee wrote on 20 Apr 2021 06:24
(address . guix-patches@gnu.org)
31MWDEN7Q9XOV.2001N5J6G4U9K@wilsonb.com
This is my first patch to gnu/services (and by inclusion doc/guix.texi), so
please scrutinize with abandon!

I did all the standard sanity checks for the package definition (guix lint,
etc/indent-code.el, guix build --check); however, I wasn't sure about the
service, so the sanity checks were limited to testing in a vm.

Cheers!
Attachment: 0001-gnu-Add-rasdaemon.patch
L
L
Leo Famulari wrote on 20 Apr 2021 07:04
(name . elaexuotee--- via Guix-patches via)(address . guix-patches@gnu.org)(address . 47905@debbugs.gnu.org)
YH5g8Em10bCEaUEI@jasmine.lan
On Tue, Apr 20, 2021 at 01:24:37PM +0900, elaexuotee--- via Guix-patches via wrote:
Toggle quote (7 lines)
> This is my first patch to gnu/services (and by inclusion doc/guix.texi), so
> please scrutinize with abandon!
>
> I did all the standard sanity checks for the package definition (guix lint,
> etc/indent-code.el, guix build --check); however, I wasn't sure about the
> service, so the sanity checks were limited to testing in a vm.

Thanks for letting us know how you tested it. That's really valuable for
reviewers.
L
L
Leo Famulari wrote on 20 Apr 2021 07:07
(address . elaexuotee@wilsonb.com)(address . 47905@debbugs.gnu.org)
YH5hk4JZhqkr9GPi@jasmine.lan
On Tue, Apr 20, 2021 at 01:24:37PM +0900, elaexuotee--- via Guix-patches via wrote:
Toggle quote (2 lines)
> +(define-public rasdaemon

Overall LGTM.

Toggle quote (2 lines)
> + (license license:gpl2+)))

I checked the license headers of the ras-*.c files.

Some of them are GPL2+, and some are GPL2 only. I think that we should
mark it GPL2 only, based on that.
L
L
Leo Famulari wrote on 20 Apr 2021 07:30
(address . elaexuotee@wilsonb.com)(address . 47905@debbugs.gnu.org)
YH5nC25CoKEIvBhK@jasmine.lan
On Tue, Apr 20, 2021 at 01:24:37PM +0900, elaexuotee--- via Guix-patches via wrote:
Toggle quote (2 lines)
> * doc/guix.texi (Linux Services): Document it.

It could be added to Monitoring Services instead. Although, RAS is
Linux-only, so I leave that decision up to you.

Toggle quote (7 lines)
> +@cindex rasdaemon
> +@cindex Platform Reliability, Availability and Serviceability daemon
> +@subsubheading Rasdaemon Service
> +
> +The Rasdaemon service provides a monitor for Platform Reliability,
> +Availability, and Serviceability (RAS) events in the Linux kernel.

It would be nice to link to some upstream documentation of RAS, as we do
for the documentation of the Zram Device Service.

I think it could also be improved with addition of one or two sentences
about how to make use of the service. As logging appears to be disabled
by default, how are users expected to learn of the events monitored by
rasdaemon?

After writing that, I looked at rasdaemon-shepherd-service and see that
it keeps a log file. Is that the same data as the optional SQLite
database, but unstructured?

Overall, the docs should clarify this :)
L
L
Leo Famulari wrote on 20 Apr 2021 07:33
(address . elaexuotee@wilsonb.com)(address . 47905@debbugs.gnu.org)
YH5noW4vKgEQkZLU@jasmine.lan
On Tue, Apr 20, 2021 at 01:24:37PM +0900, elaexuotee--- via Guix-patches via wrote:
Toggle quote (3 lines)
> +;;;
> +;;; Reliability, Availability, and Servicability (RAS) daemon

Typo -------------------------------> Serviceability

Otherwise, this LGTM. I'm no expert on Guix services but this one seems
simple and straightforward.

You mentioned that you tested in a VM. What should I look for in the VM
to verify that the service is working?
E
E
elaexuotee wrote on 20 Apr 2021 07:54
(name . Leo Famulari)(address . leo@famulari.name)(address . 47905@debbugs.gnu.org)
3R38A8AKFTHYA.2KFNX2D2XIN4U@wilsonb.com
Leo Famulari <leo@famulari.name> wrote:
Toggle quote (12 lines)
> On Tue, Apr 20, 2021 at 01:24:37PM +0900, elaexuotee--- via Guix-patches via wrote:
> > +(define-public rasdaemon
>
> Overall LGTM.
>
> > + (license license:gpl2+)))
>
> I checked the license headers of the ras-*.c files.
>
> Some of them are GPL2+, and some are GPL2 only. I think that we should
> mark it GPL2 only, based on that.

Oh! Thanks. Nice catch. This is not the first time I have missed license
details, so I looked for some method to find all licenses in a project:

$ licensecheck --recursive --machine . | awk -F$'\t' '{print $2}' | sort -u
FSF All Permissive License
GNU Lesser General Public License (v2.1)
GPL (v2 or later)
GPL (v2 or later) (with incorrect FSF address)
GPL (v2)
UNKNOWN

The `FSF All Permissive License' just comes from the INSTALL file. In general,
I'm aware that we can include multiple licenses, so the above output would look
like:

(license `(,license:fsf-free ,license:lgpl2.1 ,license:gpl2 ,license:gpl2+))

However, legally-speaking, was is the correct approach here?
E
E
elaexuotee wrote on 20 Apr 2021 08:18
(name . Leo Famulari)(address . leo@famulari.name)(address . 47905@debbugs.gnu.org)
34JN11C25VTZ1.320NQPANZ7JNY@wilsonb.com
Leo Famulari <leo@famulari.name> wrote:
Toggle quote (6 lines)
> On Tue, Apr 20, 2021 at 01:24:37PM +0900, elaexuotee--- via Guix-patches via wrote:
> > * doc/guix.texi (Linux Services): Document it.
>
> It could be added to Monitoring Services instead. Although, RAS is
> Linux-only, so I leave that decision up to you.

Hrm. Monitoring Services does make sense. Kind of a tough call. Will
give it more thought but probably keep in Linux since anyone wanting this
service probably knows exactly what they are looking for.

Toggle quote (14 lines)
> It would be nice to link to some upstream documentation of RAS, as we do
> for the documentation of the Zram Device Service.
>
> I think it could also be improved with addition of one or two sentences
> about how to make use of the service. As logging appears to be disabled
> by default, how are users expected to learn of the events monitored by
> rasdaemon?
>
> After writing that, I looked at rasdaemon-shepherd-service and see that
> it keeps a log file. Is that the same data as the optional SQLite
> database, but unstructured?
>
> Overall, the docs should clarify this :)

Agreed. My documentation was lazy. Will improve. Thanks for the specific
pointers.

About the sqlite db, I honestly don't know much about how it compares to the
logs. From a cursory glance at the code it looks basically like what you said,
a structured rendering of the logs. Will make some kind of note about this.

Speaking of, the log output looks like "rasdaemon: <message>", so it's probably
just as good to put this in the syslog rather than a dedicated file. How can
one do that?
E
E
elaexuotee wrote on 20 Apr 2021 08:23
(name . Leo Famulari)(address . leo@famulari.name)(address . 47905@debbugs.gnu.org)
3Q5HNQGY1NQ09.3MHSB1ZWFZQNX@wilsonb.com
Leo Famulari <leo@famulari.name> wrote:
Toggle quote (12 lines)
> On Tue, Apr 20, 2021 at 01:24:37PM +0900, elaexuotee--- via Guix-patches via wrote:
> > +;;;
> > +;;; Reliability, Availability, and Servicability (RAS) daemon
>
> Typo -------------------------------> Serviceability
>
> Otherwise, this LGTM. I'm no expert on Guix services but this one seems
> simple and straightforward.
>
> You mentioned that you tested in a VM. What should I look for in the VM
> to verify that the service is working?

Ah, good catch.

Just check that the daemon is running and log/database files exist. For a
non-broken system, the sqlite database will just contain a few empty tables and
the logs will just notify you that the various monitors are active:

$ herd status rasdaemon
$ pgrep -a rasdaemon
$ cat /var/log/rasdaemon
$ sqlite3 /var/lib/rasdaemon/ras-mc_event.db

I honestly don't know a lot about RAS, yet, so that's the extent of what I can
easily probe.
E
E
elaexuotee wrote on 20 Apr 2021 09:10
(name . Leo Famulari)(address . leo@famulari.name)(address . 47905@debbugs.gnu.org)
34GOUL165TOWY.3FR9YU3I80NFA@wilsonb.com
This patch updates the license field to contain lgpl2.1, gpl2, and gpl2+.
I also added a lot to the docs. Upstream docs are pretty sparse, so I mostly
just pilfered from the Linux kernel admin-guide explanation of RAS. At the end
of the explanation, I include a URL to that guide directly.
Attachment: 0001-gnu-Add-rasdaemon.patch
L
L
Leo Famulari wrote on 20 Apr 2021 18:08
(address . elaexuotee@wilsonb.com)(address . 47905@debbugs.gnu.org)
YH78kCXuaZNRZBUI@jasmine.lan
On Tue, Apr 20, 2021 at 02:54:49PM +0900, elaexuotee@wilsonb.com wrote:
Toggle quote (8 lines)
> The `FSF All Permissive License' just comes from the INSTALL file. In general,
> I'm aware that we can include multiple licenses, so the above output would look
> like:
>
> (license `(,license:fsf-free ,license:lgpl2.1 ,license:gpl2 ,license:gpl2+))
>
> However, legally-speaking, was is the correct approach here?

Overall, the program is distributed under the GPL version 2, based on
COPYING (whether or not "later versions" are allowed depends on license
headers of individual files).

Some components may have other licenses, but I'd say the whole thing —
the "program" as we use it — is GPL 2.

In general, we redistribute the program under a single license, so that
is what the license field should say. Maybe if there is some really
valuable component that can be used under a different license, we can
add a code comment about it. But, I don't think it's helpful to list
the licenses of files such as INSTALL, nor is it unusual that they have
a different license than the whole.

I would refer to this page for more advice about the GPL:


I'm not a legal expert, and my understanding is that none of this stuff
is really "settled" or "well understood" legally — that would require
extensive and repeated litigation, at least in the USA, which has not
occurred.
L
L
Leo Famulari wrote on 20 Apr 2021 18:19
(address . elaexuotee@wilsonb.com)(address . 47905@debbugs.gnu.org)
YH7/Hzz/LIowOghn@jasmine.lan
On Tue, Apr 20, 2021 at 03:18:19PM +0900, elaexuotee@wilsonb.com wrote:
Toggle quote (4 lines)
> Speaking of, the log output looks like "rasdaemon: <message>", so it's probably
> just as good to put this in the syslog rather than a dedicated file. How can
> one do that?

I poked around in the existing services, and it seems that the service
daemons themselves have the ability to log to syslog, based upon their
own configuration.

For example, the OpenSSH service 'requires' syslogd, but only in terms
of making sure that syslogd is running before sshd starts. I don't see
any extension of the syslog service. (I could be wrong — services are my
weak point)

So, I guess that rasdaemon needs to learn how to find and use syslog, if
it hasn't yet.
L
L
Leo Famulari wrote on 20 Apr 2021 18:20
(address . elaexuotee@wilsonb.com)(address . 47905@debbugs.gnu.org)
YH7/NCPvcyom1twT@jasmine.lan
On Tue, Apr 20, 2021 at 03:23:14PM +0900, elaexuotee@wilsonb.com wrote:
Toggle quote (12 lines)
> Just check that the daemon is running and log/database files exist. For a
> non-broken system, the sqlite database will just contain a few empty tables and
> the logs will just notify you that the various monitors are active:
>
> $ herd status rasdaemon
> $ pgrep -a rasdaemon
> $ cat /var/log/rasdaemon
> $ sqlite3 /var/lib/rasdaemon/ras-mc_event.db
>
> I honestly don't know a lot about RAS, yet, so that's the extent of what I can
> easily probe.

Thanks, I'll try it out.
L
L
Leo Famulari wrote on 20 Apr 2021 18:23
(address . elaexuotee@wilsonb.com)(address . 47905@debbugs.gnu.org)
YH7/9AhRNLr78SBI@jasmine.lan
On Tue, Apr 20, 2021 at 04:10:54PM +0900, elaexuotee@wilsonb.com wrote:
Toggle quote (2 lines)
> This patch updates the license field to contain lgpl2.1, gpl2, and gpl2+.

As mentioned in a previous reply, it should contain only gpl2 [0], unless
upstream has explicitly given us a license to distribute rasdaemon under
the other licenses.

E
E
elaexuotee wrote on 22 Apr 2021 12:01
(name . Leo Famulari)(address . leo@famulari.name)(address . 47905@debbugs.gnu.org)
377TR65TD6KJY.2JY05ANZ7375P@wilsonb.com
Leo Famulari <leo@famulari.name> wrote:
Toggle quote (10 lines)
> On Tue, Apr 20, 2021 at 04:10:54PM +0900, elaexuotee@wilsonb.com wrote:
> > This patch updates the license field to contain lgpl2.1, gpl2, and gpl2+.
>
> As mentioned in a previous reply, it should contain only gpl2 [0], unless
> upstream has explicitly given us a license to distribute rasdaemon under
> the other licenses.
>
> [0] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=47905#35


Fixed! Thanks for holding my hand on this. That above link cleared up a lot
of lingering questions I had.

Anyway, in addition to the license fix, I also noticed that the daemon
automatically logs to syslogd, so I stripped out /var/log/rasdaemon.log,
including the references in the documentation.

Let me know what you think.

Cheers!
Attachment: 0001-gnu-Add-rasdaemon.patch
L
L
Leo Famulari wrote on 24 Apr 2021 18:33
(address . elaexuotee@wilsonb.com)(address . 47905-done@debbugs.gnu.org)
YIRIdPmHCbwcH669@jasmine.lan
On Thu, Apr 22, 2021 at 07:01:20PM +0900, elaexuotee@wilsonb.com wrote:
Toggle quote (3 lines)
> Fixed! Thanks for holding my hand on this. That above link cleared up a lot
> of lingering questions I had.

You're welcome! My goal is help contributors keep learning until they
are sending patches that pass code review on the first try, and they
feel confident to start reviewing patches, too. Based on this submission,
I think you are more than knowledgeable enough to review some patches :)

Toggle quote (14 lines)
> Let me know what you think.

> From d31cb88e5c34cc0b650ff181ebb6a0d85d045f9d Mon Sep 17 00:00:00 2001
> From: "B. Wilson" <elaexuotee@wilsonb.com>
> Date: Tue, 20 Apr 2021 11:49:26 +0900
> Subject: [PATCH] gnu: Add rasdaemon.
> To: guix-patches@gnu.org
>
> * gnu/packages/linux.scm (rasdaemon): New variable.
> * gnu/services/linux.scm (rasdaemon-configuration)
> (rasdaemon-configuration?, rasdaemon-configuration-record?)
> (rasdaemon-service-type): New variables.
> * doc/guix.texi (Linux Services): Document it.

It's good! I pushed as 2c93df3d11bf8ceeb5c203416a2533cf32275e1a after
splitting it into two commits: one for the package, and one for the
service and its documentation.

By the way, you might ask upstream about fixing the issues in their
build scripts — the ones you fixed in the 'munge-autotools' phase. I
think most distros will need to do similar fixes.
-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEsFFZSPHn08G5gDigJkb6MLrKfwgFAmCESHQACgkQJkb6MLrK
fwgSPRAAooeaCgqZEY/NVaEroGd52Ac4+J5aQoqnYSP+G5scXlpqaWoM+hblQlYS
0Qdmt3aRrVag2Asp21PIuiJjt0U3VIsJMm/zoOFn+f1+SVCFEDxXYo7DJ0oFBogM
rsuXYZJ7M4Wbmz0tXW+k5HZvB063UtMAi+5A+IkoYfYHYOAtdI/wBr0kMsmex5Qf
VY8TP4CIlBNHLd0qQa83au58GlY3gcBAuGgO5A8b8VAAgjdIURYAJRanOdpjoN9Q
xqzArH7+Wqg1T+hfsj6W4zp3dYus5jd7Vh9gghuJkccMzjRJfV58KQTPEnHF3PUZ
fuRm24910uZ4pJttKMJDISekxxEJB0nW2ikd0VQuu8ZIjFyuo8Yb0v+wuYVBD1WE
m9Ou+m0ZSBslMrvLXQXHBDTDGLCW+WteArn7Zv8qQQH/p2r4zcq7NI5MsmnuCTsi
QV7ogqw6MfVYlcGLg29hLbZnHdiwrZm50M5IHZmfitJI4Ps9nUOCDOZPHFyY+TbF
gXq5uVYRM3qHMN+czptmR3F0SVrkQm4H2rZYxM9ZAn00d2G6IpTZFdS6ynXUVZlM
nNJiPp+gRqSSfHZQqD1KuK62awk3rlF1Gw/attMjufUw5Ce5N4BKNgD88SKFskGt
t/BF9NeX+BiEN//tER7ooqVf5DOiOytylWP6qeNoJ/bHbwAVL40=
=zS6I
-----END PGP SIGNATURE-----


Closed
?