user reliability workflow - creating reservation out of a job #1145

prakashcv13 · 2019-05-28T12:45:49Z

Describe Bug or Feature

When a job encounters a problem with application/data/script/etc, the job exits and the resources allocated to the job are released back to the server. If the user wants to run the job again (after correcting the issues that caused the problem), they need to re-submit the job which could take a while to run (depending on various factors like number of jobs in the queue, priority, scheduling policy etc.). If the user needs resources to be allocated for a time period so that the job can be re-submitted and can run without delays, they can use reservations. As of now reservations have a start and end time, so it is not yet possible to have a reservation that is schedule-able.

Describe Your Change

This PR focuses on creating a reservation out of a job.

Link to Design Doc

Design

Attach Test and Valgrind Logs/Output

hook demo.txt
qsub demo.txt
pbs_rsub demo.txt

ptl.txt

prakashcv13 · 2019-09-03T06:56:28Z

@PBSPro/pbspro-maintainers - please do not close this PR. I understand it is inactive.

agrawalravi90 · 2019-11-30T04:22:23Z

@prakashcv13 why don't you close this and re-open when you are ready? it has 0 reviews so there's really not much value in keeping this open right?

prakashcv13 · 2020-02-10T16:56:07Z

this PR is ready for review - PTL tests are pending for some scenarios - I will push them in a day or two.

bhroam · 2020-02-11T01:33:38Z

src/include/batch_request.h

@@ -175,7 +175,7 @@ struct rq_py_spawn  {

 struct rq_move {
 	char	rq_jid[PBS_MAXSVRJOBID+1];
-	char	rq_destin[(PBS_MAXSVRJOBID > PBS_MAXDEST ? PBS_MAXSVRJOBID:PBS_MAXDEST)+1];
+	char	rq_destin[(PBS_MAXSVRJOBID > PBS_MAXDEST ? PBS_MAXSVRJOBID:PBS_MAXDEST)+2];


With +1 people can guess it is for a null byte. I think +2 deserves a comment.

i don't think we need +2 there, so reverting the change.

bhroam · 2020-02-11T01:34:46Z

src/include/pbs_error.h

@@ -260,6 +260,8 @@ extern "C" {

 #define PBSE_SOFTWT_STF		     15180 /* soft_walltime is incompatible with STF jobs */

+#define PBSE_RESVFROMRESVJOB	     15181 /* A reservation used to create a reservation */
+#define PBSE_RESVFROMARRJOB	     15182 /* A reservation used to create a reservation */


Do you mean a job used to create a reservation? Also, I don't understand how these two error codes differ. Also, some underscores might make them more readable

15181 is for creating a reservation out of a job within a reservation
15182 is for creating a reservation out of an array job.
I have inserted underscores and updated the comments as well

bhroam · 2020-02-11T01:37:27Z

src/lib/Libattr/master_resv_attr_def.xml

+	<member_at_comp>comp_str</member_at_comp>
+	<member_at_free>free_str</member_at_free>
+	<member_at_action>NULL_FUNC</member_at_action>
+	<member_at_flags><both>READ_WRITE | ATR_DFLAG_ALTRUN | ATR_DFLAG_SELEQ | ATR_DFLAG_MOM</both></member_at_flags>


Does the mom care about this attribute? Actually does ATR_DFLAG_MOM mean anything to a reservation? It usually means to send a job attribute to the mom.

no - removed the flag.

bhroam · 2020-02-11T01:40:11Z

src/server/hook_func.c

+					snprintf(log_buffer, sizeof(log_buffer),
+						"Found job '%s' attribute flagged to be set",
+						ATTR_create_resv_from);
+					log_event(PBSEVENT_DEBUG2, PBS_EVENTCLASS_HOOK, LOG_ERR, phook->hook_name, log_buffer);


Consider using log_eventf(). Also, I don't think you need to use %s. ATTR_create_from_resv is going to be replaced with a quoted string. You should be able to do "found job '" ATTR_create_from_resv "' attribute flagged to be set"

bhroam · 2020-02-11T01:43:25Z

src/server/queue_func.c

+ */
+
+resc_resv *
+find_resvbyquename(char *quename)


Our standard suggests using underscores between words

bhroam · 2020-02-12T19:53:09Z

test/tests/functional/pbs_user_reliability.py

+        jid = self.server.submit(job)
+        self.server.expect(JOB, {ATTR_state: 'R'}, jid)
+
+        rid = 'R' + str(int(jid.split(".")[0]) + 1)


Don't assume this. When we get multi-server, ids will be all over the place. Do a self.server.status(RESV)[0]['id'] (here and other tests)

you are right, but here we do not have multi-server, this test will run on a single server system only, here, we can safely assume this.

I disagree. Don't assume ids. Query them. No other test I've read has assumed IDs to be like this, they query them.

bhroam · 2020-02-12T19:54:10Z

test/tests/functional/pbs_user_reliability.py

+        delete_hook = [qmgr_cmd, '-c', 'delete hook %s' % hook_name]
+        ret = self.du.run_cmd(self.server.hostname,
+                              cmd=delete_hook, sudo=True)
+        self.assertEqual(ret['rc'], 0)


Hooks will be deleted in setUp(). No need to delete it.

bhroam · 2020-02-12T20:02:17Z

test/tests/functional/pbs_user_reliability.py

+        resv = Reservation(job=jid)
+        self.server.submit(resv)
+
+        rid = 'R' + str(int(jid.split(".")[0]) + 1)


self.server.submit() returns the rid. No need to assume the id.

bhroam · 2020-02-12T20:03:58Z

test/tests/functional/pbs_user_reliability.py

+        try:
+            self.server.submit(resv)
+        except PbsSubmitError as e:
+            self.assertTrue(msg in e.msg[0])


Add an else: self.fail() to both of these try/except blocks (here and below)

bhroam · 2020-02-12T20:05:16Z

test/fw/ptl/lib/pbs_testlib.py

@@ -14262,7 +14266,7 @@ class Reservation(ResourceResv):

    dflt_attributes = {}

-    def __init__(self, username=TEST_USER, attrs=None, hosts=None):
+    def __init__(self, username=TEST_USER, attrs=None, hosts=None, job=None):


Instead of this, maybe key off of the reservation attribute instead of adding an argument to Reservation()? That's how it will work in API mode.

bhroam · 2020-02-21T01:02:46Z

src/server/hook_func.c

-				}
+				if (new_conv_str != NULL)
+					log_eventf(PBSEVENT_DEBUG2, PBS_EVENTCLASS_HOOK, LOG_ERR, phook->hook_name,
+						"Found job "ATTR_create_resv_from_job" attribute flagged to be set");


Since the %s was taken out, you can just use log_event() now. Also, put spaces after the " and before the next opening " around the macro. Right now it looks like it is quoted and not the other strings.

bhroam · 2020-02-21T01:10:01Z

src/server/req_runjob.c

-			pjob->ji_qs.ji_jobid, log_buffer);
+			pjob->ji_qs.ji_jobid, "batch request allocation failed");
+		log_event(PBSEVENT_SYSTEM, PBS_EVENTCLASS_JOB, LOG_ERR,
+			pjob->ji_qs.ji_jobid, "Could not create reservation from the job");


Combine these log messages into one.

i wanted to keep 2 separate log messages, but let us combine them into one.

bhroam · 2020-02-21T01:42:26Z

src/server/req_quejob.c

-	if (resc_def != NULL) {
-		job_resc_entry = find_resc_entry(&pjob->ji_wattr[(int)JOB_ATR_resource], resc_def);
+	job_resc_entry = (resource *)GET_NEXT(pjob->ji_wattr[(int)JOB_ATR_resource].at_val.at_list);
+	while (job_resc_entry) {


You are iterating, use a for() loop.

bhroam · 2020-02-21T01:45:20Z

src/server/req_quejob.c

+		resc_def = find_resc_def(svr_resc_def, WALLTIME, svr_resc_size);
+		if (resc_def != NULL) {
+			resv_resc_entry = find_resc_entry(&presv->ri_wattr[(int)RESV_ATR_resource], resc_def);
+			if (resv_resc_entry == NULL) {


If walltime is copied, how will this ever be NULL? We only get in here if we copied walltime, so it should be set.

if JOB_ATR_resource does not have the entry for walltime, we set the walltime of 5 years.

bhroam · 2020-02-24T21:07:23Z

src/server/queue_func.c


 	(void)strncpy(qname, quename, PBS_MAXDEST);
 	qname[PBS_MAXDEST] ='\0';
 	pc = strchr(qname, (int)'@');	/* strip off server (fragment) */
 	if (pc)
 		*pc = '\0';
 	pque = (pbs_queue *)GET_NEXT(svr_queues);
-	while (pque != NULL) {
+	for (; pque != NULL; pque = (pbs_queue *)GET_NEXT(pque->qu_link)) {


Why not add the initialization into the for() header? (here and below)

bhroam · 2020-02-24T21:09:40Z

test/tests/functional/pbs_user_reliability.py

@@ -80,15 +80,17 @@ def test_create_resv_from_job_using_runjob_hook(self):
        jid = self.server.submit(job)


FYI (above), you don't need setUp() if the only thing you do is call the super's setUp()

bhroam · 2020-02-24T21:10:34Z

test/tests/functional/pbs_user_reliability.py


        a = {ATTR_job: jid, 'reserve_state': "RESV_RUNNING",
             'Resource_List.select': '1:ncpus=3',
             'Resource_List.walltime': 9999}
        self.server.expect(RESV, a, id=rid)

+        a = {ATTR_queue: rid}


When checking for the state of a reservation do (MATCH_RE, 'RESV_RUNNING|5)' (here and other places)

prakashcv13 · 2020-02-27T06:58:21Z

hey @bhroam - could you please let me know if there are further comments.

bhroam

LGTM

riyazhakki · 2020-02-28T09:08:58Z

src/server/job_func.c

@@ -1089,6 +1089,9 @@ find_job(char *jobid)
 	char buf[PBS_MAXSVRJOBID+1];

 	/* Make a copy of the job ID string before we modify it. */
+	if (jobid == NULL)


I think above comment is for the sprintf statement, can you move your changes above the comment?

riyazhakki · 2020-02-28T09:20:03Z

src/server/queue_func.c

+
+/**
+ * @brief
+ * 		find_resvbyquename() - find a reservation by the name of its queue


find_resv_by_quename

riyazhakki · 2020-02-28T09:52:25Z

src/server/req_quejob.c

-	char		 tbuf1[256] = {0};
-	char		 tbuf2[256] = {0};
-	int		 is_maintenance = 0;
+	char buf[PBS_MAXUSER + PBS_MAXHOSTNAME + 64];


This buf is left uninitialized.

riyazhakki · 2020-02-28T09:56:09Z

src/server/req_quejob.c

-	/*for resources that are not specified in the request and
-	 *for which default values can be determined, set these values
-	 *as the values for those resources
+	/* for resources that are not specified in the request and


Please make it doxygen standard.

riyazhakki · 2020-02-28T10:05:19Z

src/server/req_runjob.c

+	plhed = &newreq->rq_ind.rq_queuejob.rq_attr;
+	CLEAR_HEAD(newreq->rq_ind.rq_queuejob.rq_attr);
+	if ((psatl = attrlist_create(ATTR_resv_job, NULL, len)) != NULL) {
+


Please remove this extra line.

riyazhakki · 2020-02-28T10:06:57Z

src/server/req_runjob.c

@@ -1966,3 +1969,47 @@ req_defschedreply(struct batch_request *preq)

 	reply_send(preq);
 }
+
+void


Doxygen comment for the function is missing.

bhroam · 2020-02-29T00:18:50Z

src/server/req_runjob.c

+/**
+ * @brief
+ *	convert_job_to_resv - creates a reservation out of the job
+ * 			      and moved the job to the newly created


I think you want the present tense here (move). Also, I you might want to make creates not plural.

prakashcv13 · 2020-03-03T04:13:13Z

hey @bhroam, shall we go ahead and merge this PR. If there are any more changes, please let me know.

bhroam

LGTM

prakashcv13 · 2020-03-04T07:25:56Z

thank you @bhroam and @riyazhakki for the review

prakashcv13 changed the title ~~[WIP] NASA user reliability~~ [WIP] user reliability May 28, 2019

agrawalravi90 added the inactive This pull request has been inactive for > 2 weeks label Dec 12, 2019

prakashcv13 removed the inactive This pull request has been inactive for > 2 weeks label Dec 20, 2019

prakashcv13 changed the title ~~[WIP] user reliability~~ user reliability workflow - creating reservation out of a job Jan 10, 2020

bhroam requested changes Feb 12, 2020

View reviewed changes

bhroam requested changes Feb 21, 2020

View reviewed changes

bhroam requested changes Feb 24, 2020

View reviewed changes

prakashcv13 added 8 commits February 27, 2020 12:44

user reliability

29ab89f

PTL tests and bug fixes

ec29826

Address review comments from Bhroam

66e0b82

pep8 errors

1c9838d

pep8 and build issues on Ubuntu.

5b27d9f

Copying all in Resource_List of job to Resource_List of the reservation

e65704b

Address Bhroam's review comments - round 2

007db80

Address Bhroam's review comments - round 3

c4b4ca2

bhroam approved these changes Feb 27, 2020

View reviewed changes

riyazhakki reviewed Feb 28, 2020

View reviewed changes

Address review comments from Riyaz

016f28e

bhroam requested changes Feb 29, 2020

View reviewed changes

Change doxygen wording

5066995

riyazhakki approved these changes Mar 2, 2020

View reviewed changes

bhroam approved these changes Mar 3, 2020

View reviewed changes

bhroam merged commit c047c42 into openpbs:master Mar 3, 2020

prakashcv13 mentioned this pull request Mar 5, 2020

Fix build error on RHEL 8 #1552

Merged

This was referenced Mar 6, 2020

set ncpus for user reliability tests #1559

Closed

Fixing user reliability test cases #1574

Merged

prakashcv13 deleted the nasa_reliable branch June 2, 2020 09:10

		@@ -80,15 +80,17 @@ def test_create_resv_from_job_using_runjob_hook(self):
		jid = self.server.submit(job)

user reliability workflow - creating reservation out of a job #1145

user reliability workflow - creating reservation out of a job #1145

Conversation

prakashcv13 commented May 28, 2019 • edited

Describe Bug or Feature

Describe Your Change

Link to Design Doc

Attach Test and Valgrind Logs/Output

prakashcv13 commented Sep 3, 2019

agrawalravi90 commented Nov 30, 2019

prakashcv13 commented Feb 10, 2020

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

prakashcv13 Feb 13, 2020 • 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

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

Choose a reason for hiding this comment

bhroam Feb 24, 2020 • edited

Choose a reason for hiding this comment

prakashcv13 commented Feb 27, 2020

bhroam 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

Choose a reason for hiding this comment

prakashcv13 commented Mar 3, 2020

bhroam left a comment

Choose a reason for hiding this comment

prakashcv13 commented Mar 4, 2020

prakashcv13 commented May 28, 2019 •

edited

prakashcv13 Feb 13, 2020 •

edited

bhroam Feb 24, 2020 •

edited