[BUG] schedule defined by time_elements is rescheduled every second when disabled (job or all jobs)

[lufik]

Description
salt reschedule every second all disabled jobs which are defined using time_elements.

https://github.com/saltstack/salt/blob/master/salt/utils/schedule.py (eval function - runs every second):
def _handle_time_elements(data) - define _seconds every eval invocation
https://github.com/saltstack/salt/blob/master/salt/utils/schedule.py#L1553

also _skipped is set for every disabled job
https://github.com/saltstack/salt/blob/master/salt/utils/schedule.py#L1708

and this leads in final section to reschedule the scheduled job
https://github.com/saltstack/salt/blob/master/salt/utils/schedule.py#L1766

Documentation says nothing about this.
Current behavior makes us troubles when we use disabling jobs that some of them never runs.

Setup
salt version 3000 and higher including master (I haven't looked for exact change/commit for this change)

Steps to Reproduce the behavior
define salt schedule job e.g.:

test:
  enabled: true
  function: test.ping
  maxrunning: 1
  minutes: 5
  run_on_start: false

after that run:
$ salt-call schedule.show_next_fire_time test
$ salt-call schedule.show_next_fire_time test
$ salt-call schedule.disable
$ salt-call schedule.show_next_fire_time test
$ salt-call schedule.show_next_fire_time test

every schedule.show_next_fire_time call returns different fire time when disabled

Expected behavior
Scheduled job gets rescheduled only at time when it should run (or as close after due to disabling as possible). Not all the time while it's disabled.

Versions Report
master (at least since 3000)

[garethgreenaway]

@lufik Thanks for the report. To be clear, is your concern that when a scheduled job is disabled the value returned from schedule.show_next_fire_time is still updating? If that is the concern then this is done by design. The schedule eval function does run every second as that is the smallest increment value that you can have a scheduled job run if you were using the seconds option, if the job is set to disabled then it job does not run, but the final step (as you pointed out) is to set the value for the next time the job would run. This is done in the event that the job becomes enabled again between that point and the next time the schedule is evaluated. When using the time elements such as seconds, minutes, etc. the time when the job will run is based on when the minion (or master) is started up and the scheduler first runs. So we need to keep track of when the next time a particular job should run, that is the value being returned by schedule.show_next_fire_time.

[lufik]

@garethgreenaway The problem is that if I define e.g. minutes: 55 for schedule job and disable schedule let say every 50 minutes (less then 55) for 5 seconds the scheduled job will never run. Because every second in disable will reschedule it for 55 minutes from now.
TBH I understand disabled/enabled and stop/start differently for time scheduling.
It was different in salt version 2018.

[garethgreenaway]

I'll need to confirm and find the point in the code where this is all happening but if this is the current behavior (and it sounds like it is) then this is definitely incorrect and a bug. Thanks for elaborating.

[lufik]

@garethgreenaway I'm hope I showed all the important parts in the description (links to git with exact lines in master branch).
I can also help with preparing PR for it. But I need to know what is expected behavior. Like just ignore run if during disable state or run the job as soon as possible after enable (how to take care about too much jobs running at same time), etc.
Thank you for taking care of this.

[garethgreenaway]

@lufik The description was perfect, I was able to put together a quick unit test and see the problem happening. The issue was that the _next_fire_time was being updated for disabled jobs every time the schedule's eval function was running, when it should have only been updating if/when the _next_fire_time was reached. If you're able to, could you test out the changes in the above PR and ensure that it fixes the issue for you? Thanks!

[lufik]

@garethgreenaway partially. It doesn't change _next_fire_time every second. But during salt-call schedule.disable and salt-call schedule.enable the data["_next_fire_time"] is empty line: https://github.com/saltstack/salt/blob/master/salt/utils/schedule.py#L988
That means salt-call schedule.disable and salt-call schedule.enable still changes the output of salt-call schedule.show_next_fire_time of the test job (same for disable_job and enable_job).

BTW to the proposed patch I think if data["_next_fire_time"] <= now: would be less error prone (e.g. stop and cont signal for process).

[lufik]

I'm not able to find why with disabling/enabling scheduled job the data["_next_fire_time"] becomes empty.

[garethgreenaway]

@lufik I'll update the PR with the suggestion for comparing if_next_fire_time is less than or equal to now.
I haven't been able to reproduce the above scenario of next_fire_time being empty, can you details all the steps you're taking that is leading to that? Thanks!