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

fix: Handle systemctl when dbus not ready #4842

Merged
merged 2 commits into from Feb 2, 2024

Conversation

TheRealFalcon
Copy link
Member

@TheRealFalcon TheRealFalcon commented Feb 2, 2024

Proposed Commit Message

fix: Handle systemctl when dbus not ready

See commit d29b744, but this commit generalizes the solution for all
calls in status.py

Additional Context

#4681 #4676 LP: #2046483

Test Steps

Checklist

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>)

See commit d29b744, but this commit generalizes the solution for all
calls in status.py
@blackboxsw blackboxsw self-assigned this Feb 2, 2024
@@ -93,6 +93,36 @@ class StatusDetails(NamedTuple):
{description}"""


def query_systemctl(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good utility function to generalize interaction with systemctl. While we see other call-sites throughout cloud-init to systemctl in cloudini/net/network_manager, cloudinit/net/activators.py, cloudinit/sources/helpers/azure.py and cloudinit/config/cc_mounts.py, these entry points are much later in boot than a potential early-boot invocation of cloud-init status from external consumers. So I don't necessarily think we "need" this logic elsewhere for this PR. It may be worth generalizing it in util or somewhere if we find another call-site that warrants early boot interaction and exposure to the socket not yet being responsive.

Copy link
Collaborator

@blackboxsw blackboxsw Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation is very status specific as well. So I don't think it's worth generalizing further at this time.

Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 and minor unittest fix. Both otherwise looks good to me.

tests/unittests/cmd/test_status.py Outdated Show resolved Hide resolved
@blackboxsw
Copy link
Collaborator

@TheRealFalcon please add the footer LP: #2046483 to the commit message when squashed/rebase merged.

Co-authored-by: Chad Smith <chad.smith@canonical.com>
@TheRealFalcon TheRealFalcon merged commit 1f6eddd into canonical:main Feb 2, 2024
29 checks passed
@TheRealFalcon TheRealFalcon deleted the fix-status branch February 2, 2024 21:17
TheRealFalcon added a commit to TheRealFalcon/cloud-init that referenced this pull request Feb 2, 2024
See commit d29b744, but this commit generalizes the solution for all
calls in status.py

LP: #2046483
TheRealFalcon added a commit that referenced this pull request Feb 2, 2024
See commit d29b744, but this commit generalizes the solution for all
calls in status.py

LP: #2046483
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants