Code review comment for ~ahasenack/ubuntu/+source/nfs-utils:disco-fix-rpc.svcgssd-args-1616123

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

> The change itself seems fine, there is no changelog change thou ?!
>
> In addition I have seen that you have tagged the bug for SRUs.
> I've had such cases in the past and just wanted to share some lessons learned.
>
> By default NEED_SVCGSSD= is "no" so it doesn't even start. And even if it is
> "yes" then RPCSVCGSSDOPTS= is empty to it would make no difference. That is
> good so all of it only matters for a small amount of people.
>
> But if somebody played with the RPCSVCGSSDOPTS= option and left it in any
> state that is not "" then this SRU will suddenly change behavior for him.
> I know that is exactly the fix you want to achieve, but just saying that for
> "regression potential" this has to be considered. You know better than I do
> what people usually put in there and if that might be a problem.

Will do it in the sru template.

> One thing that can be done is to parse the file on update and warn about
> changing behavior if it will be triggered (but people unfortuantely ignore
> such messages, so it is barely worth).

Probably not worth it.

> I checked and as you have already mentioned (it is generated) /etc/default
> /nfs-kernel-server is no conffile. So I'm wondering how this will behave on an

The file that is generated is /run/sysconfig/nfs-utils, and the systemd service files source that one (instead of the ones in /etc/default). This is upstream even.

> upgrade - is it only generated "once" on install - if so the fix will only
> help new installs (which is bad) but in return will not break existing setups
> (which is good).
> The install path with the fix seems to be great and fixed, but I haven't
> looked deeper into the "how does this work on upgrades". Fortunately rbasak
> has grabbed the full review - so I'll leave that part for him :-P

People bitten by this bug could have fixed it in a few ways.

a) change the rpc-svcgssd systemd service file to reference RPCSVCGSSDARGS. When they upgrade, the new systemd service file will reference the correct variable (SVCGSSDARGS). No dpkg prompt, since this is not a config file
b) same as (a), but via a systemd override file. I think these will be broken, because no RPCSVCGSSDARGS variable will exist anymore. To cope with this, we could export both variables in debian/nfs-utils_env.sh. I actually had that before, but thought it wasn't necessary
c) people who changed /etc/default/nfs-kernel-server will have found out that it doesn't fix the bug unless they also changed nfs-utils_env.sh

So (a) works, (b) can be coped with if we want to, and (c) are SOL.

« Back to merge proposal