Add integration test for power_state_change module by TheRealFalcon · Pull Request #717 · canonical/cloud-init

TheRealFalcon · 2020-12-08T16:10:21Z

Proposed Commit Message

Add integration test for power_state_change module

Additional Context

Based on #567 , but not naming the test after it because the changes that affected Ubuntu were only a refactor.

In the test, I've included adding a timeout to the user_data, but I don't know of a way to actually test it because the power state module is the last module to run, so even with a timeout of 1, cloud-init finishes before the timeout is triggered.

Test Steps

pytest tests/integration_tests/modules/test_power_state_change.py

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

TheRealFalcon · 2020-12-08T16:37:40Z

@@ -0,0 +1,13 @@
+def ordered_items_in_text(to_verify: list, text: str):


log_utils.py is better than utils.py, right???

But in all seriousness, I'm open to suggestions of a better place to put something like this.

OddBloke

Thanks James! The test itself LGTM, with a few minor nits/comments; the framework changes, however, I think may not be correct.

OddBloke · 2020-12-08T17:21:26Z

@@ -0,0 +1,13 @@
+def ordered_items_in_text(to_verify: list, text: str):


Suggested change

def ordered_items_in_text(to_verify: list, text: str):

def ordered_items_in_text(to_verify: list, text: str) -> bool:

(And I suppose s/list/Iterable[str]/ but let's not import typing just for that. ;))

OddBloke · 2020-12-08T17:21:46Z

+    def running(self):
+        # TODO: Implement something better in pycloudlib
+        try:
+            return True if self.execute('true').return_code == 0 else False


Suggested change

return True if self.execute('true').return_code == 0 else False

return self.execute('true').ok

Bah! True if ... else False is a code peeve of mine and I just committed it! (but yes, I'll update this 😉 )

OddBloke · 2020-12-08T17:38:22Z

+        with session_cloud.launch(
+            user_data=USER_DATA.format(
+                delay=delay, mode=mode, timeout=timeout, condition='true'),
+            launch_kwargs={'wait': False},


This feels a bit like a conflation. We universally opt out of pycloudlib's default waiting behaviour (i.e. launch(..., wait=False, ...)) and implement our own. I believe that what we're configuring here is that we don't want our waiting behaviour to apply (in addition to the invariant of pycloudlib's waiting not applying). As launch_kwargs is used to pass kwargs to pycloudlib's launch method, it feels wrong to be using it to carry configuration which, by design, will never be passed through to launch.

Given that we are actually configuring our launch method to not wait here, should this instead be a wait=True kwarg on session_cloud.launch?

OddBloke · 2020-12-08T17:51:47Z

+    # detecting the first boot or second boot, so we also check
+    # the logs to ensure we've booted twice. If the logs show we've
+    # only booted once, wait until we've booted twice
+    instance.instance.wait()


Should we be passing raise_on_cloudinit_failure=False here?

OddBloke · 2020-12-08T18:01:22Z

+    ])
+    def test_poweroff(self, session_cloud: IntegrationCloud,
+                      mode, delay, timeout, expected):
+        with session_cloud.launch(


This is a nice example of the flexibility of the framework we have; good job!

OddBloke · 2020-12-08T18:03:19Z

+            assert instance.running
+        lines_to_check = [
+            'Running module power-state-change',
+            expected.format(delay=delay),


Looks like all of the values expected can take are straight-up strings, so I think we can drop the .format here?

OddBloke · 2020-12-08T18:14:29Z

+from tests.integration_tests.log_utils import ordered_items_in_text
+import pytest
+import time
+
+from tests.integration_tests.clouds import IntegrationCloud
+from tests.integration_tests.instances import IntegrationInstance


https://www.python.org/dev/peps/pep-0008/#imports 🏃 💨

(A nit.)

OddBloke · 2020-12-08T18:34:03Z

            return image.image_id

    def _perform_launch(self, launch_kwargs):
+        wait = launch_kwargs.pop('wait', True)


If wait isn't in launch_kwargs then it will default to True, meaning that cloud-init failures will raise an exception. (More on this issue further down.)

OddBloke · 2020-12-08T22:39:18Z

This isn't running successfully for me locally; I've seen this a few times (but not every time):

E           OSError: Failed reading remote file via cat: /var/log/cloud-init.log
E           Return code: 1
E           Stderr: sudo: unknown user: ubuntu
E           sudo: unable to initialize policy plugin
E           Stdout:

But also:

>       assert ordered_items_in_text(lines_to_check, log)
E       assert False
E        +  where False = ordered_items_in_text(['Running module power-state-change', 'will execute: shutdown -r now msg', "running 'init-local'", 'config-power-state-change already ran'], "2020-12-08 22:30:56,727 - util.py[DEBUG]: Cloud-init v. 20.3-2-g371b392c-0ubuntu1~20.04.1 running 'init-local' at Tue...nds (0.12)\n2020-12-08 22:31:23,855 - handlers.py[DEBUG]: finish: init-local: SUCCESS: searching for local datasources")

(That assert could probably use a message, if you can figure out a good one to emit.)

OddBloke · 2020-12-08T22:40:10Z

This isn't running successfully for me locally

(Other than this, the code LGTM now, thanks!)

OddBloke · 2020-12-09T14:52:38Z

This isn't running successfully for me locally

(Let me know if there's anything I can do to help debug!)

TheRealFalcon · 2020-12-09T20:50:35Z

@OddBloke I don't think it's worth investing anymore time in this test at this point. I get different weird errors (or just bugs in pycloudlib and lxd) from different clouds, and it's not worth the time fixing them now. I can either remove the test and run things manually, or keep the test around knowing its flaky and remove it from the normal test runs. I think there is value in keeping the test (hence my last commit), but I also understand not wanting to dirty our test base with flaky tests, so I'd be ok removing it too. What are your thoughts?

OddBloke · 2020-12-09T21:12:19Z

    supported_os_set = set(os_list).intersection(test_marks)
    if current_os and supported_os_set and current_os not in supported_os_set:
        pytest.skip("Cannot run on OS {}".format(current_os))
+    if 'unstable' in test_marks:


Is it worth adding RUN_UNSTABLE as a setting, so the interface for forcing a run is CLOUD_INIT_RUN_UNSTABLE=true rather than a code modification?

OddBloke · 2020-12-09T21:19:06Z

+# This test is marked unstable because even though it should be able to
+# run anywhere, I can only get it to run in an lxd container, and even then
+# occasionally some timing issues will crop up.
+@pytest.mark.unstable


It might be nice to have a timed element like we have in the curtin tests: unstable_until("2021/02/01") to force us to fix it at some point. Not needed for now, though.

OddBloke

LGTM, thanks! Ideally we'd have a stable test here, but pragmatically we're better landing this (which we know is inert by default, and useful enough for SRU verification) and moving forward.

Add integration test for power_state_change module

20fcc5a

TheRealFalcon commented Dec 8, 2020

View reviewed changes

OddBloke reviewed Dec 8, 2020

View reviewed changes

review comments

f25f7c6

TheRealFalcon requested a review from OddBloke December 8, 2020 21:10

Mark the power state change test unstable

3a2e147

TheRealFalcon force-pushed the test-power-state-change branch from 8b85251 to 3a2e147 Compare December 9, 2020 20:45

OddBloke reviewed Dec 9, 2020

View reviewed changes

Comment thread tests/integration_tests/modules/test_power_state_change.py Outdated

make run unstable configureable

2c7f39d

OddBloke reviewed Dec 9, 2020

View reviewed changes

OddBloke approved these changes Dec 9, 2020

View reviewed changes

OddBloke and others added 2 commits December 9, 2020 16:20

Merge branch 'master' into test-power-state-change

1275ccb

Update tests/integration_tests/modules/test_power_state_change.py

1ee6622

OddBloke merged commit 17ca02e into canonical:master Dec 9, 2020

TheRealFalcon deleted the test-power-state-change branch August 19, 2021 20:38

TheRealFalcon restored the test-power-state-change branch August 19, 2021 20:38

This was referenced May 12, 2023

Release 21.1 #3846

Closed

Randomly set credentials written in cleartext to world-readable file #3854

Closed

TheRealFalcon deleted the test-power-state-change branch March 21, 2025 18:47

		@@ -0,0 +1,13 @@
		def ordered_items_in_text(to_verify: list, text: str):

	return True if self.execute('true').return_code == 0 else False
	return self.execute('true').ok

Conversation

TheRealFalcon commented Dec 8, 2020

Proposed Commit Message

Additional Context

Test Steps

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OddBloke commented Dec 8, 2020

Uh oh!

OddBloke commented Dec 8, 2020

Uh oh!

OddBloke commented Dec 9, 2020

Uh oh!

TheRealFalcon commented Dec 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants