feat: Istio implementation #341
Conversation
Codecov Report
@@ Coverage Diff @@
## master #341 +/- ##
==========================================
+ Coverage 83.95% 84.01% +0.05%
==========================================
Files 67 69 +2
Lines 6215 6350 +135
==========================================
+ Hits 5218 5335 +117
- Misses 711 721 +10
- Partials 286 294 +8
Continue to review full report at Codecov.
|
dthomson25
changed the title
dthomson25
changed the title
| @@ -37,6 +37,10 @@ func DesiredReplicaCountsForCanary(rollout *v1alpha1.Rollout, newRS, stableRS *a | |||
| desiredNewRSReplicaCount = rolloutSpecReplica | |||
| desiredStableRSReplicaCount = 0 | |||
| } | |||
| if rollout.Spec.Strategy.Canary.TrafficRouting != nil { | |||
| desiredStableRSReplicaCount = rolloutSpecReplica | |||
There was a problem hiding this comment.
This probably needs some explanation/comment here that unlike the replicaset based weighted canary, a service mesh / ingress based canary leaves the stable as 100% scaled until the rollout completes.
rollout/trafficrouting.go
Outdated
|
|
||
| // TrafficRoutingReconciler common function across all TrafficRouting implementation | ||
| type TrafficRoutingReconciler interface { | ||
| Reconcile() error |
There was a problem hiding this comment.
The TrafficRoutingReconciler interface containing a Reconcile() method which takes no args (since the args for reconcilation were supplied as part of the constructor) is a little odd. I would expect the interface to be flipped, more like:
type TrafficRoutingReconciler interface {
Reconcile(roCtx rolloutContext, desiredWeight int32) error
}This makes the interface more extensible if there are additional methods that need to be added which take different set of args.
There was a problem hiding this comment.
This is fine too:
type TrafficRoutingReconciler interface {
Reconcile(desiredWeight int32) error
}
rollout/trafficrouting.go
Outdated
| _, index := replicasetutil.GetCurrentCanaryStep(rollout) | ||
| desiredWeight := int32(0) | ||
| if !atDesiredReplicaCount { | ||
| //Use the previous weight since the new RS is not ready for neww weight |
There was a problem hiding this comment.
- Typo
neww - space after
// - grammar
is not ready for a new
| olderRS := roCtx.OlderRSs() | ||
|
|
||
| atDesiredReplicaCount := replicasetutil.AtDesiredReplicaCountsForCanary(rollout, newRS, stableRS, olderRS) | ||
| _, index := replicasetutil.GetCurrentCanaryStep(rollout) |
There was a problem hiding this comment.
Can index be nil? If yes, it's possible to get nil pointer dereference below.
rollout/trafficrouting.go
Outdated
| } | ||
| // This if statement prevents the desiredWeight being set to 100 | ||
| // when the rollout has progressed through all the steps. The rollout | ||
| // should send all traffic to the stable service by using a weight of 0 |
There was a problem hiding this comment.
I believe this comment is actually referring to the following else if on line 51, but is included in the previous if block, which confused me. I think it should be moved under else if *index != int32(len(rollout.Spec.Strategy.Canary.Steps)), and explained what it means if we are there.
| } | ||
| patches = append(patches, patch) | ||
| } | ||
| if host == stableSvc && weight != int64(100-desiredWeight) { |
There was a problem hiding this comment.
unnecessary conversion (from unconvert)
| patch := virtualServicePatch{ | ||
| routeIndex: i, | ||
| destinationIndex: j, | ||
| weight: int64(100 - desiredWeight), |
There was a problem hiding this comment.
unnecessary conversion (from unconvert)
dthomson25
force-pushed
the
istio
branch
2 times, most recently
from
325aff9 to
c6d6591
Compare
|
addresses #40 |
Integrates Istio with Rollouts based on https://github.com/argoproj/argo-rollouts/blob/master/docs/features/traffic-management/istio.md.
Future work: