Refactor status.py

[TheRealFalcon]

Proposed Commit Message

refactor: Refactor status.py

The one major functional change in this commit is around how we detect
running vs error states. Status reporting has a fundamental problem in
that we can't accurately tell if cloud-init is done because cloud-init
is actually several processes. There isn't always a way to tell whether
a service isn't running because it simply hasn't started yet vs the
service being blocked/crashed and will never start/finish.

In the past, if any of the cloud-init services reported an error, we
would assume that cloud-init as a whole has crashed and report that
cloud-init is "done", but with error. This commit flips that logic to
assume that cloud-init is always running unless we see indication that
cloud-init has completely finished. This means that
`cloud-init status --wait` may run forever if cloud-init has crashed or
is blocked on another service. This is preferable to returning early
and potentially allowing provisioning scripts that wait for cloud-init
to continue provisioning. On systemd-enabled systems, there is extra
logic to inspect the state of the services, so this should rarely be a
problem in practice.

Additionally, this commit includes the following refactoring:
- Split UXAppStatus into RunningStatus and ConditionStatus so they can
  be tracked independently
- Simplify the tabular printing
- On error print extended_status as "error - done" or "error - running"
- Add several helper functions in `get_status_details` to simplify
  logic
- Rename `_get_error_or_running_from_systemd` to `systemd_failed`
  and only return if error is detected
- Change "is running" logic to be determined solely by the existence
  of the status.json and results.json files.

Additional Context

Test Steps

Checklist

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

Merge type

• Squash merge using "Proposed Commit Message"
• Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

[holmanb]

Thanks @TheRealFalcon, huge maintainability improvements here - and a much better UI too. Thanks for doing this. I have a couple of minor change requests inline - mostly just comments and questions.

We have a couple of strings that still need to be updated in the docs, comments, and tests:

$ rg "degraded running"
cloudinit/cmd/status.py
165:    # Handle the "degraded done" and "degraded running" states

doc/rtd/howto/status.rst
58:    "degraded running"

$ rg "degraded done"
doc/rtd/howto/status.rst
43:      "extended_status": "degraded done",
57:    "degraded done"

doc/rtd/explanation/exported_errors.rst
27:      "extended_status": "degraded done",
83:      "extended_status": "degraded done",

cloudinit/cmd/status.py
165:    # Handle the "degraded done" and "degraded running" states

tests/unittests/cmd/test_status.py
671:                    "extended_status": "degraded done",

[holmanb]

This was formerly named after the UXAppStatus it contained, however we rename NOT_RUN to NOT_STARTED under RunningStatus. Could we update this variable to also follow the rename?

i.e.

Suggested change

	STATUS_DETAILS_NOT_RUN = status.StatusDetails(
	STATUS_DETAILS_NOT_STARTED = status.StatusDetails(

and the variable updated throughout?

[holmanb]

Which service to check is unknown to the user by this message, but we could easily return the service name that failed from systemd_failed() for a more informative error message.

I'm not requesting this change now, but it's an idea for a future improvement we could make.

[holmanb]

We might eventually want to abstract systemd_failed() into the distros/ directory (or maybe even an init-system-specific abstraction) so that other init systems can be better extended to check status. This isn't something we need to do today, just an idea for the future.

[holmanb]

This logic normally should work, but I think it would be slightly safer to do if boot_status_code in DISABLED_BOOT_CODES before is_running(status_file, result_file) due to edge cases. This is because I think that boot_status_code is a higher confidence signal than the file existence.

A system that experiences a cloud-init crash that leaves behind a status file but not a result file will return RunningStatus.RUNNING (non-systemd). While this makes sense, if cloud-init is subsequently disabled we will still report RunningStatus.RUNNING due to the existence of this file.

[holmanb]

After this change, the message to stderr below might get printed once from both get_status_details()::get_bootstatus() and again in get_status_details()::systemd_failed() when cloud-init status is called early in boot. Is this intentional? +1 either way, but it just seems unnecessary from a user's perspective.

[TheRealFalcon]

I refactored it some to account for this. Let me know if that works for you

[holmanb]

style question: Was it intentional to use parenthesis on this line and the next return but not the final one? I personally prefer never using them when returning tuples, but I'm curious if there is some stylistic reason for doing it this way?

[TheRealFalcon]

Nope, just lack of conscious thought. I was also notorious at a previous company for mixing double quotes and single quotes in the same file/function, but thankfully black has made that a non-issue for me. 😄

I removed the parens from these.

[TheRealFalcon]

Also force pushed a rebase

[holmanb]

LGTM, thanks @TheRealFalcon!