Add version information to tasks #3930

mdellweg · 2023-06-16T15:27:36Z

Workers will refuse to pick up a task caliming a version of a plugin to be higher than their own. The corresponding plugin an it's version will be inferred from the task import path.

fixes #3369

MichalPysik · 2023-07-04T14:55:09Z

CHANGES/3369.feature

+Added version information to tasks. Workers will refuse to pickup tasks with a version higher
+than their own.


Suggested change

Added version information to tasks. Workers will refuse to pickup tasks with a version higher

than their own.

Added version information to tasks. Workers will refuse to pick up tasks with a version higher

than their own.

gerrod3 · 2023-07-10T15:32:56Z

pulpcore/tests/functional/api/test_tasking.py

@@ -42,19 +42,30 @@ def _dispatch_task(*args, **kwargs):
 @pytest.fixture(scope="module")
 def task(dispatch_task, monitor_task):
    """Fixture containing a Task."""
-    task_href = dispatch_task("time.sleep", args=(0,))
+    task_href = dispatch_task("pulpcore.app.tasks.test.sleep", args=(0,))


Interestingly we are losing the ability to run outside python functions as tasks with this change. Are we sure that no plugin has been using this behavior already? (I think this is a better design and we should only allow running pulpcore/plugin code)

Yes, I agree, we never made any such statement. But i'd be happy to call it undefined behaviour. Back in the days celery tasks needed to be decorated, and i think that is not the worst design. Actually I would want to be even much stricter about what a task can be. But that is for another day. (sneak preview: #3997)

gerrod3 · 2023-07-10T15:34:57Z

pulpcore/tasking/tasks.py

+    else:
+        function_name = func
+
+    versions = MODULE_PLUGIN_VERSIONS[function_name.split(".", maxsplit=1)[0]]


Referring to my previous comment, I feel this should be wrapped in a try/except block and raise a better error stating that the task function must live either inside pulpcore or a plugin!

ggainey · 2023-07-11T18:54:35Z

Would it be desirable for the tasking-system to recognize that none of the available tasks can be picked up due to version-incompatibility and log warnings?

Is there any way to have a test-case that places an incompatible-task-entry into the queue and then sees it not-get-picked-up?

mdellweg · 2023-07-11T19:28:22Z

Would it be desirable for the tasking-system to recognize that none of the available tasks can be picked up due to version-incompatibility and log warnings?

The distributed nature of the tasking system makes it kind of hard (without a lot of overhead including db queries) to identify that no available worker is compatible with the task at hand. Do we want that? This change is designed to help prevent bad to happen in a system that does rolling upgrades. So is suspect that the moment you spin up a new api worker you probable spin up a new worker too (maybe in the same container). And when you don't, you are in a half upgraded system and expect some quirks. It's not that the system is in an undefined state. The task will be picked up once at least one sufficiently recent worker is started.

Is there any way to have a test-case that places an incompatible-task-entry into the queue and then sees it not-get-picked-up?

Possibly. Not quite sure if we want that. The CI would probably not care that there will be a task waiting forever. Other development systems, idk.

mdellweg · 2023-07-12T10:11:18Z

I added a test, and also changed the semantics a little bit. A worker is now still able to cancel a task even if the versions are not met. I believe this is necessary to be able to clean up e.g. when an upgrade needed rollback or a plugin was removed.

@gerrod3 you may want to take a second look.

Workers will refuse to pick up a task claiming a version of a plugin to be higher than their own. The corresponding plugin and it's version will be inferred from the task import path. fixes pulp#3369

decko · 2023-07-12T11:59:02Z

pulpcore/tasking/pulpcore_worker.py

+            or parse_version(self.versions[label]) < parse_version(version)
+        ]
+        if unmatched_versions:
+            _logger.info(


What about using a _logger.warn here?

I had it warn first but wasn't sure. In case of an ongoing rolling upgrade this can get quite verbose. I was even thinking about debug.
We have conflicting suggestions here:
@ggainey > "I'd make it INFO. We def want to leave traces - esp if/when the task-queue is full of nothing but New Tasks but the only workers running are running Olde Code"

gerrod3 · 2023-07-12T13:33:39Z

pulpcore/tasking/pulpcore_worker.py

@@ -220,6 +221,23 @@ def cancel_abandoned_task(self, task, final_state, reason=None):
            self.notify_workers()
        return True

+    def is_compatible(self, task):
+        unmatched_versions = [
+            f"task: {label}>={version} worker: {self.versions.get(label)}"


Suggested change

f"task: {label}>={version} worker: {self.versions.get(label)}"

f"task: {label}>={version} worker: {self.versions.get(label, 'None')}"

Can you elaborate? My experiment (and it is part of the tests too) is that the stringinterpolation works just fine here.

Actually I don't think this is needed after checking the string interpolation. f"{None}" == "None".

gerrod3 · 2023-07-12T13:37:07Z

pulpcore/tasking/pulpcore_worker.py

+            f"task: {label}>={version} worker: {self.versions.get(label)}"
+            for label, version in task.versions.items()
+            if label not in self.versions
+            or parse_version(self.versions[label]) < parse_version(version)


I am not sure about this condition check. I think it should be !=. Maybe if plugin writers passed in the min required version for their tasks then we could use the this check, but if this is filled out by default we have no guarantee that a plugin's task is compatible with its previous version.

Then we'd basically be saying "you have to cancel all waiting tasks when you upgrade". A task can't declare future-compatibility, so by definition every time you update core or a plugin, existing tasks could never be handled.

Example: I have 200 waiting tasks issued by core/3.25. I upgrade to core/3.28. Using '!=' means now nothing can ever pick up those tasks.

zero-downtime-upgrade requires that new-task-code continue to be able to handle old-tasks; if we can't do that, we can't have ZDU.

No, the rule is whenever a worker has a version greater or equal than the api dispatching the task, it needs to be able to handle it. You cannot assume there will be an old worker around to handle all the old tasks.
We have that rule already. Plugins must always be able to handle old tasks. This is not even ZDU related.

Very well then, I guess this is a requirement I need to be cognizant of when reviewing plugin code as I was not aware of this before.

mdellweg marked this pull request as ready for review June 16, 2023 15:27

mdellweg force-pushed the zdu_tasks branch 5 times, most recently from 44259aa to ffceafb Compare June 19, 2023 09:14

mdellweg requested review from ggainey and decko June 19, 2023 11:04

mdellweg force-pushed the zdu_tasks branch from ffceafb to bc953a8 Compare June 27, 2023 07:59

mdellweg force-pushed the zdu_tasks branch from bc953a8 to af572c1 Compare July 4, 2023 08:31

MichalPysik reviewed Jul 4, 2023

View reviewed changes

mdellweg force-pushed the zdu_tasks branch 2 times, most recently from 21bedfc to 9e296ce Compare July 10, 2023 15:08

gerrod3 reviewed Jul 10, 2023

View reviewed changes

gerrod3 requested changes Jul 10, 2023

View reviewed changes

mdellweg force-pushed the zdu_tasks branch from 9e296ce to 24ba625 Compare July 11, 2023 12:43

gerrod3 approved these changes Jul 11, 2023

View reviewed changes

mdellweg force-pushed the zdu_tasks branch 2 times, most recently from b930878 to 8b94b40 Compare July 12, 2023 10:09

Add version information to tasks

103cfe7

Workers will refuse to pick up a task claiming a version of a plugin to be higher than their own. The corresponding plugin and it's version will be inferred from the task import path. fixes pulp#3369

mdellweg force-pushed the zdu_tasks branch from 8b94b40 to 103cfe7 Compare July 12, 2023 11:07

decko reviewed Jul 12, 2023

View reviewed changes

decko approved these changes Jul 12, 2023

View reviewed changes

ggainey approved these changes Jul 12, 2023

View reviewed changes

gerrod3 requested changes Jul 12, 2023

View reviewed changes

gerrod3 approved these changes Jul 12, 2023

View reviewed changes

mdellweg merged commit b0743ef into pulp:main Jul 12, 2023
15 checks passed

mdellweg deleted the zdu_tasks branch July 12, 2023 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add version information to tasks #3930

Add version information to tasks #3930

mdellweg commented Jun 16, 2023 •

edited

MichalPysik Jul 4, 2023

gerrod3 Jul 10, 2023

mdellweg Jul 10, 2023

gerrod3 Jul 10, 2023

ggainey commented Jul 11, 2023

mdellweg commented Jul 11, 2023

mdellweg commented Jul 12, 2023

decko Jul 12, 2023

mdellweg Jul 12, 2023

gerrod3 Jul 12, 2023

mdellweg Jul 12, 2023

gerrod3 Jul 12, 2023

gerrod3 Jul 12, 2023

ggainey Jul 12, 2023

mdellweg Jul 12, 2023

gerrod3 Jul 12, 2023

		Added version information to tasks. Workers will refuse to pickup tasks with a version higher
		than their own.

	f"task: {label}>={version} worker: {self.versions.get(label)}"
	f"task: {label}>={version} worker: {self.versions.get(label, 'None')}"

Add version information to tasks #3930

Add version information to tasks #3930

Conversation

mdellweg commented Jun 16, 2023 • edited

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

ggainey commented Jul 11, 2023

mdellweg commented Jul 11, 2023

mdellweg commented Jul 12, 2023

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

mdellweg commented Jun 16, 2023 •

edited