Skip to content
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

[fluorine] Fixes to scheduler for jobs with seconds, minutes, etc. #49104

Conversation

Projects
None yet
4 participants
@garethgreenaway
Copy link
Member

commented Aug 14, 2018

What does this PR do?

Do not log about various things like jid_include and max_running unless the job is set to run. If the job contains _seconds, eg. it's using seconds, minutes, hours, etc. then only set the _next_fire_time if the job actually runs. Adding more tests to test simple scenarios when seconds, minutes, hours and days to ensure we get multiple scheduled job runs when they're expected to run.

What issues does this PR fix or reference?

#49082
#49093

Tests written?

Yes

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

Do not log about various things like jid_include and max_running unle…
…ss the job is set to run. If the job contains _seconds, eg. it's using seconds, minutes, hours, etc. then only set the _next_fire_time if the job actually runs. Adding more tests to test simple scenarios when seconds, minutes, hours and days to ensure we get multiple scheduled job runs when they're expected to run.

@s0undt3ch s0undt3ch requested review from s0undt3ch and pizzapanther Aug 14, 2018

@garethgreenaway

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

I'll add an integration test for calculating futures this morning.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2018

Please don't merge this until we have signoffs from both @s0undt3ch and @pizzapanther

@s0undt3ch
Copy link
Member

left a comment

I defer my review to @pizzapanther

@pizzapanther
Copy link
Contributor

left a comment

A couple findings. First in enterprise we use salt as a python import when calculating schedules and there looks to be a loading issue:

File "/root/.local/share/virtualenvs/raas-xhpEmuIf/lib/python3.6/site-packages/salt/utils/schedule.py", line 101, in __new__
    utils=utils)
  File "/root/.local/share/virtualenvs/raas-xhpEmuIf/lib/python3.6/site-packages/salt/utils/schedule.py", line 132, in __singleton_init__
    self.utils = utils or salt.loader.utils(opts)
  File "/root/.local/share/virtualenvs/raas-xhpEmuIf/lib/python3.6/site-packages/salt/loader.py", line 339, in utils
    _module_dirs(opts, 'utils', ext_type_dirs='utils_dirs'),
  File "/root/.local/share/virtualenvs/raas-xhpEmuIf/lib/python3.6/site-packages/salt/loader.py", line 147, in _module_dirs
    ext_types = os.path.join(opts['extension_modules'], ext_type)
KeyError: 'extension_modules'

My guess is you are loading extension_modules when salt gets executed as a CLI command and not doing it when used as a python import. So should we file a separate bug for that?

Anyways I couldn't get it to work right with out a little hacking. But once I worked around the loader and got it calculating schedules and there are big differences.

I'm thinking maybe we submit some of the enterprise code for a unit test on the scheduler so you can see how we use it. I put some work arounds in to use it for our use case. Maybe we need to use it more "properly" or also this change might have broken the calculation too much.

@garethgreenaway

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

@pizzapanther For the extension_modules issue, a quick fix would be to add that to the extension_modules to the opts that you're passing to salt.utils.schedule.Schedule.

@pizzapanther

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2018

adding extension_modules worked, thanks! Maybe it should default to an empty string though?

garethgreenaway added some commits Aug 15, 2018

If the scheduler is not running in standalone mode, only set the _nex…
…t_fire_time (if we're using time elements) if the job successfully runs.
Making seconds more accurate by chopping off the microseconds. If dat…
…a contains '_seconds' and we're running in standalone, always set the _next_fire_time. If the job was skipped, set the _next_fire_time, if there are _seconds, same if the run successfully ran.
@pizzapanther
Copy link
Contributor

left a comment

OK this works for me now.

@garethgreenaway

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2018

Awesome! I have a couple more tests I want to add before merging just to make sure everything is good. Thanks @pizzapanther.

Bulking up the seconds, minutes, hours and days eval tests to ensure …
…_next_run_time is correct. Adding a run_seconds_skip to ensure skipping is working with time elements and the _next_fire_time is populated.
@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Aug 17, 2018

@garethgreenaway Are you ready to go here?

@garethgreenaway

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2018

@cachedout yup! Good to go on my end.

@cachedout cachedout merged commit bfa7775 into saltstack:fluorine Aug 17, 2018

4 of 8 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details

garethgreenaway added a commit to garethgreenaway/salt that referenced this pull request Oct 18, 2018

cachedout pushed a commit that referenced this pull request Oct 19, 2018

Mike Place
Merge pull request #50115 from garethgreenaway/apply_49104_to_2018_3
[2018.3] Apply scheduler fixes and tests from #49104 to 2018.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.