Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to search by specific failure severity #56

Merged
merged 1 commit into from
Jul 29, 2019

Conversation

rcritten
Copy link
Collaborator

A WARNING is not a failure. Drop it from --failures only.

Add an option to search by specific failure severity.

@fcami
Copy link
Collaborator

fcami commented Jul 29, 2019

LGTM. One comment is that I'd feel better if we had a test for each CLI knob ( #57 ).

@fcami
Copy link
Collaborator

fcami commented Jul 29, 2019

So after thinking about it, I'm not so sure filtering WARNINGs from --failures-only is fine.

Consider the following WARNING:

  {
    "source": "ipahealthcheck.ipa.files",
    "check": "TomcatFileCheck",
    "result": "WARNING",
    "uuid": "c05bdaea-a53d-48a6-a152-e88c97a5a7d4",
    "when": "20190729095557Z",
    "duration": "0.000245",
    "kw": {
      "key": "_etc_pki_pki-tomcat_server.xml_mode",
      "path": "/etc/pki/pki-tomcat/server.xml",
      "type": "mode",
      "expected": "0660",
      "got": "0000",
      "msg": "Permissions of /etc/pki/pki-tomcat/server.xml are 0000 and should be 0660"
    }
  },

Since it's a warning:

# ipa-healthcheck --failures-only
[]

But (obviously) this leads into pki-tomcatd not starting after "ipactl restart":

# systemctl status pki-tomcatd@pki-tomcat.service
● pki-tomcatd@pki-tomcat.service - PKI Tomcat Server pki-tomcat
   Loaded: loaded (/lib/systemd/system/pki-tomcatd@.service; enabled; vendor preset: disabled)
  Drop-In: /etc/systemd/system/pki-tomcatd@pki-tomcat.service.d
           └─ipa.conf
   Active: failed (Result: exit-code) since Mon 2019-07-29 11:58:01 CEST; 51s ago
  Process: 1790 ExecStop=/usr/libexec/tomcat/server stop (code=exited, status=1/FAILURE)
  Process: 1421 ExecStartPost=/usr/libexec/ipa/ipa-pki-wait-running (code=exited, status=0/SUCCESS)
  Process: 1420 ExecStart=/usr/libexec/tomcat/server start (code=exited, status=143)
  Process: 1261 ExecStartPre=/usr/bin/pkidaemon start pki-tomcat (code=exited, status=0/SUCCESS)
  Process: 2153 ExecStartPre=/usr/sbin/pki-server migrate --instance pki-tomcat (code=exited, status=1/FAILURE)
 Main PID: 1420 (code=exited, status=143)

Jul 29 11:58:00 f29-idm0.laptop.example.org systemd[1]: Starting PKI Tomcat Server pki-tomcat...
Jul 29 11:58:01 f29-idm0.laptop.example.org pki-server[2153]: ERROR: Error reading file '/etc/pki/pki-tomcat/server.xml': failed to load external entity "/etc/pki/pki-tomcat/serv>
Jul 29 11:58:01 f29-idm0.laptop.example.org systemd[1]: pki-tomcatd@pki-tomcat.service: Control process exited, code=exited status=1
Jul 29 11:58:01 f29-idm0.laptop.example.org systemd[1]: pki-tomcatd@pki-tomcat.service: Failed with result 'exit-code'.
Jul 29 11:58:01 f29-idm0.laptop.example.org systemd[1]: Failed to start PKI Tomcat Server pki-tomcat.

This is because core/files.py uses "if mode != fmode:" and should use a more precise mode comparison: if fmode is more permissive that fmode, yield WARNING, elif fmode is less permissive than fmode, yield ERROR.

Obviously restarting ipa leads to a non-functional pki-tomcatd and then ipa-healthcheck catches some errors, but I would argue this is too late.

A related logic applies to IPACertmongerExpirationCheck where WARNING is used to catch expiring certificates. An admin using --failures-only would not catch the obvious action item unless it's too late, incurring downtime.

However the --severity filter is useful and nice.

Can you split this PR in two?

@rcritten rcritten changed the title Drop WARNING from --failures-only, add --severity option Add an option to search by specific failure severity Jul 29, 2019
@rcritten
Copy link
Collaborator Author

Converted this PR into only adding the ability to query based on severity level. We can tackle how to handle warnings in the future. My concern was with out to deal with false positives.

@fcami
Copy link
Collaborator

fcami commented Jul 29, 2019

hi @rcritten LGTM. I'm wondering whether we should change the logic - see comment above. But that's not mandatory.

It is a multi-valued option so can be provided on the command-line
multiple times to look for any range of severity levels.

This will let one search for only WARNING, for example, or for
ERROR and CRITICAL.
@rcritten
Copy link
Collaborator Author

Made --severity multi-valued so one can search on ERROR and CRITICAL, for example.

@fcami
Copy link
Collaborator

fcami commented Jul 29, 2019

Code LGTM, man page matches actual behavior. ACK.

@fcami fcami added the ack label Jul 29, 2019
src/ipahealthcheck/core/output.py Outdated Show resolved Hide resolved
@fcami fcami merged commit 3b69ae5 into freeipa:master Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants