Conversation
IvanBM18
added
the
swarming
| # Should be routed normally to Kubernetes. | ||
| self.mock_k8s_service.create_utask_main_jobs.assert_called_once_with(tasks) | ||
|
|
||
| @mock.patch.object(remote_task_gate.RemoteTaskGate, '_is_swarming_applicable') |
There was a problem hiding this comment.
For this tests i used this @mock annotation convention because it was how this class mocks were setup and its way easier to continue to do so then to migrate them to our newer helpers.patch syntax.
|
|
||
| def _is_swarming_task(self, job_type): | ||
| return swarming.is_swarming_task(job_type) | ||
| return swarming.is_swarming_task(job_type, ignore_feature_flag=True) |
There was a problem hiding this comment.
instead of adding ignore_feature_flag in is_swarming_task, would it be clearer to check the feature flag before doing the action? e.g. before scheduling, before executing the task?
is_swarming_task sounds like a check that should always tell us whether a task is swarming or not, and we should have a separate is_swarming_enabled check before taking action.
There was a problem hiding this comment.
In this bot's case, we do check for the flag existence before doing the action (pulling tasks), but if by the next action (scheduling) the flag is down, then is_swarming_task would return false.
It was meant to be this way so swarming jobs, were tried in other platforms, but things have changed.
Agree on your separation of responsabilities, but since is_swarming_task is used quite a lot, Does it works for you if i handle this in a separate Refactor PR?
There was a problem hiding this comment.
yeah, follow up is fine. having to consider whether you should ignore_feature_flag in is_swarming_task seemed strange.
| logs.info( | ||
| f'[Swarming] Swarming flag not enabled, {len(swarming_tasks)} tasks' | ||
| ' unscheduled.') | ||
| unscheduled_tasks.extend(swarming_tasks) |
There was a problem hiding this comment.
what do we do with these unscheduled tasks if swarming is diabled? shouldn't they get thrown out?
There was a problem hiding this comment.
They get sent back to the utask_main-swarming queue, and they are not pulled back until the flag is turned on.
There was a problem hiding this comment.
Also no more new tasks would be pushed, and since they are not pulled back to any bot, i don't think discarding them would be our go to.
IvanBM18
added
the
ready for GOSST review
8d86442 to
11128be
Compare
4 participants
As found out in the parent PR: #5476
In the scheduler, when a task fails to get scheduled in swarming, it then tries to go on to other backends, so if for any reason(e.g. the platform is not supported in batch per the batch config), then the whole slice of batch tasks get rejected.
So for this PR, we make it so swarming tasks are only ever tried on swarming, and other tasks continue to be tried on other backends.
Also when making this changes i thought of an edge case not considered previously, what if there was a swarming task in the queue & then we disable the flag? Then the scheduled would mark the task as not swarming & would try it on other backends, and again this could show errors if the platform is not supported there. So i changed the validations.
With this changes task now appear in swarming!
Changes
swarming/__init__.py: Now we allow to validate if a given job is swarming or not without considering the flag.remote_task_gate.py: Splits tasks in swarming and other, swarming tasks get redirected to the swarming service and the unscheduled tasks from this service are not added back again with the other tasksTests
A bunch of Tests + refactoring some tests to support this new edge cases.
Left this changes running in dev for 24 hours, and no longer saw the error that canceled the whole slice of batch tasks if the job platform didn't existed for batch,
I only saw this error:
At first sight i also thought that a swarming job ended up being considered for batch, but we can ignore that because the real root cause to even being it considered for batch is because the job doesn't exist!
So, this kind of errors doesn't appear because of a swarming task was considered for batch, it appears because a non existen job was considered for batch, im positive this error happens because of the problems between the job-exporter and because that's a job that i manually inserted/scheduled to test the parent ticket.