pmlogger: Implement zeroconf defaults using an additional env file

[SunilMohanAdapa]

Earlier, a new way provide zeroconf values for pmlogger was implemented in
d19714e and
9859201. This solution worked on systems with
systemd but not others. This caused problem for the piuparts test which
apparently does not start under systemd during the test[1].

To implement the solution in a more consistent way across all systems (with and
without systemd), the earlier solution is reverted with a new solution that
drops an additional defaults file with pcp-zeroconf package.

Tests:

• Install on a machine with non-systemd Debian based distribution. When pmlogger
  is started, the PMLOGGER_INTERNAL is part in the pmlogger process environment
  with value 10.

• Install on a Debian machine with systemd. When pmlogger is started, the
  PMLOGGER_INTERNAL is part in the pmlogger process environment with value 10.

• Update /etc/default/pmlogger with a value for PMLOGGER_INTERNAL. That value is
  used.

• Install without pcp-zeroconf. When pmlogger is started, the PMLOGGER_INTERNAL
  is not part of pmlogger process environment.

• The file /etc/defaults/pmlogger_zeroconf is part of the pcp-zeroconf package.

Links:

1. https://piuparts.debian.org/sid/fail/pcp-zeroconf_5.3.4-1.log

Signed-off-by: Sunil Mohan Adapa sunil@medhas.org

[goodwinos]

Thanks for the updated solution Sunil and for considering portability for non-systemd platforms. The changes in pmlogger_check.sh aren't quite right - we would want the zeroconf environment variables to take precedence over the standard values, right? So this line:

envs=grep -h ^PMLOGGER "$PMLOGGERZEROCONFENVS" "$PMLOGGERENVS" 2>/dev/null
should be

envs=grep -h ^PMLOGGER "$PMLOGGERENVS" "$PMLOGGERZEROCONFENVS" 2>/dev/null`

So later on when $envs is eval'd (when pmlogger is launched), we'll get the zeroconf values if that file has been installed. Also it would help to add a comment in each file explaining the precedence rules, e.g. in the zeroconf file, state that it overrides the standard environment (and in the $PMLOGGERENVS file, state that it will be overridden by the zeroconf file, if present.

If you'd like to make these additional changes and update the PR that would be great, otherwise I'll do so in the morning (my morning in Australia).

Also note: these settings are only applied for the primary logger - do you have any thoughts on that? This has come up for discussion before but we decided to leave it as is because otherwise existing pmlogger "farms" may get an unexpected increase in log volumes when they upgrade.
`

[goodwinos]

Hi Sunil, I went ahead and made the additional changes (mentioned above), and have now merged this into main branch for pcp-5.3.5.

Thanks

[SunilMohanAdapa]

@goodwinos , thank you for the review and comments.

Thanks for the updated solution Sunil and for considering portability for non-systemd platforms. The changes in pmlogger_check.sh aren't quite right - we would want the zeroconf environment variables to take precedence over the standard values, right? So this line:

envs=grep -h ^PMLOGGER "$PMLOGGERZEROCONFENVS" "$PMLOGGERENVS" 2>/dev/null should be

envs=grep -h ^PMLOGGER "$PMLOGGERENVS" "$PMLOGGERZEROCONFENVS" 2>/dev/null`

So later on when $envs is eval'd (when pmlogger is launched), we'll get the zeroconf values if that file has been installed. Also it would help to add a comment in each file explaining the precedence rules, e.g. in the zeroconf file, state that it overrides the standard environment (and in the $PMLOGGERENVS file, state that it will be overridden by the zeroconf file, if present.

In my patch, I assumed that /etc/default/pmlogger is the file that user will want to edit. User is not expected to touch the pmlogger_zeroconf at all. Also, I assumed that the order of priority should be User configuration > Zeroconf value > Unconfigured default. Reasoning:

• Users are currently used to editing the /etc/default/pmlogger . Configuration file that needs to be edited (for settings to take effect) will not change with the next version of pcp containing this patch. Perhaps there is existing documentation that says the file to be edited for configuration changes is /etc/default/pmlogger .
• There will be existing installations where user's may have already changed the file /etc/default/pmlogger to set their preferred logging interval. When they are upgrade to the newer version, I suppose that we should not override the value with the zeroconf file and expect them to additionally modify the newly installed zeroconf file.

It is a bit of confusion for the user to see two files instead one. To address this problem and to make sure that users don't ever edit the zeroconf file, we can add a comment asking them to edit the other file or, better yet, we can consider putting the zeroconf file in /usr/. This way, the user won't ever consider editing the zeroconf file.

If you'd like to make these additional changes and update the PR that would be great, otherwise I'll do so in the morning (my morning in Australia).

I shall now make the changes to add comments reflecting the above viewpoint.

However, if you think zeroconf settings should get the highest priority instead, let me know and I will make the changes accordingly tomorrow. If you wish to avoid the round trip with to US West :), please feel free to make the changes (either by editing my commits or adding new ones).

Also note: these settings are only applied for the primary logger - do you have any thoughts on that? This has come up for discussion before but we decided to leave it as is because otherwise existing pmlogger "farms" may get an unexpected increase in log volumes when they upgrade. `

I haven't dug into pcp enough to make a well considered response. However, I can pass an opinion from a generic view point:

It really depends on the true purpose of zeroconf package and what users expect out of it.

1. If the purpose of zeroconf is to provide a one time configuration at the time of installation and never change it later, then updating the log interval for all the loggers will be an unexpected surprise for the users.
2. On the other hand, if zeroconf is meant to provide a low effort administration tool that does the right thing most of the time (and users currently expect this), then updating the log interval for all the loggers will be right change for the users (obviously, you saw the need for setting the interval to 10 seconds in zeroconf package). I work on the FreedomBox (which includes cockpit and pcp) project the entire job of which is to eliminate administration effort on behalf the users. From this perspective, I would argue that a zeroconf package with this definition (the one that aims to eliminate administration overhead and does the right thing most of the time) is more useful for the majority of the users. If upstream pcp authors' understanding of how things should be configured evolves, go ahead and do it in the zeroconf package. If a security update requires, better configuration go ahead and do it. To reduce surprises to users, however, announce in release notes, always give a way to override and finally, don't make the changes in security/bug fix branch, if any.

[SunilMohanAdapa]

Hi Sunil, I went ahead and made the additional changes (mentioned above), and have now merged this into main branch for pcp-5.3.5.

Aha, thanks. Never mind my comments.

[natoscott]

It is a bit of confusion for the user to see two files instead one.

+1

To address this problem and to make sure that users don't ever edit the zeroconf file, [...] better yet, we can consider putting the zeroconf file in /usr/. This way, the user won't ever consider editing the zeroconf file.

Good suggestion - I agree this is a better solution than having two side-by-side files with conflicting (duplicate) variable names in a sysadmin-editable location. Somewhere below /usr it's really clear that this is not something a user would normally be editing.

[natoscott]

@SunilMohanAdapa @goodwinos I believe these changes have accidentally caused a regression in ansible-pcp which modifies PMLOGGER_INTERVAL in /etc/sysconfig. When pcp-zeroconf is installed this setting is now ignored and pcp-zeroconf setting "wins" (however, ansible-pcp also installs zeroconf). :( Not sure on our best option here right now, but we need to consider this one further.

[SunilMohanAdapa]

@natoscott , @goodwinos I believe that the solution is straight forward. That is to say that the configuration installed by zeroconf (in /usr 638e426) should have lower precedence compared to user set values in /etc/sysconfig/ (more arguments for this in my previous comment #1462 (comment)). This was the intention of my patch (3020052). However, the priority was reversed later (2c17ba0). If we undo this change, it should be sufficient:

• User's only editable configuration file /etc/sysconfig/pmlogger will have highest precedence. The other file in /usr is not user editable and will get lower precedence.
• There will be no regressions over previous behavior. Ansible should work just like before.

[natoscott]

@SunilMohanAdapa sounds promising. However, since we ship /etc/sysconfig/pmlogger (and with a value for PMLOGGER_INTERVAL), I don't understand how you differentiate between this (our shipped default value outside zeroconf) and something the user has set...?

Oh, I see it now :) ... we don't set a value for PMLOGGER_INTERVAL there, its commented out by default. OK - your plan sounds good then!

[natoscott]

@SunilMohanAdapa any chance you have some spare cycles to put together a PR for this change? We'd like to put a bug-fix only pcp-5.3.6 out within the next week and this feels like something we should fix ASAP. I'm working on other issues at the moment, but can circle back to this one if you're out of time currently.

[SunilMohanAdapa]

I shall try to create a PR in a day or two.

[natoscott]

@SunilMohanAdapa awesome, thanks!

[SunilMohanAdapa]

@natoscott @goodwinos #1525 addresses the regression in ansible-pcp by prioritizing user configuration over zeroconf configuration.