Fixes #525 - Add checks to health subcommand

[qcjames53]

Why are you introducing these changes? (Problem description, related links). What are the changes introduced in this pull request?

This PR expands the existing foremanctl health subcommand to reach equivalency with the existing foreman-maintain health check tool.

The following tests are run on a default health check in foreman-maintain:

• services_up
• server_ping
• foreman_tasks/not_paused
• foreman/facts_names
• foreman_proxy/check_tftp_storage
• foreman_proxy/verify_dhcp_config_syntax
• puppet/verify_no_empty_cacert_requests
• system_registration

I've implemented these five checks:

• health_services_up - Checks all currently implemented quadlet services are running (with different configs supported)
• health_server_ping - Can we ping the foreman server?
• health_foreman_tasks_not_paused - Are there paused foreman tasks?
• health_facts_count - Does a host exist with more facts than health_max_fact_values_per_host?
• check_duplicate_permissions - Are permissions duplicated.

I've skipped these checks (reasoning provided):

• foreman_proxy/check_tftp_storage - No foreman proxy support yet.
• foreman_proxy/verify_dhcp_config_syntax - Ditto.
• puppet/verify_no_empty_cacert_requests - Puppet plugins for foreman and smart proxy are not yet compatible with foremanctl; work is underway to add support in the future.
• system_registration - This self-registration concern isn't valid for an upstream centos9 container, to the best of my knowledge. Please double check this reasoning :)

How to test this pull request

Steps to reproduce:

1. Spin up an environment and register a host.
2. Run foremanctl health and verify all checks pass.
3. vagrant ssh quadlet and manually stop a service. Rerun the check and confirm health_services_up fails. Restart the service.
4. Close the server firewall and confirm health_server_ping fails. Change the config back afterwards.
5. Start a sync (or similar) and pause the job. confirm health_foreman_tasks_not_paused fails.
6. Lower the health_max_fact_values_per_host value in src/vars/health.yml to below the number of facts of your host and confirm health_facts_count fails.
7. Run the unit tests and confirm they work. ./forge test first to generate needed files then run pytest tests/health_test.py.
8. Create duplicate permissions in the foreman db and verify the health check fails. Run with --fix and see that the duplicates are cleared.

Checklist

• Tests added/updated (if applicable)
• Documentation updated (if applicable) <- Didn't see anything relevant to modify

[qcjames53]

None of these test failures seem related. FWIW I cannot get the full unit test suite to run locally without issue either. My unit test passes, though :)

[ehelms]

@qcjames53 Before I review in-depth, can you review #431? This is a previous contribution that has made overlapping progress and addresses some of what would be questions I have for this PR. Ideally, we would build on #431

[qcjames53]

Of course, Eric. #431 wasn't on our team's radar at all so I'll need some time to rectify. I'll reach out to make sure we're not duplicating more effort than this.

[qcjames53]

This PR should now be up to par. I rebased on master and added a few additional checks to the existing foremanctl health subcommand (as well as a unit test). Let me know if you think we need negative-case testing for this.

[qcjames53]

The newest push adds a flag to ignore errored out foreman tasks (for automation mainly). I've also removed health checks from the GH automation since the unit test checks it instead.

[qcjames53]

The latest force push handles db auth correctly for external dbs.

[qcjames53]

I just pushed some changes to add --fix for duplicate permissions to help out Aneta's docs PR. We'd have to drop the sections on fixing duplicate permissions otherwise and it was an easy add.

New unit tests added to cover this and --ignore-foreman-tasks-check.

[qcjames53]

Newest push should take care of linting.

[qcjames53]

@ehelms @ekohl I just pushed changes that should address both of your concerns. I've feature gated check_foreman_tasks and check_foreman_api properly and added unit testing. I've also changed check_host_facts to return the IDs of all hosts that exceed the threshold. I've updated documentation where neccessary.

[ekohl]

Not for this PR, but perhaps it's easier to add mkdocs and instruct the agent on how to use that instead. Just like https://github.com/theforeman/foreman-infra/tree/master/docs generates https://theforeman.github.io/foreman-infra/.

[qcjames53]

That would be a big upgrade. I didn't want to modify the docs TOO much for a PR where sweeping changes are obviously out of scope.

[ehelms]

My opinion: building and trying to maintain a static site as the source of truth introduces overhead. Instead of reading locally, agents now have to query over the network. If you are making changes in a PR to any documentation and trying to consume those updates, you have to generate a static site. Additionally, agents then also have to parse HTML to find the relevant bits rather than just quickly parsing local markdown.

[ehelms]

@qcjames53 a reboot should fix the iop tests

[qcjames53]

Thanks Eric. I rebased and pushed to rerun CI. @ekohl would you like to review this PR again or are you okay with current changes?

[qcjames53]

Ewoud uncovered https://projects.theforeman.org/issues/38465 which can cause duplicate permissions. Around 4:30 today I will re-add the duplicate permissions check for this release and include instructions in the markdown file to remove this check once the above issue is addressed.

Edit: I'm not planning on adding "--fix" since that's silly to carry over. It will be fail-only and the onus is on the user to clean up the duplicate permissions. Let me know if that's not the ideal approach.

[qcjames53]

I've created #551 to track migrating these db queries to authenticated API calls.

I've also re-added the check_duplicate_permissions role (error detection only, no --fix) and documented that it needs to stay until https://projects.theforeman.org/issues/38465 is addressed.

[qcjames53]

I force pushed to re-run automation (previous failure was unrelated to my changes).

[ehelms]

EL10 failure is unrelated, based on other tests I am going to merge.

[ianballou]

This file does not exist.

[ianballou]

what is health_fix doing here?