Spawner: require runtime operation suitability #5649

clebergnu · 2023-04-14T21:01:46Z

This adds a new mandatory method to spawners: is_operational(). The goal of this method is to signal to the execution of the test suite whether the spawner is fully set up and capable to operate. This will of course, depend on the spawner implementation and requirements.

In the case of the podman spawner, often times jobs configured to use that spawner will succeed, when they actually need to signal a failure. This is what happens on a system without a working podman installation:

   # avocado run --spawner=podman -- /bin/true
   JOB ID     : daf6869a348f14c52460adc6f18f89f35f8d6ecd
   JOB LOG    : /root/avocado/job-results/job-2023-04-14T20.57-daf6869/job.log
   RESULTS    : PASS 0 | ERROR 0 | FAIL 0 | SKIP 1 | WARN 0 | INTERRUPT 0 | CANCEL 0
   JOB TIME   : 0.32 s
   [root@22d3a3b15197 ~]# echo $?
   0

Clearly, test workflows depending on the tests errouneously succeed. With this change, an error is reported both on the logs/UI and on the exit code.

richtja

Hi @clebergnu, thank you for this. I don't see any blockers for merging this, but I just have some comments which I would like to discuss firs.

richtja · 2023-04-20T14:04:30Z

avocado/core/plugin_interfaces.py

@@ -424,6 +424,17 @@ async def update_requirement_cache(runtime_task, result):
        :type result: `avocado.core.teststatus.STATUSES`
        """

+    @staticmethod
+    @abc.abstractmethod
+    def is_operational():


Is it necessary to have this as staticmethod. IIUIC you don't use it anywhere it static context.

It's actually used like this (static) in the process spawner.

Yes, but I meant that you never call it as static method. This is used in avocado/plugins/runner_nrunner.py where you're initializing the spawner object. Therefore, IMO this method doesn't have to be a static method and it will fix the pylint W0221 warning on PodmanSpawner.

If this method is obligatory for all spawners is there any chance we can at least add something like

@staticmethod def is_operational(): return True

to the LXC spawner within the same pull request, i.e. on the same ground as both the process and podman spawners?

Regarding the static method choices, I also think simple instance-bound methods should be good enough unless we really need something more complicated but I am aware that the spawners in general have been started with mostly static method and this and converting to simpler methods might not be within the scope of this pull request.

I'll change is_oeprational() to be an instance method. The fact that the podman spawner needs that, is a good enough "hint" that it's the better choice indeed.

richtja · 2023-04-20T14:09:52Z

avocado/plugins/runner_nrunner.py

+        spawner_name = test_suite.config.get("run.spawner")
+        spawner = SpawnerDispatcher(test_suite.config, job)[spawner_name].obj
+        if not spawner.is_operational():
+            msg = f'Spawner "{spawner_name}" is not operational, aborting execution of suite {test_suite.name}'


I am not sure if this message is sufficient, because it doesn't say anything about why the spawner is not operational. IMO, this should be more verbose and each spawner should try to describe why it is not operational.

My take is that each spawner is free to log as much as it wants about the failure. The main goal of this is to stop having false positives and lead users into investigating the issue.

We can add some method of passing the failure reason from the spawner to the runner, and having the runner log it, but I feel it's unnecessary.

Ok, I don't have a problem to leave this responsibility to spawners, but then I would add logs about failure to the PodmanSpawner. Because IMO Spawner "PodmanSpawner" is not operational doesn't give you many hints about have happened and what you need to fix.

Indeed, if the spawner can detect it has a problem it should hopefully also be able to report what that problem is.

For the podman spawner, I've added reporting for three possible errors:

The podman binary could not be found

The execution of the podman binary signaled failure (return code xxx)"

The podman binary did not report a suitable version (>= 3.0)

pevogam

Hi @clebergnu, I also left some comments as I am now slightly more in sync with changes involving spawners.

pevogam · 2023-06-13T07:28:37Z

avocado/core/plugin_interfaces.py

@@ -424,6 +424,17 @@ async def update_requirement_cache(runtime_task, result):
        :type result: `avocado.core.teststatus.STATUSES`
        """

+    @staticmethod
+    @abc.abstractmethod
+    def is_operational():


If this method is obligatory for all spawners is there any chance we can at least add something like

@staticmethod def is_operational(): return True

to the LXC spawner within the same pull request, i.e. on the same ground as both the process and podman spawners?

pevogam · 2023-06-13T07:30:21Z

avocado/core/plugin_interfaces.py

@@ -424,6 +424,17 @@ async def update_requirement_cache(runtime_task, result):
        :type result: `avocado.core.teststatus.STATUSES`
        """

+    @staticmethod
+    @abc.abstractmethod
+    def is_operational():


Regarding the static method choices, I also think simple instance-bound methods should be good enough unless we really need something more complicated but I am aware that the spawners in general have been started with mostly static method and this and converting to simpler methods might not be within the scope of this pull request.

pevogam · 2023-06-13T07:31:12Z

avocado/plugins/runner_nrunner.py

+        spawner_name = test_suite.config.get("run.spawner")
+        spawner = SpawnerDispatcher(test_suite.config, job)[spawner_name].obj
+        if not spawner.is_operational():
+            msg = f'Spawner "{spawner_name}" is not operational, aborting execution of suite {test_suite.name}'


Indeed, if the spawner can detect it has a problem it should hopefully also be able to report what that problem is.

clebergnu · 2023-07-18T15:48:53Z

@richtja @pevogam I believe I've addressed all issues you guys raised. Let me know if I'm missing something.

And thanks for the review!

clebergnu · 2023-07-18T19:11:02Z

/packit copr-build

pevogam · 2023-07-20T16:07:46Z

Great step forward @clebergnu, I will make sure to take a look and update all PRs/issues waiting for further input on my side by the end of the week.

pevogam

Hi @clebergnu just a few tiny things since there is much to say and this PR is more or less good to go. My main point is about a tiny extra step regarding the LXC spawner.

avocado/core/plugin_interfaces.py

pevogam · 2023-07-23T13:12:56Z

avocado/plugins/runner_nrunner.py

+        spawner = SpawnerDispatcher(test_suite.config, job)[spawner_name].obj
+        if not spawner.is_operational():
+            suite_name = f" {test_suite.name}" if test_suite.name else ""
+            msg = f'Spawner "{spawner_name}" is not operational, aborting execution of suite{suite_name}. Please check the logs for more information.'


I guess one would naturally look into the logs next which would allow us to not have to provide a long message like this comment can be fully ignored.

avocado/plugins/spawners/podman.py

avocado/plugins/spawners/lxc.py

richtja

Hi @clebergnu, thank you for your changes. Expect the @pevogam comments it LGTM.

pevogam · 2023-08-28T12:07:24Z

Hi @clebergnu do you think you could prioritize this so that I could move forward with the remote spawner request and have enough time to complete everything there before the LTS release?

When a job gets interrupted, users want to notice it. So far, it's been presented in the Human UI as a simple "INFO" message, that may "fade into the background" with the other messages. By bumping it a level to a warning, it should be easier for users to notice it. This is not made into a error, because the caused for the interruption may be an expected situation, such as a job timeout. Signed-off-by: Cleber Rosa <[email protected]>

clebergnu · 2024-01-08T21:03:31Z

@richtja @pevogam I've rebased this PR, so a second round of review is probably a good thing.

pevogam

Ok, LGTM and thanks for adding the is_operational methods to all spawners, the current choices are definitely reasonable for the current use.

richtja

Hi @clebergnu, thank you for this updated. I have just one comment related to Fedora image in the selftest.

selftests/functional/plugin/spawners/podman.py

This adds a new mandatory method to spawners: is_operational(). The goal of this method is to signal to the execution of the test suite whether the spawner is fully set up and capable to operate. This will of course, depend on the spawner implementation and requirements. In the case of the podman spawner, often times jobs configured to use that spawner will succeed, when they actually need to signal a failure. This is what happens on a system without a working podman installation: # avocado run --spawner=podman -- /bin/true JOB ID : daf6869a348f14c52460adc6f18f89f35f8d6ecd JOB LOG : /root/avocado/job-results/job-2023-04-14T20.57-daf6869/job.log RESULTS : PASS 0 | ERROR 0 | FAIL 0 | SKIP 1 | WARN 0 | INTERRUPT 0 | CANCEL 0 JOB TIME : 0.32 s [root@22d3a3b15197 ~]# echo $? 0 Clearly, test workflows depending on the tests errouneously succeed. With this change, an error is reported both on the logs/UI and on the exit code. Signed-off-by: Cleber Rosa <[email protected]>

clebergnu · 2024-01-10T13:49:35Z

Hi @clebergnu, thank you for this updated. I have just one comment related to Fedora image in the selftest.

FYI, just pushed a version with the requested change.

clebergnu force-pushed the spawner_is_operational branch from 9262f98 to 328a9ef Compare April 14, 2023 21:03

clebergnu marked this pull request as ready for review April 15, 2023 01:57

clebergnu self-assigned this Apr 15, 2023

clebergnu added this to the #102 (102 Dalmatians) milestone Apr 15, 2023

clebergnu force-pushed the spawner_is_operational branch from 328a9ef to 503639e Compare April 17, 2023 16:41

richtja self-requested a review April 19, 2023 13:39

richtja reviewed Apr 20, 2023

View reviewed changes

pevogam reviewed Jun 13, 2023

View reviewed changes

richtja modified the milestones: #102 (102 Dalmatians), #103 (LTS release - Codename TBD) Jun 26, 2023

clebergnu force-pushed the spawner_is_operational branch from 503639e to 3d1ddc3 Compare July 18, 2023 15:43

clebergnu force-pushed the spawner_is_operational branch 3 times, most recently from 5f3eab9 to d6bc2c2 Compare July 18, 2023 17:01

clebergnu mentioned this pull request Jul 18, 2023

Introduce remote spawner functionality #5621

Merged

clebergnu requested review from richtja and pevogam July 20, 2023 11:36

pevogam reviewed Jul 23, 2023

View reviewed changes

richtja approved these changes Jul 24, 2023

View reviewed changes

pevogam mentioned this pull request Sep 13, 2023

103 LTS Testing with external projects and test suites #5766

Closed

9 tasks

clebergnu modified the milestones: #103 (LTS release - Sound of Freedom), #104 - Codename TBD Nov 15, 2023

clebergnu force-pushed the spawner_is_operational branch 2 times, most recently from fad914c to edce2a0 Compare January 8, 2024 21:02

pevogam approved these changes Jan 9, 2024

View reviewed changes

richtja requested changes Jan 9, 2024

View reviewed changes

selftests/functional/plugin/spawners/podman.py Outdated Show resolved Hide resolved

clebergnu force-pushed the spawner_is_operational branch from edce2a0 to 5ebb619 Compare January 10, 2024 13:47

richtja approved these changes Jan 10, 2024

View reviewed changes

richtja merged commit aa82dac into avocado-framework:master Jan 10, 2024
63 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spawner: require runtime operation suitability #5649

Spawner: require runtime operation suitability #5649

clebergnu commented Apr 14, 2023

richtja left a comment

richtja Apr 20, 2023

clebergnu Apr 20, 2023 •

edited

Loading

richtja Apr 21, 2023

pevogam Jun 13, 2023

pevogam Jun 13, 2023

clebergnu Jul 18, 2023

richtja Apr 20, 2023

clebergnu Apr 20, 2023

richtja Apr 21, 2023

pevogam Jun 13, 2023

clebergnu Jul 18, 2023

pevogam left a comment

pevogam Jun 13, 2023

pevogam Jun 13, 2023

pevogam Jun 13, 2023

clebergnu commented Jul 18, 2023

clebergnu commented Jul 18, 2023

pevogam commented Jul 20, 2023

pevogam left a comment

pevogam Jul 23, 2023

richtja left a comment

pevogam commented Aug 28, 2023

clebergnu commented Jan 8, 2024

pevogam left a comment

richtja left a comment

clebergnu commented Jan 10, 2024

Spawner: require runtime operation suitability #5649

Spawner: require runtime operation suitability #5649

Conversation

clebergnu commented Apr 14, 2023

richtja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clebergnu Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pevogam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clebergnu commented Jul 18, 2023

clebergnu commented Jul 18, 2023

pevogam commented Jul 20, 2023

pevogam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richtja left a comment

Choose a reason for hiding this comment

pevogam commented Aug 28, 2023

clebergnu commented Jan 8, 2024

pevogam left a comment

Choose a reason for hiding this comment

richtja left a comment

Choose a reason for hiding this comment

clebergnu commented Jan 10, 2024

clebergnu Apr 20, 2023 •

edited

Loading