State

api/v1alpha2/conditions.go

@@ -39,7 +39,7 @@ var conditionReasons = map[ConditionReason]conditionMeta{
 
 	ConditionReasonProxySidecarRestartSucceeded:          {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionTrue, Message: ConditionReasonProxySidecarRestartSucceededMessage},
 	ConditionReasonProxySidecarRestartFailed:             {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarRestartFailedMessage},
-	ConditionReasonProxySidecarRestartPartiallyCompleted: {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarRestartPartiallyCompletedMessage},
+	ConditionReasonProxySidecarRestartPartiallySucceeded: {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarRestartPartiallySucceededMessage},
 	ConditionReasonProxySidecarManualRestartRequired:     {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarManualRestartRequiredMessage},
 
 	ConditionReasonIngressGatewayRestartSucceeded: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonIngressGatewayRestartSucceededMessage},

api/v1alpha2/istio_types.go

@@ -74,8 +74,8 @@ const (
 	ConditionReasonProxySidecarRestartSucceededMessage                          = "Proxy sidecar restart succeeded"
 	ConditionReasonProxySidecarRestartFailed                    ConditionReason = "ProxySidecarRestartFailed"
 	ConditionReasonProxySidecarRestartFailedMessage                             = "Proxy sidecar restart failed"
-	ConditionReasonProxySidecarRestartPartiallyCompleted        ConditionReason = "ProxySidecarRestartPartiallyCompleted"
-	ConditionReasonProxySidecarRestartPartiallyCompletedMessage                 = "Proxy sidecar restart partially completed"
+	ConditionReasonProxySidecarRestartPartiallySucceeded        ConditionReason = "ProxySidecarRestartPartiallySucceeded"
+	ConditionReasonProxySidecarRestartPartiallySucceededMessage                 = "Proxy sidecar restart partially succeeded"
 	ConditionReasonProxySidecarManualRestartRequired            ConditionReason = "ProxySidecarManualRestartRequired"
 	ConditionReasonProxySidecarManualRestartRequiredMessage                     = "Proxy sidecar manual restart is required for some workloads"
 

internal/restarter/restart.go

@@ -10,6 +10,7 @@ import (
 // Restarter is an interface for restarting Istio components.
 // It uses predicates to evaluate if the restart is needed.
 // If the evaluation returns true, the restarter restarts the component.
+// Additional boolean return parameter indicates if the reconciliation should be requeued.
 type Restarter interface {
 	Restart(ctx context.Context, istioCR *operatorv1alpha2.Istio) (described_errors.DescribedError, bool)
 }

internal/restarter/restart_test.go

@@ -12,42 +12,80 @@ import (
 )
 
 var _ = Describe("Restart", func() {
-
-	It("should return nil if no restarters fail", func() {
+	It("Should return nil if no restarters fail", func() {
+		// given
 		r1 := &restarterMock{}
 		r2 := &restarterMock{}
 
-		Expect(restarter.Restart(context.Background(), &operatorv1alpha2.Istio{}, []restarter.Restarter{r1, r2})).Should(Succeed())
+		// when
+		err, requeue := restarter.Restart(context.Background(), &operatorv1alpha2.Istio{}, []restarter.Restarter{r1, r2})
 
+		// then
+		Expect(err).ToNot(HaveOccurred())
+		Expect(requeue).To(BeFalse())
 	})
 
-	It("should return nil if no restarters are provided", func() {
-		Expect(restarter.Restart(context.Background(), &operatorv1alpha2.Istio{}, nil)).Should(Succeed())
+	It("Should return nil if no restarters are provided", func() {
+		// when
+		err, requeue := restarter.Restart(context.Background(), &operatorv1alpha2.Istio{}, nil)
+
+		// then
+		Expect(err).ToNot(HaveOccurred())
+		Expect(requeue).To(BeFalse())
 	})
 
-	It("should invoke Restart", func() {
+	It("Should invoke Restart", func() {
+		// given
 		r := &restarterMock{}
 
-		Expect(restarter.Restart(context.Background(), &operatorv1alpha2.Istio{}, []restarter.Restarter{r})).Should(Succeed())
+		// when
+		err, requeue := restarter.Restart(context.Background(), &operatorv1alpha2.Istio{}, []restarter.Restarter{r})
 
+		// then
+		Expect(err).ToNot(HaveOccurred())
+		Expect(requeue).To(BeFalse())
 		Expect(r.RestartCalled()).Should(BeTrue())
 	})
 
-	It("should return error if Restart fails", func() {
+	It("Should return error if Restart fails", func() {
+		// given
 		r := &restarterMock{err: described_errors.NewDescribedError(errors.New("restart error"), "")}
 
-		Expect(restarter.Restart(context.Background(), &operatorv1alpha2.Istio{}, []restarter.Restarter{r})).Should(MatchError("restart error"))
+		// when
+		err, requeue := restarter.Restart(context.Background(), &operatorv1alpha2.Istio{}, []restarter.Restarter{r})
+
+		// then
+		Expect(err).Should(MatchError("restart error"))
+		Expect(requeue).To(BeFalse())
 	})
 
-	It("should return error with Error level when restarters return Error and Warning level errors", func() {
+	It("Should return error with Error level when restarters return Error and Warning level errors", func() {
+		// given
 		r1 := &restarterMock{err: described_errors.NewDescribedError(errors.New("restart error"), "")}
 		r2 := &restarterMock{err: described_errors.NewDescribedError(errors.New("restart warning"), "").SetWarning()}
 
+		// when
 		err, requeue := restarter.Restart(context.Background(), &operatorv1alpha2.Istio{}, []restarter.Restarter{r1, r2})
 
+		// then
 		Expect(err).Should(MatchError("restart error"))
 		Expect(requeue).To(BeFalse())
 	})
+
+	It("Should respect requeue condition if one of the restarters return it", func() {
+		// given
+		r1 := &restarterMock{requeue: false}
+		r2 := &restarterMock{requeue: true}
+
+		// when
+		err, requeue := restarter.Restart(context.Background(), &operatorv1alpha2.Istio{}, []restarter.Restarter{r1, r2})
+
+		// then
+		Expect(err).NotTo(HaveOccurred())
+		Expect(r1.RestartCalled()).Should(BeTrue())
+		Expect(r2.RestartCalled()).Should(BeTrue())
+		Expect(requeue).To(BeTrue())
+	})
 })
 
 type restarterMock struct {

internal/restarter/sidecars.go

@@ -118,6 +118,11 @@ func (s *SidecarsRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio
 		return warningErr, false
 	}
 
-	s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartSucceeded))
+	if !requeue {
+		s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartSucceeded))
+	} else {
+		s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartPartiallySucceeded))
+	}
+
 	return nil, requeue
 }

internal/restarter/sidecars_test.go

@@ -31,7 +31,7 @@ import (
 )
 
 var _ = Describe("SidecarsRestarter reconciliation", func() {
-	It("should fail proxy reset if Istio pods do not match target version", func() {
+	It("Should fail proxy reset if Istio pods do not match target version", func() {
 		// given
 		numTrustedProxies := 1
 		istioCr := operatorv1alpha2.Istio{ObjectMeta: metav1.ObjectMeta{
@@ -61,7 +61,7 @@ var _ = Describe("SidecarsRestarter reconciliation", func() {
 		Expect((*istioCr.Status.Conditions)[0].Message).To(Equal("Proxy sidecar restart failed"))
 	})
 
-	It("should succeed proxy reset even if more than 5 proxies could not be reset and will return a warning", func() {
+	It("Should succeed proxy reset even if more than 5 proxies could not be reset and will return a warning", func() {
 		// given
 		numTrustedProxies := 1
 		istioCr := operatorv1alpha2.Istio{ObjectMeta: metav1.ObjectMeta{
@@ -103,6 +103,7 @@ var _ = Describe("SidecarsRestarter reconciliation", func() {
 					Namespace: "ns6",
 				},
 			},
+			hasMorePods: true,
 		}
 		fakeClient := createFakeClient(&istioCr, istiod)
 		statusHandler := status.NewStatusHandler(fakeClient)
@@ -119,7 +120,7 @@ var _ = Describe("SidecarsRestarter reconciliation", func() {
 		Expect((*istioCr.Status.Conditions)[0].Message).To(ContainSubstring("The sidecars of the following workloads could not be restarted: ns1/name1, ns2/name2, ns3/name3, ns4/name4, ns5/name5 and 1 additional workload(s)"))
 	})
 
-	It("should succeed proxy reset even if less than 5 proxies could not be reset and will return a warning", func() {
+	It("Should succeed proxy reset even if less than 5 proxies could not be reset and will return a warning", func() {
 		// given
 		numTrustedProxies := 1
 		istioCr := operatorv1alpha2.Istio{ObjectMeta: metav1.ObjectMeta{
@@ -145,6 +146,7 @@ var _ = Describe("SidecarsRestarter reconciliation", func() {
 					Namespace: "ns2",
 				},
 			},
+			hasMorePods: true,
 		}
 		fakeClient := createFakeClient(&istioCr, istiod)
 		statusHandler := status.NewStatusHandler(fakeClient)
@@ -161,7 +163,7 @@ var _ = Describe("SidecarsRestarter reconciliation", func() {
 		Expect((*istioCr.Status.Conditions)[0].Message).To(Equal("The sidecars of the following workloads could not be restarted: ns1/name1, ns2/name2"))
 	})
 
-	It("should succeed proxy reset when there is no warning or errors", func() {
+	It("Should succeed proxy reset when there is no warning or errors", func() {
 		// given
 		numTrustedProxies := 1
 		istioCr := operatorv1alpha2.Istio{ObjectMeta: metav1.ObjectMeta{
@@ -191,6 +193,39 @@ var _ = Describe("SidecarsRestarter reconciliation", func() {
 		Expect((*istioCr.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonProxySidecarRestartSucceeded)))
 		Expect((*istioCr.Status.Conditions)[0].Message).To(Equal(operatorv1alpha2.ConditionReasonProxySidecarRestartSucceededMessage))
 	})
+
+	It("Should succeed proxy reset even if not all proxies are reset and requeue is required", func() {
+		// given
+		numTrustedProxies := 1
+		istioCr := operatorv1alpha2.Istio{ObjectMeta: metav1.ObjectMeta{
+			Name:            "default",
+			ResourceVersion: "1",
+			Annotations:     map[string]string{},
+		},
+			Spec: operatorv1alpha2.IstioSpec{
+				Config: operatorv1alpha2.Config{
+					NumTrustedProxies: &numTrustedProxies,
+				},
+			},
+		}
+		istiod := createPod("istiod", gatherer.IstioNamespace, "discovery", "1.16.1")
+		proxyResetter := &proxyResetterMock{
+			hasMorePods: true,
+		}
+		fakeClient := createFakeClient(&istioCr, istiod)
+		statusHandler := status.NewStatusHandler(fakeClient)
+		sidecarsRestarter := restarter.NewSidecarsRestarter(logr.Discard(), createFakeClient(&istioCr, istiod),
+			&MergerMock{"1.16.1-distroless"}, proxyResetter, statusHandler)
+
+		// when
+		err, requeue := sidecarsRestarter.Restart(context.Background(), &istioCr)
+
+		// then
+		Expect(err).ToNot(HaveOccurred())
+		Expect(requeue).To(BeTrue())
+		Expect((*istioCr.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonProxySidecarRestartPartiallySucceeded)))
+		Expect((*istioCr.Status.Conditions)[0].Message).To(Equal(operatorv1alpha2.ConditionReasonProxySidecarRestartPartiallySucceededMessage))
+	})
 })
 
 func createFakeClient(objects ...client.Object) client.Client {

pkg/lib/sidecars/pods/filter_test.go

@@ -8,20 +8,29 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-var _ = Describe("Evaluate restart", func() {
-	It("should should return false when pod has custom image annotation", func() {
+var _ = Describe("RequiresProxyRestart", func() {
+	It("Should should return false when pod has custom image annotation", func() {
+		// given
 		pod := createPodWithProxySidecar("test-pod", "test-namespace", "1.21.0", map[string]string{"sidecar.istio.io/proxyImage": "istio/proxyv2:1.21.0"})
-
 		predicate := pods.NewRestartProxyPredicate(pods.NewSidecarImage("istio", "1.22.0"), v1.ResourceRequirements{})
-		Expect(predicate.RequiresProxyRestart(pod)).To(BeFalse())
 
+		// when
+		shouldRestart := predicate.RequiresProxyRestart(pod)
+
+		// then
+		Expect(shouldRestart).To(BeFalse())
 	})
 
-	It("should should return true when pod does not have custom image annotation", func() {
+	It("Should should return true when pod does not have custom image annotation", func() {
+		// given
 		pod := createPodWithProxySidecar("test-pod", "test-namespace", "1.21.0", map[string]string{})
-
 		predicate := pods.NewRestartProxyPredicate(pods.NewSidecarImage("istio", "1.22.0"), v1.ResourceRequirements{})
-		Expect(predicate.RequiresProxyRestart(pod)).To(BeTrue())
+
+		// when
+		shouldRestart := predicate.RequiresProxyRestart(pod)
+
+		// then
+		Expect(shouldRestart).To(BeTrue())
 	})
 })
 

pkg/lib/sidecars/pods/get.go

@@ -13,8 +13,6 @@ import (
 )
 
 const (
-	maxPodsToRestartAtOnce = 10
-
 	istioSidecarContainerName string = "istio-proxy"
 )
 
@@ -38,13 +36,13 @@ func (r SidecarImage) matchesImageIn(container v1.Container) bool {
 	return container.Image == r.String()
 }
 
-func listRunningPods(ctx context.Context, c client.Client, continueToken string) (*v1.PodList, error) {
+func listRunningPods(ctx context.Context, c client.Client, limit int, continueToken string) (*v1.PodList, error) {
 	podList := &v1.PodList{}
 
 	err := retry.RetryOnError(retry.DefaultRetry, func() error {
 		listOps := []client.ListOption{
 			client.MatchingFieldsSelector{Selector: fields.OneTermEqualSelector("status.phase", string(v1.PodRunning))},
-			client.Limit(maxPodsToRestartAtOnce),
+			client.Limit(limit),
 		}
 		if continueToken != "" {
 			listOps = append(listOps, client.Continue(continueToken))
@@ -55,8 +53,8 @@ func listRunningPods(ctx context.Context, c client.Client, continueToken string)
 	return podList, err
 }
 
-func getSidecarPods(ctx context.Context, c client.Client, logger *logr.Logger, continueToken string) (*v1.PodList, error) {
-	podList, err := listRunningPods(ctx, c, continueToken)
+func getSidecarPods(ctx context.Context, c client.Client, logger *logr.Logger, limit int, continueToken string) (*v1.PodList, error) {
+	podList, err := listRunningPods(ctx, c, limit, continueToken)
 	if err != nil {
 		return nil, err
 	}
@@ -77,13 +75,13 @@ func getSidecarPods(ctx context.Context, c client.Client, logger *logr.Logger, c
 	return podsWithSidecar, nil
 }
 
-func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) (*v1.PodList, error) {
+func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, limit int, logger *logr.Logger) (*v1.PodList, error) {
 	//Add predicate for image version and resources configuration
 	predicates = append(predicates, NewRestartProxyPredicate(expectedImage, expectedResources))
 
 	podsToRestart := &v1.PodList{}
 	for while := true; while; {
-		podsWithSidecar, err := getSidecarPods(ctx, c, logger, podsToRestart.Continue)
+		podsWithSidecar, err := getSidecarPods(ctx, c, logger, limit, podsToRestart.Continue)
 		if err != nil {
 			return nil, err
 		}
@@ -98,7 +96,7 @@ func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage Sideca
 		}
 
 		podsToRestart.Continue = podsWithSidecar.Continue
-		while = len(podsToRestart.Items) < maxPodsToRestartAtOnce && podsToRestart.Continue != ""
+		while = len(podsToRestart.Items) < limit && podsToRestart.Continue != ""
 	}
 
 	if len(podsToRestart.Items) > 0 {