Skip to content

Log generic warning on non-systemd systems.#1450

Merged
blackboxsw merged 2 commits into
canonical:mainfrom
aciba90:better-warnings-non-systemd-ssh-status
May 13, 2022
Merged

Log generic warning on non-systemd systems.#1450
blackboxsw merged 2 commits into
canonical:mainfrom
aciba90:better-warnings-non-systemd-ssh-status

Conversation

@aciba90
Copy link
Copy Markdown
Contributor

@aciba90 aciba90 commented May 12, 2022

If the SSH status is checked in a non-systemd system, do not
distinguish between service statuses and log a generic warning
as we cannot expect status exit codes fulfilling any convention.

Proposed Commit Message

Log generic warning on non-systemd systems.

If the SSH status is checked in a non-systemd system, do not
distinguish between service statuses and log a generic warning
as we cannot expect status exit codes fulfilling any convention.

Additional Context

#1422 (comment)

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

Comment on lines +194 to +195
cloud = self.tmp_cloud(distro="ubuntu")
cloud.distro.init_cmd = ["service"]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The PR in 1449, which only just landed while you were working on this PR, refactored some unittests to use pytest instead of subclassing from CiTestCase. #1449 provided a parameterized TestHandleSSHPwauth test case test_no_restart_when_service_is_not_running. Would these new tests make more sense if parameterized in either TestHandleSSHPwauth.test_no_restart_when_service_is_not_running or a new parameterized testcase instead of separate unittests?

Two other minor points:

  • generally in unittests, I think we want to start avoiding calls to self.tmp_cloud and prefer tests.unittests.util.get_cloud instead as we try to retired tmp_cloud (and CiTestCase) where possible.
  • I don't think we need to set cloud.distro.init_cmd = ["service"] on ubuntu distro because default is init_cmd = ["service"] and this only gets reset to "systemctl" if uses_systemd == True, which you've mocked in this test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for breaking the integration tests and thanks for pointing out the updates.

I have parametrized the tests.

@aciba90 aciba90 marked this pull request as draft May 13, 2022 08:51
aciba90 added 2 commits May 13, 2022 10:51
If the SSH status is checked in a non-systemd system, do not
distinguish between service statuses and log a generic warning
as we cannot expect status exit codes fulfilling any convention.
@aciba90 aciba90 force-pushed the better-warnings-non-systemd-ssh-status branch from 08913c7 to 643a6fd Compare May 13, 2022 08:52
@aciba90 aciba90 marked this pull request as ready for review May 13, 2022 09:08
@aciba90
Copy link
Copy Markdown
Contributor Author

aciba90 commented May 13, 2022

@TheRealFalcon this PR is ready now. Thanks.

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your flexibility on this!

Since @blackboxsw requested changes, I'll wait until he gives his approval.

Copy link
Copy Markdown
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.

LGTM

@blackboxsw blackboxsw merged commit 0dfe51e into canonical:main May 13, 2022
@dermotbradley
Copy link
Copy Markdown
Contributor

@blackboxsw @TheRealFalcon. This change now means I get the following on the console for Alpine Linux systems (openrc/busybox init-based) where previously I did not:

2022-05-15 19:06:05,300 - cc_set_passwords.py[WARNING}: Ignoring config 'ssh_pwauth: False'. SSH service 'sshd' is not available. Error: unexpected error while running command.
Command: ['rc-service', 'sshd', 'status']
Exit code: 3
Reason: -
Stdout:  * status: stopped
Stderr: .

This is because the sshd service is started later in the boot sequence.

Also note that I have "ssh_pwauth: False" defined in /etc/cloud/cloud.cfg

@aciba90
Copy link
Copy Markdown
Contributor Author

aciba90 commented May 16, 2022

2022-05-15 19:06:05,300 - cc_set_passwords.py[WARNING}: Ignoring config 'ssh_pwauth: False'. SSH service 'sshd' is not available. Error: unexpected error while running command.
Command: ['rc-service', 'sshd', 'status']
Exit code: 3
Reason: -
Stdout: * status: stopped
Stderr: .

@dermotbradley I think we could fix this by:

  1. if service != systemd and status != 0 then applying the config
  2. reword the warning to be more generic, as it is not very accurate. We could say something like SSH service 'sshd' will not be restarted because it is not running or not available.

Let me prepare a PR.

@aciba90 aciba90 mentioned this pull request May 16, 2022
3 tasks
@aciba90 aciba90 deleted the better-warnings-non-systemd-ssh-status branch May 23, 2022 09:36
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.

4 participants