Refactor continuous release logic #222
Conversation
myname4423
force-pushed
the
fix
branch
2 times, most recently
from
e5d8110
to
70409aa
Compare
| @@ -10,4 +10,5 @@ type ReleaseManager interface { | |||
| doCanaryFinalising(c *RolloutContext) (bool, error) | |||
| fetchBatchRelease(ns, name string) (*v1beta1.BatchRelease, error) | |||
| removeBatchRelease(c *RolloutContext) (bool, error) | |||
| doCanaryReset(c *RolloutContext) (bool, error) | |||
There was a problem hiding this comment.
what is the difference bewteen doCanaryFinalising and doCanaryReset, plz add the missing description of all these methods, and the workflow to call these methods
myname4423
force-pushed
the
fix
branch
2 times, most recently
from
54cf862
to
9cd7411
Compare
| return done, err | ||
| } | ||
| if subStatus.LastUpdateTime != nil && subStatus.LastUpdateTime.Add(time.Second*time.Duration(3)).After(time.Now()) { |
There was a problem hiding this comment.
use time.Since() instead of After to compare two time for better readability.
plz change similar comparison in the patch
| } else { | ||
| klog.Infof("rollout(%s/%s) in step (%s), and success", c.Rollout.Namespace, c.Rollout.Name, subStatus.FinalisingStep) | ||
| subStatus.LastUpdateTime = &metav1.Time{Time: time.Now()} | ||
| subStatus.FinalisingStep = v1beta1.FinalisingStepTypeDeleteBR |
There was a problem hiding this comment.
just fallthrough to FinalisingStepTypeDeleteBR ?
| return done, err | ||
| } | ||
| if subStatus.LastUpdateTime != nil && subStatus.LastUpdateTime.Add(time.Second*time.Duration(3)).After(time.Now()) { |
There was a problem hiding this comment.
use defaultGracePeriodSeconds instead of const 3
plz change similar const in the patch
pkg/trafficrouting/manager.go
Outdated
| @@ -274,6 +274,119 @@ func (m *Manager) FinalisingTrafficRouting(c *TrafficRoutingContext, onlyRestore | |||
| return true, nil | |||
| } | |||
|
|
|||
| // RestoreGateway can be seen as route all traffic to stable service | |||
There was a problem hiding this comment.
plz add comment about the return variables in this file
pkg/trafficrouting/manager.go
Outdated
| cService := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: c.Namespace, Name: cServiceName}} | ||
| // end to end deployment, don't remove the canary service; | ||
| // because canary service is stable service | ||
| if !c.OnlyTrafficRouting && !c.DisableGenerateCanaryService { |
There was a problem hiding this comment.
plz double check the logic, is it And or Or
pkg/trafficrouting/manager.go
Outdated
| // RestoreGateway restore gateway resources without graceful time | ||
| // bool return value indicates whether the gateway resources has been successfully restored | ||
| // error return value indicates error encountered during restoring | ||
| func (m *Manager) RestoreGateway(c *TrafficRoutingContext) (bool, error) { |
There was a problem hiding this comment.
the first return variable seems redundant, consider remove it. actually this problem also applies to other func changed in this file
There was a problem hiding this comment.
it is mainly to maintain consistency with other traffic routing functions.
anyway, i removed the first return variable.
| return false, err | ||
| } | ||
| // usually, GracePeriodSeconds means duration to wait after an operation done | ||
| // we use defaultGracePeriodSeconds+1 because the count start from the time before the RestoreGateway called |
There was a problem hiding this comment.
shall we use trafficrouting.GracePeriodSeconds?
| // usually, GracePeriodSeconds means duration to wait after an operation done | ||
| // we use defaultGracePeriodSeconds+1 because the count start from the time before the RestoreGateway called | ||
| if subStatus.LastUpdateTime != nil && time.Since(subStatus.LastUpdateTime.Time) < time.Second*time.Duration(defaultGracePeriodSeconds+1) { | ||
| klog.Infof("rollout(%s/%s) in step (%s), and wait %d seconds", c.Rollout.Namespace, c.Rollout.Name, subStatus.FinalisingStep, defaultGracePeriodSeconds+1) |
There was a problem hiding this comment.
plz add //TODO to wait for gateway accept the new routing setting
myname4423
force-pushed
the
fix
branch
2 times, most recently
from
0b56327
to
c04b484
Compare
Signed-off-by: yunbo <[email protected]>
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zmberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Ⅰ. Describe what this PR does
Ⅱ. Does this pull request fix one issue?
#221
Ⅲ. Special notes for reviews