fix(trafficrouting): Fix downtime on initial deployment using Istio D…

…estinationRule Subsets. Fixes argoproj#2507 Signed-off-by: Wietse Muizelaar <wmuizelaar@bol.com>
wmuizelaar · Jul 8, 2024 · 582f087 · 582f087
1 parent 590206b
commit 582f087
Show file tree

Hide file tree

Showing 3 changed files with 148 additions and 44 deletions.
diff --git a/rollout/trafficrouting.go b/rollout/trafficrouting.go
@@ -41,9 +41,9 @@ func (c *Controller) NewTrafficRoutingReconciler(roCtx *rolloutContext) ([]traff
 	}
 	if rollout.Spec.Strategy.Canary.TrafficRouting.Istio != nil {
 		if c.IstioController.VirtualServiceInformer.HasSynced() {
-			trafficReconcilers = append(trafficReconcilers, istio.NewReconciler(rollout, c.IstioController.DynamicClientSet, c.recorder, c.IstioController.VirtualServiceLister, c.IstioController.DestinationRuleLister))
+			trafficReconcilers = append(trafficReconcilers, istio.NewReconciler(rollout, c.IstioController.DynamicClientSet, c.recorder, c.IstioController.VirtualServiceLister, c.IstioController.DestinationRuleLister, roCtx.allRSs))
 		} else {
-			trafficReconcilers = append(trafficReconcilers, istio.NewReconciler(rollout, c.IstioController.DynamicClientSet, c.recorder, nil, nil))
+			trafficReconcilers = append(trafficReconcilers, istio.NewReconciler(rollout, c.IstioController.DynamicClientSet, c.recorder, nil, nil, roCtx.allRSs))
 		}
 	}
 	if rollout.Spec.Strategy.Canary.TrafficRouting.Nginx != nil {

diff --git a/rollout/trafficrouting/istio/istio.go b/rollout/trafficrouting/istio/istio.go
@@ -6,10 +6,13 @@ import (
 	"fmt"
 	"strings"
 
+	replicasetutil "github.com/argoproj/argo-rollouts/utils/replicaset"
+	"github.com/mitchellh/mapstructure"
+	appsv1 "k8s.io/api/apps/v1"
+
 	"github.com/argoproj/argo-rollouts/rollout/trafficrouting"
 
 	jsonpatch "github.com/evanphx/json-patch/v5"
-	"github.com/mitchellh/mapstructure"
 	log "github.com/sirupsen/logrus"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -32,7 +35,7 @@ const Type = "Istio"
 const SpecHttpNotFound = "spec.http not found"
 
 // NewReconciler returns a reconciler struct that brings the Virtual Service into the desired state
-func NewReconciler(r *v1alpha1.Rollout, client dynamic.Interface, recorder record.EventRecorder, virtualServiceLister, destinationRuleLister dynamiclister.Lister) *Reconciler {
+func NewReconciler(r *v1alpha1.Rollout, client dynamic.Interface, recorder record.EventRecorder, virtualServiceLister, destinationRuleLister dynamiclister.Lister, replicaSets []*appsv1.ReplicaSet) *Reconciler {
 	return &Reconciler{
 		rollout: r,
 		log:     logutil.WithRollout(r),
@@ -41,6 +44,7 @@ func NewReconciler(r *v1alpha1.Rollout, client dynamic.Interface, recorder recor
 		recorder:              recorder,
 		virtualServiceLister:  virtualServiceLister,
 		destinationRuleLister: destinationRuleLister,
+		replicaSets:           replicaSets,
 	}
 }
 
@@ -52,6 +56,7 @@ type Reconciler struct {
 	recorder              record.EventRecorder
 	virtualServiceLister  dynamiclister.Lister
 	destinationRuleLister dynamiclister.Lister
+	replicaSets           []*appsv1.ReplicaSet
 }
 
 type virtualServicePatch struct {
@@ -315,6 +320,17 @@ func (r *Reconciler) reconcileVirtualService(obj *unstructured.Unstructured, vsv
 }
 
 func (r *Reconciler) UpdateHash(canaryHash, stableHash string, additionalDestinations ...v1alpha1.WeightDestination) error {
+	// We need to check if the replicasets are ready here as well if we didn't define any services in the rollout
+	// See: https://github.com/argoproj/argo-rollouts/issues/2507
+	if r.rollout.Spec.Strategy.Canary.CanaryService == "" && r.rollout.Spec.Strategy.Canary.StableService == "" {
+
+		for _, rs := range r.replicaSets {
+			if *rs.Spec.Replicas > 0 && !replicasetutil.IsReplicaSetAvailable(rs) {
+				return fmt.Errorf("delaying destination rule switch: ReplicaSet %s not fully available", rs.Name)
+			}
+		}
+	}
+
 	dRuleSpec := r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule
 	if dRuleSpec == nil {
 		return nil