Inherit siteLists from upper level task while creating WMBS subscriptions #11724
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Jenkins results:
|
todor-ivanov
force-pushed
the
feature_EnforceSiteLists_fix-10270
branch
from
7b0324b
to
d76d52d
Compare
|
Jenkins results:
|
|
@amaltaro I have tested this PR just as before:
And here is what I see:
And one can clearly see that we now have records for all subscriptions and for all of them I am about to restart the FYI @amaltaro |
|
Upon releasing those two workflows, I can now see there are jobs in condor related to the toplevel task (still in idle though): And just to mention, for later discussion: FYI: @amaltaro |
amaltaro
reviewed
Sep 19, 2023
There was a problem hiding this comment.
Honestly, it is not clear to me whether we should set the site lists at the workload level or directly into the database, the latter is what is implemented here. The issue I see with this implementation is that the workload object will be different than what actually gets considered in WMBSHelper.
In other words, if site lists - or maybe we should be specific to the site disallowed list - is defined at the task level within a workload object, would we still need to have these changes made to the WMBSHelper?
In parallel to that, I think one hard requirement for this development to go in is, we need to be able to identify and potentially fail jobs that can only run at a site that is meant to NOT be allowed. Otherwise, this will introduce an eventual very deep and nasty behavior to debug in the future.
|
@todor-ivanov I forgot to mention, I created this wiki: and it would be great if you could move most of the initial PR content to that wiki. Such that we persist documentation in a more searchable way and also provide a cleaner PR description. |
|
Hi @amaltaro , Sorry but I do not understand what you mean by:
We do have it set at the workload level. The only problem is that we have a |
|
What I meant by "workload level" here is the actual WMWorkload object, which contains the whole description of the workflow, as well as all the tasks (WMTask) and steps (WMStep) objects and their definition. This is one example: What you provide in this PR indeed update the agent's database, which ultimately is what we need to have sites allowed/disallowed to be enforced in the subscriptions (all tasks of a workflow). However, these changes are not reflected in the WMWorkload object. In other words, if one looks at the workload description, only the top task will contain the site restrictions, while the agent is enforcing that to be applied to all the tasks. Said that, what I suggest is to define it for every task in the workload. It might be that the agent will then automatically add those site restrictions to the database without the explicit setter calls that you made here. But upon changing this code, please keep it in separate commits because I am not sure what can get exposed with such changes or not. Two items that come to my mind are:
|
|
Ok, and just to make things even more confusing now here is the result of a workflow which did have all the site limitations applied in WMBS but still the condor jobs materialized from this workflow include the wrong sites in their
FYI: @amaltaro |
|
Jenkins results:
|
|
Hi @amaltaro , And here is what the WMWorkload object says about one of the child tasks (a random one). |
|
Jenkins results:
|
|
@amaltaro I hope I have correctly understood your idea and have created this Commit: 117d550 With this, we will basically raise a WMCore exception on every job we find impossible to run because of lack of location. And we will do it as early as the Jobs are split in WMBS. Which IIUC was the goal here. Now two questions stand still:
[1] WMCore/src/python/WMComponent/JobSubmitter/JobSubmitterPoller.py Lines 331 to 338 in a73021b |
|
Todor, it looks like there is no exception involved, but we actually flag jobs that are supposed to be marked as and adopt the same mechanism, if possible, for this issue as well. |
|
@amaltaro I am not quite confident.... This mechanism seems inapplicable at this stage. At the time the JobFactory is executed by It seems the WMCore/src/python/WMCore/WMBS/Subscription.py Line 533 in a73021b |
and is site disallowed list enforced with that? The way I understand the job creation cycle is: JobCreatorPoller executes the algorithm, which executes JobFactory and the relevant JobSplitting algorithm (e.g. LumiBased), information is persisted in the database and before the cycle is over, those jobs have to be marked as The way we can mark jobs as I still don't think we need to wait for it to get propagated all the way to the JobSubmitter component. |
|
Jenkins results:
|
|
hi @amaltaro Ok, then. In this case my latest commit cfaa00e is going to be more inline with your idea. But I still fail to see how those job attributes will get propagated to BTW, If I read this correctly, the marker we opted to use here WMCore/src/python/WMCore/WMExceptions.py Line 211 in 4d906fe Which seems strange to me - having a single error code put every time a failed job report is generated. |
|
Those attributes don't come neither go to the relational database. It is all python object in memory. On your question about WMExceptions module, we use the error message from there whenever we require a static error message. The example above is just saying that a file is unprocessable, as it has too many events. Anyways, it's better not to add noise here. |
|
Ok, @amaltaro .... please feel free to go for another review, then. |
|
Jenkins results:
|
todor-ivanov
force-pushed
the
feature_EnforceSiteLists_fix-10270
branch
from
3862da4
to
793fb4d
Compare
|
Jenkins results:
|
|
Jenkins results:
|
|
@amaltaro please feel free to start reviewing the PR. I am currently testing it in one of mu agents. and in my dev environment. And so far, just as before, the Unfortunately due to some "Internal ServerError" the |
|
ok @amaltaro Now the result
NOTE: I am currently keeping
The disturbing part in the above output is that the only whitelisted site
I am about to release |
|
And we failed the parent task for some reason. And the reason I cannot find out what it is, because the FYI: @amaltaro |
|
ok @amaltaro, I finally made this test happen:
NOTE: I notice we do not propagate
|
|
@amaltaro This PR is ready. Please feel free to go for another review. |
amaltaro
requested changes
Oct 12, 2023
There was a problem hiding this comment.
@todor-ivanov this PR looks good in general. I left a couple of comments for improvements though.
| @@ -326,6 +326,12 @@ def _createSubscriptionsInWMBS(self, task, fileset, alternativeFilesetClose=Fals | |||
| mergedOutputFileset = None | |||
|
|
|||
| for childTask in task.childTaskIterator(): | |||
| # # NOTE: The two lines bellow will propagate the upper level task's site lists to the child tasks in wmbs only. | |||
There was a problem hiding this comment.
yes, the whole change to WMBSHelper will be removed while squashing the commits.
There was a problem hiding this comment.
Thanks for the review @amaltaro I just addressed your comment, could you please take another look.
…ions Typo Propagae siteBlacklist to all tasks in a wrkload && Revert changes to WMBSHelper Raise JobFactoryException if we find jobs with no location to run. Mark jobs with no location to run as failedOnCreation from JobFactory Add failedErrCode to the WMBS job attributes && Properly generate error jobreport based on the failedErrCode Add back import of WM_JOB_ERROR_CODES Update some log messages && Remove previous solution from WMBSHelper.
todor-ivanov
force-pushed
the
feature_EnforceSiteLists_fix-10270
branch
from
2664faa
to
34336c1
Compare
|
Jenkins results:
|
|
Jenkins results:
|
|
test this please |
|
Jenkins results:
|
amaltaro
approved these changes
Oct 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #10270
Status
Ready
Description
With the current PR we suggest a change in the way how we propagate tasks' Site lists during the creation of the respective subscriptions in wmbs.
Here, very important is to remember what the definition of a subscription is:
The change here would make the child tasks to inherit their respective site lists from the upper level tasks. This by itslef would trigger a record in the the
wmbs_subscription_validationtable per every subscription related to the child task in question, exactly as it is done for the upper level task. Bellow I put some example records at the WMBS tables reflecting the current situation:As we already said the combination between a
filesetand awmbs_workflowuniquely describes a subscription.filesetscreated for this workflow:wmbs_workflowrecords created for this workflow:wmbs_subscription_validationtable, which defines where a givenwmbs_workflow/taskis allowed to run or not:As one can see we have records related only to
subscription_id = 1. Which is the combination only between the top leveltaskand the top levelfilesetas they have been created by the local workqueu at the agent:With the current change I'd expect to have those created for all the rest of the workflow's subscriptions in WMBS
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
None
External dependencies / deployment changes
None