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

holidays file enhancements #979

Merged
merged 1 commit into from Feb 13, 2019

Conversation

Projects
None yet
4 participants
@agrawalravi90
Copy link
Contributor

agrawalravi90 commented Feb 4, 2019

Feature Description

  • Holidays file has information about prime/non-prime windows for days of the week for a site. Right now, there's no documented behavior for what happens if data for any of the days is missing or if the data is inconsistent. Also, the behavior for when the year entry is missing is not clearly defined. The general use case, as well as the theme of the prime-time feature in PBS, seems to be to assume prime time if data is insufficient, so it seems like a good idea to officially enforce this behavior. It is also desired to have an empty/commented out holidays file which doesn't need to be updated every year so that it is all prime-time by default.

Affected Platform(s)

  • All

Design

Solution Description

  • Now, after parsing holidays file, if there's data missing for a day, we enforce 'all' prime-time for it
  • If an entry in holidays file sets both prime and non-prime start times to 'all'/'none', we enforce 'all' prime-time for it.
  • The holidays file will now be completely commented out by default, so it'll be 24x7 prime-time

Testing logs/output

Checklist:

***For further information please visit the Developer Guide Home.***

@agrawalravi90 agrawalravi90 force-pushed the agrawalravi90:holidays branch from c45a841 to 1682f1e Feb 4, 2019

* @return void
*/
static void
handle_missing_prime_info(void)

This comment has been minimized.

@bhroam

bhroam Feb 4, 2019

Contributor

This should happen in init_config()

This comment has been minimized.

@agrawalravi90

agrawalravi90 Feb 5, 2019

Author Contributor

I wanted to do the same thing, but, if somebody adds "all all" as prime and non prime start in holidays file, then we need to be able to catch that in parse_holidays and set all prime-time, not all non-prime time. We don't have a good way of catching that if we set the bits to "all prime" inside init_config() itself as we won't be able to tell inside load_day() whether the bits are all prime because the holidays file has "all" entry for prime, or if it is what was set by init_config(). So, we could refactor the function if you really hate this, but otherwise i can't put it inside init_config().

This comment has been minimized.

@bhroam

bhroam Feb 5, 2019

Contributor

I don't understand. You call handle_missing_prime_info() right at the top of parse_holidays(). There should be no difference between the same code being called in init_config() or in parse_holidays()

This comment has been minimized.

@agrawalravi90

agrawalravi90 Feb 6, 2019

Author Contributor

No, I call it at the end after parsing the holidays file

jid1 = self.server.submit(j1)

# Verify that the job does not run as it is 24hrs prime-time
self.server.expect(JOB, {'job_state': 'Q'}, jid1)

This comment has been minimized.

@bhroam

bhroam Feb 4, 2019

Contributor

Check to see that comment is set before checking for job_state: Q. There is a race that you do your expect() before the scheduler even sees the job.

/*
* The user specified both all prime and all non-prime for this day
* all prime wins in such a situation, so just ignore the non-prime info
*/

This comment has been minimized.

@bhroam

bhroam Feb 4, 2019

Contributor

Turn this comment into a more concise log message. This is an error case, so we shouldn't just assume something without saying it.

This comment has been minimized.

@agrawalravi90

agrawalravi90 Feb 5, 2019

Author Contributor

Just to be sure, you want me to actually log a message to sched logs here, or just trim the comment?

This comment has been minimized.

@bhroam

bhroam Feb 5, 2019

Contributor

The comment as a comment was fine. I want a scheduler log message. If we're adding a scheduler log message, I don't think we need both a comment and the log message. For a log message, the comment is a bit wordy.

if (pr == NON_PRIME && conf.prime[d][PRIME].none == TRUE) {
/*
* The user specified both none prime and none non-prime for this day
* Make it all prime-time for this day as we don't know better

This comment has been minimized.

@bhroam

bhroam Feb 4, 2019

Contributor

Same as above about the log message.

@@ -1069,7 +1069,7 @@ struct config
fairshare_head *fairshare; /* fairshare tree */
time_t decay_time; /* time in seconds for the decay period*/
time_t sync_time; /* time between syncing usage to disk */
struct t prime[HIGH_DAY][HIGH_PRIME]; /* prime time start and prime time end*/
struct t prime[HIGH_DAY][2]; /* prime time start and prime time end*/

This comment has been minimized.

@bhroam

bhroam Feb 4, 2019

Contributor

Why the change from HIGH_PRIME to 2?

This comment has been minimized.

@agrawalravi90

agrawalravi90 Feb 5, 2019

Author Contributor

the other 2 rows (corresponding to ALL and NONE) were not used, so it was just unused memory.

This comment has been minimized.

@bhroam

bhroam Feb 5, 2019

Contributor

This will probably never happen, but the idea was if we ever add more time windows than just prime and non-prime, this will just automatically update itself. We'll add a new value to the enum and HIGH_PRIME will increase. The amount of lost memory is pretty minor.

@agrawalravi90 agrawalravi90 changed the title holidays file: set 'all' prime-time for missing/inconsistent days holidays file enhancements Feb 6, 2019

@agrawalravi90

This comment has been minimized.

Copy link
Contributor Author

agrawalravi90 commented Feb 6, 2019

@bhroam thanks for your comments, I've addressed them. I also made some additional changes according to the comments on the design from the forums. Please provide feedback.


def check_nonprime(self):
"""
Helper function to test that it is non-prime time right now

This comment has been minimized.

@bhroam

bhroam Feb 6, 2019

Contributor

If the log_filter isn't filtering DEBUG2, the scheduler will tell you what prime status it is in and when it ends in the log. Consider using the log message.

"""
Test that scheduler assumes all prime time if the year entry is
missing from holidays file
"""

This comment has been minimized.

@bhroam

bhroam Feb 6, 2019

Contributor

You really have tested this above. You are testing what the scheduler will do with a completely commented out holidays file. Above, the year was comment out, and here the commented out year is missing. In either case the entire file is commented out.

This comment has been minimized.

@agrawalravi90

agrawalravi90 Feb 7, 2019

Author Contributor

I know, but I'd like to keep it just to be thorough.

This comment has been minimized.

@bhroam

bhroam Feb 8, 2019

Contributor

Then make the test different. Set non-prime such that it should be in non-prime, but since the year is missing it will be 24hr prime.

This comment has been minimized.

@agrawalravi90

agrawalravi90 Feb 11, 2019

Author Contributor

Actually I think the test already does that, it sets 24x7 non-prime time and then checks that it's 24x7 prime-time because of the missing year entry

This comment has been minimized.

@bhroam

bhroam Feb 11, 2019

Contributor

It has 3 lines that are commented out. I'm saying don't have them commented out. Have them set non-prime. This way we can easily check if 24hr prime is working.

This comment has been minimized.

@agrawalravi90

agrawalravi90 Feb 12, 2019

Author Contributor

D'oh! Those lines were never meant to commented out, sorry about that, yes, the test would be very much the same as the previous one with them commented out :) I've uncommented them now, please let me know if it looks good now.

*
YEAR 2018
* UNCOMMENT AND CHANGE THIS TO THE CURRENT YEAR
*YEAR 1970

This comment has been minimized.

@bhroam

bhroam Feb 6, 2019

Contributor

As I said in the forum post, use '#' to comment out the lines. The '#' always worked, but '*' was the correct comment character for this file to be compliant with UNICOS.

This comment has been minimized.

@agrawalravi90

agrawalravi90 Feb 7, 2019

Author Contributor

Ok, I'll make it '#'.

btw, I actually don't see your comment in the forum, could you please post it once more? Thanks.

This comment has been minimized.

@bhroam

bhroam Feb 8, 2019

Contributor

Heh, I typed out the message but forgot to click reply :)

# Verify that scheduler didn't log a message for out of date file
msg = "holidays;The holiday file is out of date; please update it."
self.scheduler.log_match(msg, starttime=ctime, existence=False,
max_attempts=5)

This comment has been minimized.

@bhroam

bhroam Feb 6, 2019

Contributor

One of the use cases of commenting out the holiday file is so the scheduler doesn't add prime status events to the calendar. Consider writing a test to check for that. To do it, you need to set log_filter to 2048. You then add a job that takes up all the resources without requesting walltime. You then add a second job that will get calendared. You look in the log that no policy change events happened.

@agrawalravi90

This comment has been minimized.

Copy link
Contributor Author

agrawalravi90 commented Feb 7, 2019

@bhroam I've addressed your comments, please provide further feedback. Thanks!

@bhroam

This comment has been minimized.

Copy link
Contributor

bhroam commented Feb 8, 2019

Codacy is complaining about an unused variable in your test.

@bhroam

This comment has been minimized.

Copy link
Contributor

bhroam commented Feb 9, 2019

My only comment is the one I mentioned above for the test without a YEAR line.

@agrawalravi90

This comment has been minimized.

Copy link
Contributor Author

agrawalravi90 commented Feb 11, 2019

@bhroam thanks for the review, I've addressed your comments. I also decided to add 2 more PTL tests to cover some corner cases. Please review the latest commit and provide feedback. Thanks.

@bhroam

This comment has been minimized.

Copy link
Contributor

bhroam commented Feb 12, 2019

My only comment is about the test with the YEAR line. I'm fine after we come to consensus about that test.

@bhroam

bhroam approved these changes Feb 12, 2019

Copy link
Contributor

bhroam left a comment

Looks good to me. I sign off.

@agrawalravi90

This comment has been minimized.

Copy link
Contributor Author

agrawalravi90 commented Feb 12, 2019

Thanks a lot for the thorough review Bhroam!
@arungrover could you please review this PR as well?

@arungrover
Copy link
Contributor

arungrover left a comment

Sorry about the delay in reviewing this change. It looks good to me. I just have a small comment if you want to consider it.

else if (!strcmp(tok, "none") || !strcmp(tok, "NONE")) {
conf.prime[d][pr].none = TRUE;
conf.prime[d][pr].none = FALSE;
} else if (!strcmp(tok, "none") || !strcmp(tok, "NONE")) {

This comment has been minimized.

@arungrover

arungrover Feb 13, 2019

Contributor

Why don't we use strcasecmp instead?

This comment has been minimized.

@agrawalravi90

agrawalravi90 Feb 13, 2019

Author Contributor

I'm not sure if values like NoNe or NOne are supposed to be allowed. Since you left it up to me, I'd like to keep the code as is for now. Thanks for being diligent :)

This comment has been minimized.

@mike0042

mike0042 Feb 13, 2019

Collaborator

+1 for strcasecmp()
+2 for strncasecmp()

@agrawalravi90 agrawalravi90 force-pushed the agrawalravi90:holidays branch from 23ff55e to 9e03b31 Feb 13, 2019

@agrawalravi90

This comment has been minimized.

Copy link
Contributor Author

agrawalravi90 commented Feb 13, 2019

@arungrover and @bhroam thanks for the sign-offs guys, I've rebase the branch, please merge it if possible. Thanks!

@bhroam bhroam merged commit 9e03b31 into PBSPro:master Feb 13, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@bhroam

This comment has been minimized.

Copy link
Contributor

bhroam commented Feb 13, 2019

Thanks @agrawalravi90. I've merged in the PR.

@agrawalravi90 agrawalravi90 deleted the agrawalravi90:holidays branch Feb 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment