-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add job controller, job monitor and tools to support compute4punch backend #430
Add job controller, job monitor and tools to support compute4punch backend #430
Conversation
I opened this as a draft pull request in order to start a discussion with the maintainers. At the moment I have added two new arguments to the compute4punch job controller knowing that they need to be added to the I have found #32, but it seems that only |
54768b0
to
dc0fc33
Compare
6abe063
to
8177ab5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## maint-0.9 #430 +/- ##
=============================================
- Coverage 45.99% 39.72% -6.27%
=============================================
Files 17 18 +1
Lines 1235 1470 +235
=============================================
+ Hits 568 584 +16
- Misses 667 886 +219
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @giffels, sorry for the delay!
I have left some comments after my first look at this PR, most of them are just due to my small knowledge of HTCondor and of Compute4PUNCH ;)
I still haven't looked at the token authentication part, though, and I haven't run the code yet.
I have found #32, but it seems that only htcondor_max_runtime and htcondor_accounting_group have been implemented so far?
Yes, those are the only ones as far as I can tell.
Regarding these new values:
- c4p_cpu_cores
- c4p_memory_limit
- c4p_additional_requirements
Even though the first two could be harmonised with the ones for Kubernetes, I guess that for now these will work. We will also need to add them to the specification of reana.yaml
.
@tiborsimko what do you think? Is it ok to keep these new values separate from the ones used for k8s jobs? See also this related comment: #32 (comment)
One last thing: how hard would it be to have at least some small tests for these changes? Those are the only way for us to make sure that the code keeps working also in the future, as we do not have access on Compute4PUNCH and we will not be able to test the integration with REANA there. |
0fea500
to
db63edd
Compare
Sure, it would be really useful to have test coverage for this. I will work on this. |
16159ad
to
4fc51e5
Compare
515c974
to
a244df3
Compare
a244df3
to
fa3ecab
Compare
Except for some comments that are left, I have not found issue with my local deployment of REANA, even when using HTCondor@CERN and Slurm@CERN, so these changes should not affect other REANA instances that do not use the C4P backend. |
9b856fa
to
52b903c
Compare
Confirmed, just tried HTCondorCERN, and the |
52b903c
to
6e62daa
Compare
6e62daa
to
3581378
Compare
3581378
to
f20e523
Compare
f20e523
to
4243252
Compare
This pull request contains a job controller, job monitor and tools to support the compute4punch backend.
HELMHOLTZ_TOP
token needs to be added as secret using thereana-client
.condor_q
andcondor_history
commands.