Refs #32968 - Add management of Pulpcore log level

[wbclark]

No description provided.

[ofedoren]

Thanks, @wbclark! Can it be connected/solving this issue as well https://projects.theforeman.org/issues/32954 ?

[wbclark]

Thanks, @wbclark! Can it be connected/solving this issue as well https://projects.theforeman.org/issues/32954 ?

HI @ofedoren , thanks for your response.

It looks like we have partially overlapping issues here:

1. BZ https://bugzilla.redhat.com/show_bug.cgi?id=1977693 / Redmine https://projects.theforeman.org/issues/32968

2. BZ https://bugzilla.redhat.com/show_bug.cgi?id=1976885 / Redmine https://projects.theforeman.org/issues/32954

A few things to consider about how we could handle this (imo, rather interesting) situation:

1. The change here is dependent upon the change in puppet-pulpcore, because the configuration for logging in pulp3 spans multiple lines (see the fixture on this PR for the example) and hammer admin will try to create only the single line (resulting in a non-working configuration) if the complete structure doesn't already exist in the file.

2. I'm not sure if it's possible or ever makes sense to do many-to-one assignment of Redmines to PRs

3. Attaching too many PRs in different components risks overloading QE in verification. This could be mitigated if we provide detailed verification steps.

4. These original bug reports are arguably partially but not completely overlapping. One concerns only service definitions in hammer admin (and somewhat includes both removing pulp2 and adding pulp3), the other is more broad about configuring debug logging for Pulp3.

5. Backporting the addition of pulp3 definition in hammer admin may be desirable, separate from removal of of pulp2 definition. For example, to troubleshoot pulp2 to pulp3 migration issues when both are running simultaneously.

So therefore my proposal is:

1. Let's keep PRs related to configuring logging for pulp3 assigned to a single BZ, that may possibly be safely backported if desired to a version that contains both pulp2 and pulp3.

2. Let's open an additional PR for the removal of pulp2 definitions and attach that one to https://projects.theforeman.org/issues/32954

[wbclark]

Opened #13

[ehelms]

How does the code make sure that this line ends up in the right spot in the file without a declared anchor or descriptor of where it should exist?

[wbclark]

I tested this on a live instance with the real configuration generated by the installer (hacked a bit to get latest changes in pulpcore module, given that katello nightlies were broken until this morning)

I changed it back and forth multiple times between debug mode and production, validating by restarting services and observing appropriate verbosity in logs, as well as manually inspecting the config file each time

so I can state confidently that as the config file is currently structured, it works as expected. I am sure some corner case config file could be constructed to break it (given my intention to put https://github.com/theforeman/puppet-pulpcore/pull/210/files into the settings.py header I think I am OK with that though).

In particular, I copied the basic structure of this feature from the pulp2 equivalent, where in testing I noticed that it actually changes a commented line describing log levels into the actual configuration (and also makes the change a few lines below where the setting already existed). This is admittedly janky but otherwise has no production impact... a commented line is destroyed and some helpful text is lost, replaced with a doubled config line. Once that change from the initial config file is made on the first run, both versions of the line continue to exist and both get updated on each subsequent log level change via hammer admin.

[wbclark]

One alternative that I considered is that the logging configuration could be a single line in settings.py :

LOGGING = {"dynaconf_merge": True, "loggers": {'': {'handlers': ['console'], 'level': 'DEBUG'}}}

I would argue that it's OK to sacrifice ease of human readability in settings.py for this purpose. The only thing they should read in that file anyway is the warning that says to immediately close your editor without saving changes and manage the config through the proper tooling only.

I think there are other methods present here in hammer cli foreman admin to ensure the operation is more robust towards different things you may find in a hand edited config file, but I didn't explore it further as I was able to get it working reliably with the config file format we planned to ship.

[wbclark]

P.S. I did author a "fix" for the destroyed comment / duplicate log level config issue in pulp2: #13

:^)

[lzap]

Thank you, let's get this in.