PR - Issue 50550 - DS installer debug messages leaking to ipa-server-install

[389-ds-bot]

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50551

• Created at 2019-08-19 12:27:26 by spichugi (@droideck)
• Merged at 2019-09-03 18:05:22

---

Bug Description:
DS installer debug messages are now leaked in the main ipa-server-install output.
This looks as a (very minor) regression, I did not see this text in the past.

Fix Description:
Clean up loging in lib389. Replace 'sepolicy' module with subprocess call
to 'semanage' tool. It is done because 'sepolicy' has verbose output that
appears on 'import'. Instead of developing a tricky workaround, direct
'semange' call was used.

Resolves: #3606

Reviewed by: ?

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-08-19 12:57:00

rebased onto 449840557709df06bddef3c31a935291340c4102

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-08-20 02:32:29

This should stay as info to ALWAYS indicate clearly that no action is taken on dry run.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-08-20 02:33:01

This doesn't look like a logging change ;)

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-08-20 02:33:52

Why are we changing stderr here? That seems incorrect ....

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-08-20 02:34:43

This change concerns me because the whole point of this logging was so that in the case of a failure we had the logs in ipa's install log still. So ... why are we removing this? It seems like "clean logs" matters less than "we have all the info needed to solve a problem".

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-08-20 21:32:58

1 new commit added

• Fix an issue reported by William. NOOP = debug -> info

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-08-20 21:40:43

This should stay as info to ALWAYS indicate clearly that no action is taken on dry run.
Why are we changing stderr here? That seems incorrect ....

Right. I was a bit harsh there... Setting it back.

This doesn't look like a logging change ;)

As I mentioned in the PR/Commit message, it is done because sepolicy has verbose output that appears on import (I've seen it only during ipa-server-install though...). Instead of developing a tricky workaround, direct semange call was used (it is still workaround but it is a straight forward one :) ).

This change concerns me because the whole point of this logging was so that in the case of a failure we had the logs in ipa's install log still. So ... why are we removing this? It seems like "clean logs" matters less than "we have all the info needed to solve a problem".

In case of a failure, we still have logging that is put to a separate file in IPA installation.
Also, I really see no benefit in systemctl enable/disable output on every (even non-verbose) run. The same is true for useless sepolicy import output (we have a separate check for selinux anyway).

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-08-20 21:44:26

2 new commits added

• Fix issues reported by William. NOOP = debug -> info, semanage stderr
• Issue 50550 - DS installer debug messages leaking to ipa-server-install

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-08-21 02:14:02

I'm worried about you removing these - these are important to detect and avoid selinux calls on systems without selinux (ie suse :) ). If there is an issue with the log message, just change it to log.warn instead? But this needs to stay.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-08-21 02:14:11

As does this.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-08-21 02:14:43

After these two comments I think it's okay :) but those blocks shouldn't be removed.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-08-21 10:52:44

I'm worried about you removing these - these are important to detect and avoid selinux calls on systems without selinux (ie suse :) ). If there is an issue with the log message, just change it to log.warn instead? But this needs to stay.

As I said previously, we have a check for selinux already...
It is done by import selinux; if selinux.is_selinux_enabled():

sepolicy is used only for acquiring labeled ports so we can relabel them.
And I can't set the logging to warn because it happens during import sepolicy and shown only during ipa-server-install (not sure why, some wierd logger setting?). I spent an hour trying to suppress the message and it already had become a weird workaround... So I decided to make a straight forward workaround. :)

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-08-21 17:27:30

Let's remove the initial newline from the text string, thanks

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-08-21 19:31:12

1 new commit added

• Remove a new line character

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-08-21 19:31:54

Let's remove the initial newline from the text string, thanks

Fixed

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-08-30 22:28:38

I also see this seem annoying behavior from systemd every time I create an instance

# dscreate from-file /data/dssetup.inf 

Starting installation...
Created symlink /etc/systemd/system/multi-user.target.wants/dirsrv@localhost.service → /usr/lib/systemd/system/dirsrv@.service.
Completed installation for localhost

I originally moved that message to DEVNULL as well but that change was rejected by William I think :-p I personally want to see these messages removed from STDOUT by whatever means possible. :-)

Sorry just a side comment that's partially related to this issue

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-08-31 03:19:44

Gosh that William kid, just causing trouble for everyone, sheesh ;) Who even let that person be on the team anyway ....

There is a difficult balance here - between too much detail when not needed, and not enough info when it is needed. My greatest concern is "when something breaks, can the admin resolve it with the information we provided?". This is why I always err to "too much detail" because in the good case, we wasted some terminal space. If we remove that detail, we now have people who can't solve their issues, have a negative experience and can't supply the needed details to us to fix their issues.

I think that the current setup of the log system in lib389 is all focused around stdout/stderr. What we need is a way to also have it write as debug level to a location and then we can have a lower level in the stdout/stderr levels.

So instead of directing these to devnull, we should direct them to a python buffer via subprocess, then we should write these buffers to the log.debug. From there we can have the log code setup the file back and stderr/stdout handlers.

Thoughts?

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-09-02 01:20:42

Also what about the selinux bits?

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-09-03 02:20:52

rebased onto 462f9cdbf70d031b627559e702d213e1a79f8f39

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-09-03 02:24:42

I also see this seem annoying behavior from systemd every time I create an instance
dscreate from-file /data/dssetup.inf

I moved it to debug level so it won't be bagging during the non-verbose installation.

So instead of directing these to devnull, we should direct them to a python buffer via subprocess, then we should write these buffers to the log.debug. From there we can have the log code setup the file back and stderr/stdout handlers.
Thoughts?

Fully agree.
I've moved it to the debug and we can add a FileHandler option for logging in a separate issue (and it will be more of a new feature, I guess).

Anyway, this issue is ready for review. :)

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-09-03 02:59:03

selinux comments? you haven't addressed these ....

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-09-03 03:00:20

Ahhh you refactored it and I missed it.
Ack from me then.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-09-03 18:02:48

rebased onto aa17a8f

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-09-03 18:05:22

Pull-Request has been merged by droideck

[389-ds-bot]

Patch
50551.patch