Requirements database #5412

richtja · 2022-06-23T22:10:39Z

This PR adds the ability to spawners to store requirements into cache.
This will make more efficient running multiple tests with the same
requirement or rerunning the jobs itself.

This also brings ability to use requirements with podman spawner. So now
is possible to run something like this:

class SuccessTest(Test):

    def check_hello(self):
        result = process.run("hello", ignore_status=True)
        self.assertEqual(result.exit_status, 0)
        self.assertIn('Hello, world!', result.stdout_text,)

    def test_a(self):
        """
        avocado dependency={"type": "package", "name": "hello"}
        """
        self.check_hello()

Warning: Right now, the requirement cache doesn't have any controls for
clearing cache or updating, and also didn't check the current
environment. So if the user do some changes to the environment outside
Avocado runtime, like uninstalling package, podman prune, removing
asset, etc. The tests with requirements will be broken.

Signed-off-by: Jan Richter [email protected]

avocado/core/task/statemachine.py

It changes order of runtime tasks in dependency graph, in such way that pre-test task are depending on each other, which ensures serial run in state machine for pre-test tasks. It will solve problems with conflicts between different pre-test task runs. Signed-off-by: Jan Richter <[email protected]>

The runtime task has information about all its dependencies. Let's add a method which will list all of them which have been already finished. Signed-off-by: Jan Richter <[email protected]>

The RuntimeTask has all the information to about dependencies to make a decision if the dependencies were resolved, and it is possible to run a task. So let's move this responsibility to the RuntimeTask. Signed-off-by: Jan Richter <[email protected]>

This adds few methods for manipulating with requirements cache. This will be useful for spawners which will use requirements cache. Signed-off-by: Jan Richter <[email protected]>

This commit add ability to spawners to store requirements into cache. This will make more efficient running multiple tests with the same requirement or rerunning the jobs itself. This also brings ability to use requirements with podman spawner. So now is possible to run something like this: class SuccessTest(Test): def check_hello(self): result = process.run("hello", ignore_status=True) self.assertEqual(result.exit_status, 0) self.assertIn('Hello, world!', result.stdout_text,) def test_a(self): """ 🥑 dependency={"type": "package", "name": "hello"} """ self.check_hello() Warning: Right now, the requirement cache doesn't have any controls for clearing cache or updating, and also didn't check the current environment. So if the user do some changes to the environment outside Avocado runtime, like uninstalling package, podman prune, removing asset, etc. The tests with requirements will be broken. Reference: Signed-off-by: Jan Richter <[email protected]>

clebergnu

Hi @richtja,

This is some GREAT work! I've spotted some things that need to be updated, and also some discussion points.

BTW, I tried quite hard to break this first version, and couldn't. Just about everything worked the way I expected.

For a new version, we need to improve the documentation and make this very visible to users (maybe even a section on the README).

clebergnu · 2022-06-28T11:19:41Z

avocado/core/task/runtime.py

@@ -201,29 +198,23 @@ def __init__(self, tests, test_suite_name, status_server_uri, job_id):
                                                     test_suite_name,
                                                     status_server_uri,
                                                     job_id)
-            self.graph[runtime_test] = runtime_test


There's a bit of "diff noise" here, with one empty line being removed, and another one added. Seems like it was unintentional.

clebergnu · 2022-06-28T11:28:39Z

avocado/core/task/runtime.py

@@ -62,6 +62,14 @@ def are_dependencies_finished(self):
                return False
        return True

+    def get_finished_dependencies(self):
+        """Returns all dependencies which already finished."""
+        finished = []


It's certainly a matter of taste, but I find list comprehensions more readable in situations like this. Suggestion:

def get_finished_dependencies(self): """Returns all dependencies which already finished.""" return [dep for dep in self.dependencies if dep.status and "FINISHED" in dep.status]

But as it is a matter of style, free to ignore this.

Yes, I can change that.

clebergnu · 2022-06-28T11:33:02Z

avocado/core/task/runtime.py

@@ -70,6 +69,15 @@ def get_finished_dependencies(self):
                finished.append(dependency)
        return finished

+    def is_possible_to_run(self):
+        dependency_finished = self.are_dependencies_finished()


Because the dependency_finished variable is never used again, this could become:

if self.are_dependencies_finished(): for dependency in self.dependencies: ...

Or:

if not self.are_dependencies_finished(): return False for dependency in self.dependencies: ...

clebergnu · 2022-06-28T11:39:53Z

avocado/core/task/statemachine.py

        result_stats = set(key.upper()for key in
                           self._state_machine._status_repo.result_stats.keys())
        if self._failfast and not result_stats.isdisjoint(STATUSES_NOT_OK):
            await self._state_machine.abort("FAILFAST is enabled")
            raise TestFailFast("Interrupting job (failfast).")

-        await self._state_machine.finish_task(runtime_task)
+        await self._state_machine.finish_task(runtime_task, "FINISHED")


I'm not sure about this, because without a status_reason, the log message produced will be:

Task "foo-bar" finished

And with this change, it will be:

Task "foo-bar" finished: FINISHED

I am not sure it adds value... To me, adds a bit of confusion.

Maybe we can change the log message here. Because the important part of this change is, that the finish_task will set the runtime_task status to FINISHED and the status is used for detecting all finished tasks in here.

I think adds some value, if we have all the states well defined. this is the reason and we could definitely be explicit about the reason. What are possible reasons for a finished task?

Maybe we can change the log message here. Because the important part of this change is, that the finish_task will set the runtime_task status to FINISHED and the status is used for detecting all finished tasks in here.

I'm fine with that, and I overlooked the function of the status. We can have the message something like:

Task "foo-bar" finished with status: FINISHED

That seems fine to me.

clebergnu · 2022-06-28T11:58:09Z

avocado/core/dependencies/requirements/cache/backends/sqlite.py

+                            requirement_type, requirement):
+    """Checks if requirement is in cache.
+
+        :rtype: True if requirement is in cache


These docstring lines are misaligned.

clebergnu · 2022-06-28T12:04:54Z

avocado/core/dependencies/requirements/cache/backends/sqlite.py

    return False
+
+
+def update_enviroment(environment_type, old_environment, new_environment):


Typo: s/enviroment/environment

clebergnu · 2022-06-28T12:18:27Z

avocado/core/task/statemachine.py

+
+                if is_task_in_cache:
+                    await self._state_machine.finish_task(
+                        runtime_task, "FINISHED: Task in cache")


I think FINISHED is not needed, given that finish_test will already put that information in the log.

FINISHED is important here because it will be part of runtime_task status and the status is used for detecting all finished tasks in here. It is the same problem as I mentioned in monitor method.

clebergnu · 2022-06-28T12:25:01Z

avocado/plugins/spawners/process.py

+from avocado.core.teststatus import STATUSES_NOT_OK
+
+ENVIRONMENT_TYPE = 'local'
+ENVIRONMENT = 'localhost.localdomain'


I am not sure if this is the best choice for the environment name. Maybe use the actual hostname?

sure, I will change that.

clebergnu · 2022-06-28T12:30:19Z

selftests/functional/serial/test_requirements.py

            result = process.run(command, ignore_status=True)
            self.assertEqual(result.exit_status, exit_codes.AVOCADO_ALL_OK)
            self.assertIn('PASS 1', result.stdout_text,)
            self.assertIn('SKIP 2', result.stdout_text,)
            self.assertNotIn('-foo-bar-', result.stdout_text,)

+    @unittest.skipUnless(os.getenv('CI'), skip_package_manager_message)


These could later be converted into Avocado tests and use variants (instead of the repeated test with the podman=True parameters).

Another point to be discussed and considered, is if we could/should rely on requirements mechanism itself to determine if podman tests should run (if podman package is available?), instead of using skip*.

This is a great idea, I will do that. I was so focused on the problem itself that I didn't use avocado features for making my testing easier 😅

richtja · 2022-06-28T13:38:41Z

Hi @richtja,

This is some GREAT work! I've spotted some things that need to be updated, and also some discussion points.

Thanks @clebergnu for review. I will use your suggestions. We just need to discuss the usage of runtime_task.status for detecting finished tasks.

BTW, I tried quite hard to break this first version, and couldn't. Just about everything worked the way I expected.

That is great to hear, thanks for the testing.

For a new version, we need to improve the documentation and make this very visible to users (maybe even a section on the README).

Sure, I will add it to the new version.

beraldoleal

Hi @richtja this LGTM, nice work! Just left a few comments for you.

beraldoleal · 2022-06-28T15:05:29Z

avocado/core/task/runtime.py

-            return hash(self.task.identifier)
-        return hash((str(self.task.runnable), self.task.job_id,
-                     self.task.category))
+        return hash(self.task.identifier)


beraldoleal · 2022-06-28T15:08:23Z

avocado/core/task/runtime.py

@@ -70,6 +69,15 @@ def get_finished_dependencies(self):
                finished.append(dependency)
        return finished

+    def is_possible_to_run(self):
+        dependency_finished = self.are_dependencies_finished()


Or:

if not self.are_dependencies_finished(): return False for dependency in self.dependencies: ...

beraldoleal · 2022-06-28T15:09:33Z

avocado/core/task/statemachine.py

@@ -214,7 +183,7 @@ async def check_finished_dependencies(runtime_task):

            # dependencies finished, let's check if they finished
            # successfully, so we can move on with the parent task
-            dependencies_ok = await check_finished_dependencies(runtime_task)
+            dependencies_ok = runtime_task.is_possible_to_run()


Just a matter of taste, so feel free to ignore, but to make this method shorter, I would go with "can_run()".

beraldoleal · 2022-06-28T15:11:30Z

avocado/core/task/statemachine.py

+            self._state_machine._status_repo.get_latest_task_data(
+                str(runtime_task.task.identifier)) or {}
+        # maybe, the results are not available yet
+        while latest_task_data.get("result", None) is None:


get() by default returns None. So I would remove the None from the get() method.

beraldoleal · 2022-06-28T15:14:08Z

avocado/core/task/statemachine.py

        result_stats = set(key.upper()for key in
                           self._state_machine._status_repo.result_stats.keys())
        if self._failfast and not result_stats.isdisjoint(STATUSES_NOT_OK):
            await self._state_machine.abort("FAILFAST is enabled")
            raise TestFailFast("Interrupting job (failfast).")

-        await self._state_machine.finish_task(runtime_task)
+        await self._state_machine.finish_task(runtime_task, "FINISHED")


I think adds some value, if we have all the states well defined. this is the reason and we could definitely be explicit about the reason. What are possible reasons for a finished task?

beraldoleal · 2022-06-28T15:24:15Z

avocado/plugins/spawners/process.py

+        return cache.is_requirement_in_cache(ENVIRONMENT_TYPE,
+                                             ENVIRONMENT,
+                                             kind,
+                                             name)


The fact that you are passing ENVIRONMENT_TYPE, ENVIRONMENT, kind and name for most of those cache methods, makes me think if is not worth ed to created an object for it.

richtja · 2022-06-29T15:59:42Z

Thanks @clebergnu and @beraldoleal for your reviews, I created new version #5418 with fixes.

richtja added the enhancement label Jun 23, 2022

richtja added this to the #98 milestone Jun 23, 2022

richtja requested review from beraldoleal and clebergnu June 23, 2022 22:10

richtja self-assigned this Jun 23, 2022

clebergnu reviewed Jun 23, 2022

View reviewed changes

avocado/core/task/statemachine.py Outdated Show resolved Hide resolved

clebergnu mentioned this pull request Jun 24, 2022

nrunner seems to not cleanup properly on timeout #5407

Closed

richtja added 5 commits June 28, 2022 11:08

List finished dependencies

aa32ed0

The runtime task has information about all its dependencies. Let's add a method which will list all of them which have been already finished. Signed-off-by: Jan Richter <[email protected]>

Requirements cache database updates

1064d53

This adds few methods for manipulating with requirements cache. This will be useful for spawners which will use requirements cache. Signed-off-by: Jan Richter <[email protected]>

richtja force-pushed the requirements_database branch from d2c0dda to c129e87 Compare June 28, 2022 09:27

clebergnu requested changes Jun 28, 2022

View reviewed changes

beraldoleal reviewed Jun 28, 2022

View reviewed changes

richtja mentioned this pull request Jun 29, 2022

Requirements database [v2] #5418

Closed

richtja closed this Jun 29, 2022

richtja deleted the requirements_database branch June 30, 2022 11:38

clebergnu mentioned this pull request Jul 1, 2022

Requirements database [v3] #5421

Merged

richtja mentioned this pull request Jul 11, 2022

LXC spawner proof of concept #4158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Requirements database #5412

Requirements database #5412

richtja commented Jun 23, 2022 •

edited

Loading

clebergnu left a comment

clebergnu Jun 28, 2022

clebergnu Jun 28, 2022

richtja Jun 28, 2022

clebergnu Jun 28, 2022

beraldoleal Jun 28, 2022

clebergnu Jun 28, 2022

richtja Jun 28, 2022 •

edited

Loading

beraldoleal Jun 28, 2022

clebergnu Jun 28, 2022

clebergnu Jun 28, 2022

clebergnu Jun 28, 2022

clebergnu Jun 28, 2022

richtja Jun 28, 2022 •

edited

Loading

clebergnu Jun 28, 2022

richtja Jun 28, 2022

clebergnu Jun 28, 2022

richtja Jun 28, 2022

richtja commented Jun 28, 2022

beraldoleal left a comment

beraldoleal Jun 28, 2022

beraldoleal Jun 28, 2022

beraldoleal Jun 28, 2022

beraldoleal Jun 28, 2022

beraldoleal Jun 28, 2022

beraldoleal Jun 28, 2022

richtja commented Jun 29, 2022

		return False


		def update_enviroment(environment_type, old_environment, new_environment):

Requirements database #5412

Requirements database #5412

Conversation

richtja commented Jun 23, 2022 • edited Loading

clebergnu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richtja Jun 28, 2022 • 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

richtja Jun 28, 2022 • 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

richtja commented Jun 28, 2022

beraldoleal 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richtja commented Jun 29, 2022

richtja commented Jun 23, 2022 •

edited

Loading

richtja Jun 28, 2022 •

edited

Loading

richtja Jun 28, 2022 •

edited

Loading