rollout: Roll forward only if enough time passed #57
Conversation
| // TODO (gvso): Refactor this and other methods to return only a traffic object. | ||
| // Then, rename this to trafficBasedOnDiagnosis. | ||
| // Also, move those traffic-config-related methods to traffic.go file. |
There was a problem hiding this comment.
Instead of updating the service in PrepareRollForward and PrepareRollForward, which are deep in the call chain, we can just return the traffic config and make the update in, yeah, UpdateService.
We need to change the first two method's name and move them (an their helpers) to a new file since this one is getting very big.
Any thoughts?
There was a problem hiding this comment.
I think the TODO above is a good idea. Though I'm fine with it as it is right now.
pkg/rollout/rollout.go
Outdated
| StableRevisionAnnotation = "rollout.cloud.run/stableRevision" | ||
| CandidateRevisionAnnotation = "rollout.cloud.run/candidateRevision" | ||
| LastFailedCandidateRevisionAnnotation = "rollout.cloud.run/lastFailedCandidateRevision" | ||
| LastRollForwardAnnotation = "rollout.cloud.run/lastRollForward" |
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to update service after diagnosis") | ||
| } | ||
| if svc == nil { |
There was a problem hiding this comment.
add a comment inside this if about when this happens
pkg/rollout/rollout.go
Outdated
| if r.shouldRollback { | ||
| svc.Metadata.Annotations[LastFailedCandidateRevisionAnnotation] = candidate | ||
| delete(svc.Metadata.Annotations, LastRollForwardAnnotation) |
There was a problem hiding this comment.
I'd keep it (for the records)
pkg/rollout/rollout.go
Outdated
| return svc | ||
| } | ||
|
|
||
| svc.Metadata.Annotations[StableRevisionAnnotation] = stable | ||
| svc.Metadata.Annotations[CandidateRevisionAnnotation] = candidate | ||
| svc.Metadata.Annotations[LastRollForwardAnnotation] = r.time.Now().Format(time.RFC3339) |
There was a problem hiding this comment.
init once now := r.time.Now().Format(time.RFC3339)
pkg/rollout/rollout.go
Outdated
| // shouldRollForward determines if enough time has passed since last roll | ||
| // forward. | ||
| func (r *Rollout) shouldRollForward(lastRollForwardStr string, timeBetweenRollForwards int) (bool, error) { | ||
| lastRollForward, err := time.Parse(time.RFC3339, lastRollForwardStr) |
There was a problem hiding this comment.
if annotation is empty, return a different error saying X annotation is missing
need to think about the case where it bricks here forever.
There was a problem hiding this comment.
I added a TODO for this
pkg/rollout/rollout.go
Outdated
| @@ -318,6 +364,23 @@ func (r *Rollout) diagnoseCandidate(candidate string, healthCriteria []config.Me | |||
| return d, errors.Wrap(err, "failed to diagnose candidate's health") | |||
| } | |||
|
|
|||
| // shouldRollForward determines if enough time has passed since last roll | |||
| // forward. | |||
| func (r *Rollout) shouldRollForward(lastRollForwardStr string, timeBetweenRollForwards int) (bool, error) { | |||
There was a problem hiding this comment.
shouldRollForward → isTimeElapsed
roll forward → rollout
There was a problem hiding this comment.
I renamed it to hasEnoughTimeElapsed. Is it fine?
pkg/rollout/rollout.go
Outdated
| r.log.Debugf("last roll forward: %v", lastRollForward) | ||
| r.log.Debugf("current time: %v", currentTime) |
There was a problem hiding this comment.
lets move the log to the call site
pkg/rollout/rollout_test.go
Outdated
| annotations: map[string]string{ | ||
| rollout.LastRollForwardAnnotation: clockMock.Now().Format(time.RFC3339), | ||
| }, |
There was a problem hiding this comment.
create inline function
mkAnnotations := func(t time.Time, offset Duration) map[string]string {
}
inside the test function
There was a problem hiding this comment.
Since it can be also used in combination with other annotations, I created a utility function to generate the value instead.
pkg/rollout/rollout.go
Outdated
| return svc | ||
| } | ||
|
|
||
| svc.Metadata.Annotations[StableRevisionAnnotation] = stable | ||
| svc.Metadata.Annotations[CandidateRevisionAnnotation] = candidate | ||
| svc.Metadata.Annotations[LastRolloutAnnotation] = now |
There was a problem hiding this comment.
it seems like you always do this (in both cases). so why not just set it outside the if (e.g. right after now:=... ?
This adds a new annotation containing the last time the traffic to the candidate was increased. Using the configuration for minimum time between roll forwards, it decides if it can continue rolling the candidate forward. Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>
Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>
Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>
Signed-off-by: Getulio Valentin Sánchez <gvso@google.com>
|
Don't have time today, and this looks good at a glance :) |
This adds a new annotation containing the last time the traffic to the candidate was increased. Using the configuration for minimum time between roll forwards, it decides if it can continue rolling the candidate forward.