Fix requeue for git commit tracking #763
Conversation
| // Reconcile every resyncFreq to check for new commits to the branch. | ||
| pollFreq := resyncFreq(instance) | ||
| log.Info("Commit hash unchanged. Will poll for new commits.", "pollFrequency", pollFreq) | ||
| requeueAfter = min(requeueAfter, pollFreq) |
There was a problem hiding this comment.
requeueAfter defaults to 0 so this min was always disabling the requeue.
There was a problem hiding this comment.
Good find, that's totally the problem.
Regarding the duplication of code here, I think my rationale is that the resync frequency is today unduly coupled between branch tracking and the periodic resync, ie. the resyncFrequencySeconds. I anticipate new git-specific fields for branch tracking, such that one could say "check the branch every minute, and do a re-sync (for drift detection) every hour".
There was a problem hiding this comment.
I took the liberty of reverting the optimization, for clarity.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #763 +/- ##
==========================================
- Coverage 50.75% 50.71% -0.04%
==========================================
Files 30 30
Lines 4175 4180 +5
==========================================
+ Hits 2119 2120 +1
- Misses 1872 1876 +4
Partials 184 184 ☔ View full report in Codecov by Sentry. |
| // Reconcile every resyncFreq to check for new commits to the branch. | ||
| pollFreq := resyncFreq(instance) | ||
| log.Info("Commit hash unchanged. Will poll for new commits.", "pollFrequency", pollFreq) | ||
| requeueAfter = min(requeueAfter, pollFreq) |
There was a problem hiding this comment.
Good find, that's totally the problem.
Regarding the duplication of code here, I think my rationale is that the resync frequency is today unduly coupled between branch tracking and the periodic resync, ie. the resyncFrequencySeconds. I anticipate new git-specific fields for branch tracking, such that one could say "check the branch every minute, and do a re-sync (for drift detection) every hour".
| if instance.Status.LastUpdate.State == shared.FailedStackStateMessage { | ||
| requeueAfter = max(1*time.Second, time.Until(instance.Status.LastUpdate.LastResyncTime.Add(cooldown(instance)))) | ||
| } | ||
| if sess.stack.ContinueResyncOnCommitMatch { | ||
| // Schedule another poll if ContinueResyncOnCommitMatch is set, for drift detection or to maintain dynamic resources. | ||
| if instance.Status.LastUpdate.State == shared.SucceededStackStateMessage && sess.stack.ContinueResyncOnCommitMatch { |
There was a problem hiding this comment.
The "ContinueResyncOnCommitMatch" polling loop should activate only when the last update was successful. Otherwise, the backoff loop should prevail (and is likely a shorter interval).
Meanwhile the source tracking loop should be active regardless of the last update. And it should adjust the requeueAfter value downward only.
Fixes a bug where we were accidentally cancelling a requeue when we intended to poll again after a minute.
Fixes #762.