Proxy reset with pagination and reconciliation requeue

api/v1alpha2/conditions.go

@@ -23,6 +23,7 @@ var conditionReasons = map[ConditionReason]conditionMeta{
 	ConditionReasonReconcileSucceeded: {Type: ConditionTypeReady, Status: metav1.ConditionTrue, Message: ConditionReasonReconcileSucceededMessage},
 	ConditionReasonReconcileFailed:    {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonReconcileFailedMessage},
 	ConditionReasonReconcileUnknown:   {Type: ConditionTypeReady, Status: metav1.ConditionUnknown, Message: ConditionReasonReconcileUnknownMessage},
+	ConditionReasonReconcileRequeued:  {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonReconcileRequeuedMessage},
 	ConditionReasonValidationFailed:   {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonValidationFailedMessage},
 	ConditionReasonOlderCRExists:      {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonOlderCRExistsMessage},
 
@@ -37,9 +38,10 @@ var conditionReasons = map[ConditionReason]conditionMeta{
 	ConditionReasonCRsReconcileSucceeded: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonCRsReconcileSucceededMessage},
 	ConditionReasonCRsReconcileFailed:    {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonCRsReconcileFailedMessage},
 
-	ConditionReasonProxySidecarRestartSucceeded:      {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionTrue, Message: ConditionReasonProxySidecarRestartSucceededMessage},
-	ConditionReasonProxySidecarRestartFailed:         {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarRestartFailedMessage},
-	ConditionReasonProxySidecarManualRestartRequired: {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarManualRestartRequiredMessage},
+	ConditionReasonProxySidecarRestartSucceeded:          {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionTrue, Message: ConditionReasonProxySidecarRestartSucceededMessage},
+	ConditionReasonProxySidecarRestartFailed:             {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarRestartFailedMessage},
+	ConditionReasonProxySidecarRestartPartiallySucceeded: {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarRestartPartiallySucceededMessage},
+	ConditionReasonProxySidecarManualRestartRequired:     {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarManualRestartRequiredMessage},
 
 	ConditionReasonIngressGatewayRestartSucceeded: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonIngressGatewayRestartSucceededMessage},
 	ConditionReasonIngressGatewayRestartFailed:    {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonIngressGatewayRestartFailedMessage},

api/v1alpha2/istio_types.go

@@ -40,6 +40,8 @@ const (
 	ConditionReasonReconcileSucceededMessage                 = "Reconciliation succeeded"
 	ConditionReasonReconcileUnknown          ConditionReason = "ReconcileUnknown"
 	ConditionReasonReconcileUnknownMessage                   = "Module readiness is unknown. Either a reconciliation is progressing, or failed previously. Check status of other conditions"
+	ConditionReasonReconcileRequeued         ConditionReason = "ReconcileRequeued"
+	ConditionReasonReconcileRequeuedMessage                  = "Proxy reset is still ongoing. Reconciliation requeued"
 	ConditionReasonReconcileFailed           ConditionReason = "ReconcileFailed"
 	ConditionReasonReconcileFailedMessage                    = "Reconciliation failed"
 	ConditionReasonValidationFailed          ConditionReason = "ValidationFailed"
@@ -70,12 +72,14 @@ const (
 	ConditionReasonCRsReconcileFailedMessage                    = "Custom resources reconciliation failed"
 
 	// proxy reset
-	ConditionReasonProxySidecarRestartSucceeded             ConditionReason = "ProxySidecarRestartSucceeded"
-	ConditionReasonProxySidecarRestartSucceededMessage                      = "Proxy sidecar restart succeeded"
-	ConditionReasonProxySidecarRestartFailed                ConditionReason = "ProxySidecarRestartFailed"
-	ConditionReasonProxySidecarRestartFailedMessage                         = "Proxy sidecar restart failed"
-	ConditionReasonProxySidecarManualRestartRequired        ConditionReason = "ProxySidecarManualRestartRequired"
-	ConditionReasonProxySidecarManualRestartRequiredMessage                 = "Proxy sidecar manual restart is required for some workloads"
+	ConditionReasonProxySidecarRestartSucceeded                 ConditionReason = "ProxySidecarRestartSucceeded"
+	ConditionReasonProxySidecarRestartSucceededMessage                          = "Proxy sidecar restart succeeded"
+	ConditionReasonProxySidecarRestartFailed                    ConditionReason = "ProxySidecarRestartFailed"
+	ConditionReasonProxySidecarRestartFailedMessage                             = "Proxy sidecar restart failed"
+	ConditionReasonProxySidecarRestartPartiallySucceeded        ConditionReason = "ProxySidecarRestartPartiallySucceeded"
+	ConditionReasonProxySidecarRestartPartiallySucceededMessage                 = "Proxy sidecar restart partially succeeded"
+	ConditionReasonProxySidecarManualRestartRequired            ConditionReason = "ProxySidecarManualRestartRequired"
+	ConditionReasonProxySidecarManualRestartRequiredMessage                     = "Proxy sidecar manual restart is required for some workloads"
 
 	// ingress gateway
 	ConditionReasonIngressGatewayRestartSucceeded        ConditionReason = "IngressGatewayRestartSucceeded"

controllers/istio_controller.go

@@ -146,14 +146,18 @@ func (r *IstioReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 		return r.requeueReconciliation(ctx, &istioCR, resourcesErr, operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonCRsReconcileFailed))
 	}
 
-	if err := restarter.Restart(ctx, &istioCR, r.restarters); err != nil {
+	err, requeue := restarter.Restart(ctx, &istioCR, r.restarters)
+	if err != nil {
 		// We don't want to use the requeueReconciliation function here, since there is condition handling in this function, and we
 		// need to clean this up, before we can use it here as conditions are already handled in the restarters.
 		statusUpdateErr := r.statusHandler.UpdateToError(ctx, &istioCR, err)
 		if statusUpdateErr != nil {
 			r.log.Error(statusUpdateErr, "Error during updating status to error")
 		}
 		return ctrl.Result{}, err
+	} else if requeue {
+		r.statusHandler.SetCondition(&istioCR, operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonReconcileRequeued))
+		return r.requeueReconciliationWithoutError(ctx, &istioCR)
 	}
 
 	return r.finishReconcile(ctx, &istioCR, istioImageVersion.Tag())
@@ -168,11 +172,19 @@ func (r *IstioReconciler) requeueReconciliation(ctx context.Context, istioCR *op
 	if statusUpdateErr != nil {
 		r.log.Error(statusUpdateErr, "Error during updating status to error")
 	}
-
 	r.log.Error(err, "Reconcile failed")
 	return ctrl.Result{}, err
 }
 
+func (r *IstioReconciler) requeueReconciliationWithoutError(ctx context.Context, istioCR *operatorv1alpha2.Istio) (ctrl.Result, error) {
+	statusUpdateErr := r.statusHandler.UpdateToProcessing(ctx, istioCR)
+	if statusUpdateErr != nil {
+		r.log.Error(statusUpdateErr, "Error during updating status to error")
+	}
+	r.log.Info("Reconcile requeued")
+	return ctrl.Result{Requeue: true, RequeueAfter: time.Minute * 1}, nil
+}
+
 // terminateReconciliation stops the reconciliation and does not requeue the request.
 func (r *IstioReconciler) terminateReconciliation(ctx context.Context, istioCR *operatorv1alpha2.Istio, err described_errors.DescribedError, reason operatorv1alpha2.ReasonWithMessage) (ctrl.Result, error) {
 	if err.ShouldSetCondition() {

controllers/istio_controller_test.go

@@ -322,7 +322,6 @@ var _ = Describe("Istio Controller", func() {
 
 			Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
 			Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(1))
-
 			Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonReconcileSucceeded)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionTrue))
@@ -413,7 +412,6 @@ var _ = Describe("Istio Controller", func() {
 
 			Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
 			Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(1))
-
 			Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonIstioInstallUninstallFailed)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse))
@@ -519,7 +517,6 @@ var _ = Describe("Istio Controller", func() {
 
 			Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
 			Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(1))
-
 			Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonReconcileSucceeded)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionTrue))
@@ -574,7 +571,6 @@ var _ = Describe("Istio Controller", func() {
 
 			Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
 			Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(1))
-
 			Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonOlderCRExists)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse))
@@ -618,7 +614,6 @@ var _ = Describe("Istio Controller", func() {
 
 			Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
 			Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(1))
-
 			Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonReconcileFailed)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse))
@@ -680,7 +675,6 @@ var _ = Describe("Istio Controller", func() {
 
 			Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
 			Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(1))
-
 			Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonValidationFailed)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse))
@@ -721,10 +715,9 @@ var _ = Describe("Istio Controller", func() {
 			updatedIstioCR := operatorv1alpha2.Istio{}
 			Expect(fakeClient.Get(context.Background(), client.ObjectKeyFromObject(istioCR), &updatedIstioCR)).Should(Succeed())
 
+			By("Verifying that Istio CR has Condition Ready with False")
 			Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
 			Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(1))
-
-			By("Verifying that Istio CR has Condition Ready with False")
 			Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonIstioInstallUninstallFailed)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse))
@@ -753,10 +746,9 @@ var _ = Describe("Istio Controller", func() {
 			updatedIstioCR = operatorv1alpha2.Istio{}
 			Expect(fakeClient.Get(context.Background(), client.ObjectKeyFromObject(istioCR), &updatedIstioCR)).Should(Succeed())
 
+			By("Verifying that the condition lastTransitionTime is also updated when only the reason has changed")
 			Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
 			Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(1))
-
-			By("Verifying that the condition lastTransitionTime is also updated when only the reason has changed")
 			Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonCRsReconcileFailed)))
 			Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse))
@@ -766,7 +758,6 @@ var _ = Describe("Istio Controller", func() {
 		})
 
 		Context("Restarters", func() {
-
 			It("should restart if reconciliations are successful", func() {
 				//given
 				istioCR := &operatorv1alpha2.Istio{
@@ -960,22 +951,77 @@ var _ = Describe("Istio Controller", func() {
 				Expect(updatedIstioCR.Status.State).Should(Equal(operatorv1alpha2.Warning))
 				Expect(updatedIstioCR.Status.Description).To(ContainSubstring("Restart Warning description"))
 			})
+
+			It("should requeue reconcile request when a restarter needs to finish work on the next reconcile", func() {
+				//given
+				istioCR := &operatorv1alpha2.Istio{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:              istioCrName,
+						Namespace:         testNamespace,
+						UID:               "1",
+						CreationTimestamp: metav1.Unix(1494505756, 0),
+						Finalizers: []string{
+							"istios.operator.kyma-project.io/istio-installation",
+						},
+					},
+				}
+
+				fakeClient := createFakeClient(istioCR)
+				ingressGatewayRestarter := &restarterMock{restarted: false}
+				proxySidecarsRestarter := &restarterMock{restarted: false, requeue: true}
+				sut := &IstioReconciler{
+					Client:                 fakeClient,
+					Scheme:                 getTestScheme(),
+					istioInstallation:      &istioInstallationReconciliationMock{},
+					istioResources:         &istioResourcesReconciliationMock{},
+					restarters:             []restarter.Restarter{ingressGatewayRestarter, proxySidecarsRestarter},
+					log:                    logr.Discard(),
+					statusHandler:          status.NewStatusHandler(fakeClient),
+					reconciliationInterval: testReconciliationInterval,
+				}
+
+				//when
+				reconcileResult, err := sut.Reconcile(context.Background(), reconcile.Request{NamespacedName: types.NamespacedName{Namespace: testNamespace, Name: istioCrName}})
+
+				//then
+				Expect(err).ToNot(HaveOccurred())
+				Expect(reconcileResult.Requeue).To(BeTrue())
+				Expect(reconcileResult.RequeueAfter).To(Equal(time.Minute * 1))
+
+				Expect(ingressGatewayRestarter.RestartCalled()).To(BeTrue())
+				Expect(proxySidecarsRestarter.RestartCalled()).To(BeTrue())
+
+				updatedIstioCR := operatorv1alpha2.Istio{}
+				err = fakeClient.Get(context.Background(), client.ObjectKeyFromObject(istioCR), &updatedIstioCR)
+				Expect(err).To(Not(HaveOccurred()))
+
+				Expect(updatedIstioCR.Status.State).To(Equal(operatorv1alpha2.Processing))
+
+				By("Verifying that Istio CR has Condition Ready status with Requeued reason")
+				Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
+				Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(1))
+				Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
+				Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonReconcileRequeued)))
+				Expect((*updatedIstioCR.Status.Conditions)[0].Message).To(Equal(operatorv1alpha2.ConditionReasonReconcileRequeuedMessage))
+				Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse))
+			})
 		})
 	})
 })
 
 type restarterMock struct {
 	err       described_errors.DescribedError
+	requeue   bool
 	restarted bool
 }
 
 func (i *restarterMock) RestartCalled() bool {
 	return i.restarted
 }
 
-func (i *restarterMock) Restart(_ context.Context, _ *operatorv1alpha2.Istio) described_errors.DescribedError {
+func (i *restarterMock) Restart(_ context.Context, _ *operatorv1alpha2.Istio) (described_errors.DescribedError, bool) {
 	i.restarted = true
-	return i.err
+	return i.err, i.requeue
 }
 
 type istioResourcesReconciliationMock struct {

docs/contributor/04-10-technical-design.md

@@ -103,17 +103,20 @@ Additionally, clear status reporting and the ability to provide detailed informa
 
 ### Restart Predicates
 
-The [SidecarRestarter](#sidecarsrestarter) and [IngressGatewayRestarter](#ingressgatewayrestarter)  components use Restart Predicates.
+The [SidecarRestarter](#sidecarsrestarter) and [IngressGatewayRestarter](#ingressgatewayrestarter) components use Restart Predicates.
 Depending on the implemented interfaces, a predicate can trigger a restart of Ingress Gateways, Proxy Sidecars, or both Ingress Gateways and Proxy Sidecars.
 
 For cases where it isn't trivial to check whether the configuration has been applied to the cluster state, Restart Predicates use a timestamp-based approach. For example, the `envoy_filter_allow_partial_referer` resource has the `istios.operator.kyma-project.io/updatedAt` annotation, which includes the timestamp of its last update.
 The predicate initiates a restart of the sidecar and Ingress Gateway if the target was created before this timestamp.
 
 ### SidecarsRestarter
 
-The SidecarsRestarter is responsible for keeping the proxy sidecars in the desired state. It restarts Pods that are part of the service mesh or
-that must be added to the service mesh.
-The Istio CR and [Istio Version](#istio-version) represent the desired state.
+The SidecarsRestarter is responsible for keeping the proxy sidecars in the desired state. It restarts Pods that are in the `Running` state, are part of the service mesh, and have the annotation `sidecar.istio.io/status`.
+The Istio CR and the [Istio version](#istio-version) represent the desired state. Pods are restarted in chunks with limits on the number that can be restarted in one reconciliation and the number that can be listed when requesting from the Kubernetes API Server. If the number of Pods that must be restarted exceeds the limits, it happens in the next reconciliation. In such a case, the reconciliation request is requeued with a 1-minute delay to allow time for the Kubernetes scheduler to restart the Deployments.
+
+During the proxy sidecars restarting phase, the Istio CR remains in the `Processing` state having the following status conditions:
+- The `Ready` condition is set to `false` with the reason `ReconcileRequeued`.
+- The `ProxySidecarRestartSucceeded` condition is set to `false` with the reason `ProxySidecarPartiallySucceeded`.
 
 This component covers the following restart triggers: