From 26dcc925432b07e5b43c788b9d5c3e82e8509510 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Mon, 8 Jul 2024 18:18:58 +0200 Subject: [PATCH 01/22] Initial --- api/v1alpha2/conditions.go | 7 ++-- api/v1alpha2/istio_types.go | 14 ++++--- controllers/istio_controller.go | 5 ++- controllers/istio_controller_test.go | 5 ++- internal/filter/predicate.go | 7 +--- internal/restarter/ingress_gateway.go | 8 ++-- internal/restarter/ingress_gateway_test.go | 18 ++++++--- internal/restarter/restart.go | 11 +++-- internal/restarter/restart_test.go | 9 +++-- internal/restarter/sidecars.go | 20 ++++----- internal/restarter/sidecars_test.go | 19 +++++---- pkg/lib/sidecars/pods/filter.go | 18 +-------- pkg/lib/sidecars/pods/filter_test.go | 9 +---- pkg/lib/sidecars/pods/get.go | 47 +++++++++++++--------- pkg/lib/sidecars/proxy.go | 22 +++++++--- pkg/lib/sidecars/test/cases.go | 6 ++- pkg/lib/sidecars/test/test_client.go | 1 + 17 files changed, 125 insertions(+), 101 deletions(-) diff --git a/api/v1alpha2/conditions.go b/api/v1alpha2/conditions.go index 3b3d58f50..e2fcb84f3 100644 --- a/api/v1alpha2/conditions.go +++ b/api/v1alpha2/conditions.go @@ -37,9 +37,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}, + ConditionReasonProxySidecarRestartPartiallyCompleted: {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarRestartPartiallyCompletedMessage}, + ConditionReasonProxySidecarManualRestartRequired: {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarManualRestartRequiredMessage}, ConditionReasonIngressGatewayRestartSucceeded: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonIngressGatewayRestartSucceededMessage}, ConditionReasonIngressGatewayRestartFailed: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonIngressGatewayRestartFailedMessage}, diff --git a/api/v1alpha2/istio_types.go b/api/v1alpha2/istio_types.go index 38e736541..18a1f3ee9 100644 --- a/api/v1alpha2/istio_types.go +++ b/api/v1alpha2/istio_types.go @@ -70,12 +70,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" + ConditionReasonProxySidecarRestartPartiallyCompleted ConditionReason = "ProxySidecarRestartPartiallyCompleted" + ConditionReasonProxySidecarRestartPartiallyCompletedMessage = "Proxy sidecar restart partially completed" + ConditionReasonProxySidecarManualRestartRequired ConditionReason = "ProxySidecarManualRestartRequired" + ConditionReasonProxySidecarManualRestartRequiredMessage = "Proxy sidecar manual restart is required for some workloads" // ingress gateway ConditionReasonIngressGatewayRestartSucceeded ConditionReason = "IngressGatewayRestartSucceeded" diff --git a/controllers/istio_controller.go b/controllers/istio_controller.go index e9417e5c5..f331ecf63 100644 --- a/controllers/istio_controller.go +++ b/controllers/istio_controller.go @@ -146,7 +146,8 @@ 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) @@ -154,6 +155,8 @@ func (r *IstioReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl r.log.Error(statusUpdateErr, "Error during updating status to error") } return ctrl.Result{}, err + } else if requeue { + return r.requeueReconciliation(ctx, &istioCR, installationErr, operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonIstioInstallUninstallFailed)) } return r.finishReconcile(ctx, &istioCR, istioImageVersion.Tag()) diff --git a/controllers/istio_controller_test.go b/controllers/istio_controller_test.go index 0f6e53aee..04f7a2d46 100644 --- a/controllers/istio_controller_test.go +++ b/controllers/istio_controller_test.go @@ -966,6 +966,7 @@ var _ = Describe("Istio Controller", func() { type restarterMock struct { err described_errors.DescribedError + requeue bool restarted bool } @@ -973,9 +974,9 @@ 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 { diff --git a/internal/filter/predicate.go b/internal/filter/predicate.go index 0186c8272..82dbf9ea7 100644 --- a/internal/filter/predicate.go +++ b/internal/filter/predicate.go @@ -2,21 +2,18 @@ package filter import ( "context" + v1 "k8s.io/api/core/v1" ) type SidecarProxyPredicate interface { - NewProxyRestartEvaluator(context.Context) (ProxyRestartEvaluator, error) + RequiresProxyRestart(v1.Pod) bool } type IngressGatewayPredicate interface { NewIngressGatewayEvaluator(context.Context) (IngressGatewayRestartEvaluator, error) } -type ProxyRestartEvaluator interface { - RequiresProxyRestart(v1.Pod) bool -} - type IngressGatewayRestartEvaluator interface { // The RequiresIngressGatewayRestart method does not evaluate the restart per pod, //as there is only one Ingress Gateway deployment under Istio module control. diff --git a/internal/restarter/ingress_gateway.go b/internal/restarter/ingress_gateway.go index d2cbdc1d4..cb2a96171 100644 --- a/internal/restarter/ingress_gateway.go +++ b/internal/restarter/ingress_gateway.go @@ -37,28 +37,28 @@ func NewIngressGatewayRestarter(client client.Client, predicates []filter.Ingres } } -func (r *IngressGatewayRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio) described_errors.DescribedError { +func (r *IngressGatewayRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio) (described_errors.DescribedError, bool) { ctrl.Log.Info("Restarting Istio Ingress Gateway") r.predicates = append(r.predicates, ingressgateway.NewRestartPredicate(istioCR)) for _, predicate := range r.predicates { evaluator, err := predicate.NewIngressGatewayEvaluator(ctx) if err != nil { - return described_errors.NewDescribedError(err, "Could not create Ingress Gateway restart evaluator") + return described_errors.NewDescribedError(err, "Could not create Ingress Gateway restart evaluator"), false } if evaluator.RequiresIngressGatewayRestart() { err = restartIngressGateway(ctx, r.client) if err != nil { r.statusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonIngressGatewayRestartFailed)) - return described_errors.NewDescribedError(err, "Failed to restart Ingress Gateway") + return described_errors.NewDescribedError(err, "Failed to restart Ingress Gateway"), false } } } r.statusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonIngressGatewayRestartSucceeded)) ctrl.Log.Info("Successfully restarted Istio Ingress Gateway") - return nil + return nil, false } func restartIngressGateway(ctx context.Context, k8sClient client.Client) error { diff --git a/internal/restarter/ingress_gateway_test.go b/internal/restarter/ingress_gateway_test.go index c3e9db992..19f46e02f 100644 --- a/internal/restarter/ingress_gateway_test.go +++ b/internal/restarter/ingress_gateway_test.go @@ -2,6 +2,8 @@ package restarter_test import ( "context" + "time" + operatorv1alpha2 "github.com/kyma-project/istio/operator/api/v1alpha2" "github.com/kyma-project/istio/operator/internal/filter" "github.com/kyma-project/istio/operator/internal/restarter" @@ -14,7 +16,6 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" - "time" ) var _ = Describe("Istio Ingress Gateway restart", func() { @@ -38,11 +39,13 @@ var _ = Describe("Istio Ingress Gateway restart", func() { igRestarter := restarter.NewIngressGatewayRestarter(fakeClient, []filter.IngressGatewayPredicate{mockIgPredicate{shouldRestart: true}}, statusHandler) //when - err := igRestarter.Restart(context.Background(), istioCR) + err, requeue := igRestarter.Restart(context.Background(), istioCR) //then - e := fakeClient.Get(context.Background(), client.ObjectKey{Namespace: gatherer.IstioNamespace, Name: "istio-ingressgateway"}, igDep) Expect(err).Should(Not(HaveOccurred())) + Expect(requeue).To(BeFalse()) + + e := fakeClient.Get(context.Background(), client.ObjectKey{Namespace: gatherer.IstioNamespace, Name: "istio-ingressgateway"}, igDep) Expect(e).Should(Not(HaveOccurred())) Expect(annotations.HasRestartAnnotation(igDep.Spec.Template.Annotations)).To(BeTrue()) @@ -70,11 +73,13 @@ var _ = Describe("Istio Ingress Gateway restart", func() { igRestarter := restarter.NewIngressGatewayRestarter(fakeClient, []filter.IngressGatewayPredicate{mockIgPredicate{shouldRestart: false}}, statusHandler) //when - err := igRestarter.Restart(context.Background(), istioCR) + err, requeue := igRestarter.Restart(context.Background(), istioCR) //then - e := fakeClient.Get(context.Background(), client.ObjectKey{Namespace: gatherer.IstioNamespace, Name: "istio-ingressgateway"}, igDep) Expect(err).Should(Not(HaveOccurred())) + Expect(requeue).To(BeFalse()) + + e := fakeClient.Get(context.Background(), client.ObjectKey{Namespace: gatherer.IstioNamespace, Name: "istio-ingressgateway"}, igDep) Expect(e).Should(Not(HaveOccurred())) Expect(annotations.HasRestartAnnotation(igDep.Spec.Template.Annotations)).To(BeFalse()) @@ -100,10 +105,11 @@ var _ = Describe("Istio Ingress Gateway restart", func() { igRestarter := restarter.NewIngressGatewayRestarter(fakeClient, []filter.IngressGatewayPredicate{mockIgPredicate{shouldRestart: true}}, statusHandler) //when - err := igRestarter.Restart(context.Background(), istioCR) + err, requeue := igRestarter.Restart(context.Background(), istioCR) //then Expect(err).Should(Not(HaveOccurred())) + Expect(requeue).To(BeFalse()) Expect((*istioCR.Status.Conditions)[0].Reason).Should(Equal(string(operatorv1alpha2.ConditionReasonIngressGatewayRestartSucceeded))) Expect((*istioCR.Status.Conditions)[0].Message).Should(Equal(operatorv1alpha2.ConditionReasonIngressGatewayRestartSucceededMessage)) diff --git a/internal/restarter/restart.go b/internal/restarter/restart.go index 44d3be66d..861eef25f 100644 --- a/internal/restarter/restart.go +++ b/internal/restarter/restart.go @@ -2,6 +2,7 @@ package restarter import ( "context" + operatorv1alpha2 "github.com/kyma-project/istio/operator/api/v1alpha2" "github.com/kyma-project/istio/operator/internal/described_errors" ) @@ -10,19 +11,21 @@ import ( // It uses predicates to evaluate if the restart is needed. // If the evaluation returns true, the restarter restarts the component. type Restarter interface { - Restart(ctx context.Context, istioCR *operatorv1alpha2.Istio) described_errors.DescribedError + Restart(ctx context.Context, istioCR *operatorv1alpha2.Istio) (described_errors.DescribedError, bool) } // Restart invokes the given restarters and returns the most severe error. -func Restart(ctx context.Context, istioCR *operatorv1alpha2.Istio, restarters []Restarter) described_errors.DescribedError { +func Restart(ctx context.Context, istioCR *operatorv1alpha2.Istio, restarters []Restarter) (described_errors.DescribedError, bool) { var restarterErrs []described_errors.DescribedError + needsRequeue := false for _, r := range restarters { - err := r.Restart(ctx, istioCR) + err, requeue := r.Restart(ctx, istioCR) + needsRequeue = requeue || needsRequeue if err != nil { restarterErrs = append(restarterErrs, err) } } - return described_errors.GetMostSevereErr(restarterErrs) + return described_errors.GetMostSevereErr(restarterErrs), needsRequeue } diff --git a/internal/restarter/restart_test.go b/internal/restarter/restart_test.go index 024a0b882..092e917e9 100644 --- a/internal/restarter/restart_test.go +++ b/internal/restarter/restart_test.go @@ -2,6 +2,7 @@ package restarter_test import ( "context" + operatorv1alpha2 "github.com/kyma-project/istio/operator/api/v1alpha2" "github.com/kyma-project/istio/operator/internal/described_errors" "github.com/kyma-project/istio/operator/internal/restarter" @@ -42,14 +43,16 @@ var _ = Describe("Restart", func() { r1 := &restarterMock{err: described_errors.NewDescribedError(errors.New("restart error"), "")} r2 := &restarterMock{err: described_errors.NewDescribedError(errors.New("restart warning"), "").SetWarning()} - err := restarter.Restart(context.Background(), &operatorv1alpha2.Istio{}, []restarter.Restarter{r1, r2}) + err, requeue := restarter.Restart(context.Background(), &operatorv1alpha2.Istio{}, []restarter.Restarter{r1, r2}) Expect(err).Should(MatchError("restart error")) + Expect(requeue).To(BeFalse()) }) }) type restarterMock struct { err described_errors.DescribedError + requeue bool restarted bool } @@ -57,7 +60,7 @@ 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 } diff --git a/internal/restarter/sidecars.go b/internal/restarter/sidecars.go index b11d340ef..663f2d2af 100644 --- a/internal/restarter/sidecars.go +++ b/internal/restarter/sidecars.go @@ -44,12 +44,12 @@ func NewSidecarsRestarter(logger logr.Logger, client client.Client, merger istio } // Restart runs Proxy Reset action, which checks if any of sidecars need a restart and proceed with rollout. -func (s *SidecarsRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio) described_errors.DescribedError { +func (s *SidecarsRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio) (described_errors.DescribedError, bool) { clusterSize, err := clusterconfig.EvaluateClusterSize(ctx, s.Client) if err != nil { s.Log.Error(err, "Error occurred during evaluation of cluster size") s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartFailed)) - return described_errors.NewDescribedError(err, errorDescription) + return described_errors.NewDescribedError(err, errorDescription), false } ctrl.Log.Info("Istio proxy resetting with", "profile", clusterSize.String()) @@ -57,14 +57,14 @@ func (s *SidecarsRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio if err != nil { s.Log.Error(err, "Failed to get IstioOperator") s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartFailed)) - return described_errors.NewDescribedError(err, errorDescription) + return described_errors.NewDescribedError(err, errorDescription), false } istioImageVersion, err := s.Merger.GetIstioImageVersion() if err != nil { ctrl.Log.Error(err, "Error getting Istio version from istio operator file") s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartFailed)) - return described_errors.NewDescribedError(err, "Could not get Istio version from istio operator file") + return described_errors.NewDescribedError(err, "Could not get Istio version from istio operator file"), false } expectedImage := pods.NewSidecarImage(iop.Spec.Hub, iop.Spec.Tag.GetStringValue()) @@ -73,21 +73,21 @@ func (s *SidecarsRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio err = gatherer.VerifyIstioPodsVersion(ctx, s.Client, istioImageVersion.Version()) if err != nil { s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartFailed)) - return described_errors.NewDescribedError(err, "Verifying Pod versions in istio-system namespace failed") + return described_errors.NewDescribedError(err, "Verifying Pod versions in istio-system namespace failed"), false } expectedResources, err := istioCR.GetProxyResources(iop) if err != nil { s.Log.Error(err, "Failed to get Istio Proxy resources") s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartFailed)) - return described_errors.NewDescribedError(err, errorDescription) + return described_errors.NewDescribedError(err, errorDescription), false } - warnings, err := s.ProxyResetter.ProxyReset(ctx, s.Client, expectedImage, expectedResources, s.Predicates, &s.Log) + warnings, requeue, err := s.ProxyResetter.ProxyReset(ctx, s.Client, expectedImage, expectedResources, s.Predicates, &s.Log) if err != nil { s.Log.Error(err, "Failed to reset proxy") s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartFailed)) - return described_errors.NewDescribedError(err, errorDescription) + return described_errors.NewDescribedError(err, errorDescription), false } warningsCount := len(warnings) @@ -108,9 +108,9 @@ func (s *SidecarsRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio warningErr := described_errors.NewDescribedError(errors.New("Istio controller could not restart one or more istio-injected pods."), "Not all pods with Istio injection could be restarted. Please take a look at kyma-system/istio-controller-manager logs to see more information about the warning").SetWarning() s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarManualRestartRequired, warningMessage)) s.Log.Info(warningMessage) - return warningErr + return warningErr, false } s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartSucceeded)) - return nil + return nil, requeue } diff --git a/internal/restarter/sidecars_test.go b/internal/restarter/sidecars_test.go index 8f09f6b24..a5fae072c 100644 --- a/internal/restarter/sidecars_test.go +++ b/internal/restarter/sidecars_test.go @@ -51,12 +51,13 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { sidecarsRestarter := restarter.NewSidecarsRestarter(logr.Discard(), createFakeClient(&istioCr, istiod), &MergerMock{"1.16.1-distroless"}, sidecars.NewProxyResetter(), []filter.SidecarProxyPredicate{}, statusHandler) // when - err := sidecarsRestarter.Restart(context.Background(), &istioCr) + err, requeue := sidecarsRestarter.Restart(context.Background(), &istioCr) // then Expect(err).Should(HaveOccurred()) Expect(err.Level()).To(Equal(described_errors.Error)) Expect(err.Error()).To(ContainSubstring("istio-system Pods version 1.16.0 do not match istio operator version 1.16.1")) + Expect(requeue).To(BeFalse()) Expect((*istioCr.Status.Conditions)[0].Message).To(Equal("Proxy sidecar restart failed")) }) @@ -109,11 +110,12 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { &MergerMock{"1.16.1-distroless"}, proxyResetter, []filter.SidecarProxyPredicate{}, statusHandler) // when - err := sidecarsRestarter.Restart(context.Background(), &istioCr) + err, requeue := sidecarsRestarter.Restart(context.Background(), &istioCr) // then Expect(err).Should(HaveOccurred()) Expect(err.Level()).To(Equal(described_errors.Warning)) + Expect(requeue).To(BeFalse()) 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)")) }) @@ -150,11 +152,12 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { &MergerMock{"1.16.1-distroless"}, proxyResetter, []filter.SidecarProxyPredicate{}, statusHandler) // when - err := sidecarsRestarter.Restart(context.Background(), &istioCr) + err, requeue := sidecarsRestarter.Restart(context.Background(), &istioCr) // then Expect(err).Should(HaveOccurred()) Expect(err.Level()).To(Equal(described_errors.Warning)) + Expect(requeue).To(BeFalse()) Expect((*istioCr.Status.Conditions)[0].Message).To(Equal("The sidecars of the following workloads could not be restarted: ns1/name1, ns2/name2")) }) @@ -180,10 +183,11 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { &MergerMock{"1.16.1-distroless"}, proxyResetter, []filter.SidecarProxyPredicate{}, statusHandler) // when - err := sidecarsRestarter.Restart(context.Background(), &istioCr) + err, requeue := sidecarsRestarter.Restart(context.Background(), &istioCr) // then Expect(err).Should(Not(HaveOccurred())) + Expect(requeue).To(BeFalse()) Expect((*istioCr.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonProxySidecarRestartSucceeded))) Expect((*istioCr.Status.Conditions)[0].Message).To(Equal(operatorv1alpha2.ConditionReasonProxySidecarRestartSucceededMessage)) }) @@ -249,10 +253,11 @@ func (m MergerMock) GetIstioImageVersion() (istiooperator.IstioImageVersion, err func (m MergerMock) SetIstioInstallFlavor(_ clusterconfig.ClusterSize) {} type proxyResetterMock struct { - err error restartWarnings []restart.RestartWarning + hasMorePods bool + err error } -func (p *proxyResetterMock) ProxyReset(_ context.Context, _ client.Client, _ pods.SidecarImage, _ v1.ResourceRequirements, _ []filter.SidecarProxyPredicate, _ *logr.Logger) ([]restart.RestartWarning, error) { - return p.restartWarnings, p.err +func (p *proxyResetterMock) ProxyReset(_ context.Context, _ client.Client, _ pods.SidecarImage, _ v1.ResourceRequirements, _ []filter.SidecarProxyPredicate, _ *logr.Logger) ([]restart.RestartWarning, bool, error) { + return p.restartWarnings, p.hasMorePods, p.err } diff --git a/pkg/lib/sidecars/pods/filter.go b/pkg/lib/sidecars/pods/filter.go index 80674aaf5..d804c32cf 100644 --- a/pkg/lib/sidecars/pods/filter.go +++ b/pkg/lib/sidecars/pods/filter.go @@ -1,8 +1,6 @@ package pods import ( - "context" - "github.com/kyma-project/istio/operator/internal/filter" v1 "k8s.io/api/core/v1" ) @@ -20,22 +18,10 @@ func NewRestartProxyPredicate(expectedImage SidecarImage, expectedResources v1.R return &RestartProxyPredicate{expectedImage: expectedImage, expectedResources: expectedResources} } -type ProxyRestartEvaluator struct { - expectedImage SidecarImage - expectedResources v1.ResourceRequirements -} - -func (p ProxyRestartEvaluator) RequiresProxyRestart(pod v1.Pod) bool { +func (p RestartProxyPredicate) RequiresProxyRestart(pod v1.Pod) bool { return needsRestart(pod, p.expectedImage, *p.expectedResources.DeepCopy()) } -func (r RestartProxyPredicate) NewProxyRestartEvaluator(_ context.Context) (filter.ProxyRestartEvaluator, error) { - return ProxyRestartEvaluator{ - expectedImage: r.expectedImage, - expectedResources: *r.expectedResources.DeepCopy(), - }, nil -} - func needsRestart(pod v1.Pod, expectedImage SidecarImage, expectedResources v1.ResourceRequirements) bool { return HasIstioSidecarStatusAnnotation(pod) && IsPodReady(pod) && @@ -72,7 +58,6 @@ func hasCustomImageAnnotation(pod v1.Pod) bool { } func hasSidecarContainerWithWithDifferentImage(pod v1.Pod, expectedImage SidecarImage) bool { - for _, container := range pod.Spec.Containers { if isContainerIstioSidecar(container) && !expectedImage.matchesImageIn(container) { return true @@ -82,7 +67,6 @@ func hasSidecarContainerWithWithDifferentImage(pod v1.Pod, expectedImage Sidecar } func hasDifferentSidecarResources(pod v1.Pod, expectedResources v1.ResourceRequirements) bool { - for _, container := range pod.Spec.Containers { if isContainerIstioSidecar(container) && !containerHasResources(container, expectedResources) { return true diff --git a/pkg/lib/sidecars/pods/filter_test.go b/pkg/lib/sidecars/pods/filter_test.go index 1ce7b0119..79dd46004 100644 --- a/pkg/lib/sidecars/pods/filter_test.go +++ b/pkg/lib/sidecars/pods/filter_test.go @@ -1,7 +1,6 @@ package pods_test import ( - "context" "github.com/kyma-project/istio/operator/pkg/lib/sidecars/pods" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -14,9 +13,7 @@ var _ = Describe("Evaluate restart", func() { 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{}) - evaluator, err := predicate.NewProxyRestartEvaluator(context.Background()) - Expect(err).ToNot(HaveOccurred()) - Expect(evaluator.RequiresProxyRestart(pod)).To(BeFalse()) + Expect(predicate.RequiresProxyRestart(pod)).To(BeFalse()) }) @@ -24,9 +21,7 @@ var _ = Describe("Evaluate restart", func() { pod := createPodWithProxySidecar("test-pod", "test-namespace", "1.21.0", map[string]string{}) predicate := pods.NewRestartProxyPredicate(pods.NewSidecarImage("istio", "1.22.0"), v1.ResourceRequirements{}) - evaluator, err := predicate.NewProxyRestartEvaluator(context.Background()) - Expect(err).ToNot(HaveOccurred()) - Expect(evaluator.RequiresProxyRestart(pod)).To(BeTrue()) + Expect(predicate.RequiresProxyRestart(pod)).To(BeTrue()) }) }) diff --git a/pkg/lib/sidecars/pods/get.go b/pkg/lib/sidecars/pods/get.go index 61257da21..3ea821c54 100644 --- a/pkg/lib/sidecars/pods/get.go +++ b/pkg/lib/sidecars/pods/get.go @@ -13,6 +13,8 @@ import ( ) const ( + maxCountPodsToRestart = 10 + istioSidecarContainerName string = "istio-proxy" ) @@ -36,13 +38,17 @@ func (r SidecarImage) matchesImageIn(container v1.Container) bool { return container.Image == r.String() } -func getAllRunningPods(ctx context.Context, c client.Client) (*v1.PodList, error) { +func getRunningPods(ctx context.Context, c client.Client, continueToken string) (*v1.PodList, error) { podList := &v1.PodList{} isRunning := fields.OneTermEqualSelector("status.phase", string(v1.PodRunning)) err := retry.RetryOnError(retry.DefaultRetry, func() error { - return c.List(ctx, podList, client.MatchingFieldsSelector{Selector: isRunning}) + listOps := []client.ListOption{client.MatchingFieldsSelector{Selector: isRunning}, client.Limit(maxCountPodsToRestart)} + if continueToken != "" { + listOps = append(listOps, client.Continue(continueToken)) + } + return c.List(ctx, podList, listOps...) }) if err != nil { return podList, err @@ -51,33 +57,38 @@ func getAllRunningPods(ctx context.Context, c client.Client) (*v1.PodList, error return podList, nil } -func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) (outputPodsList *v1.PodList, err error) { - podList, err := getAllRunningPods(ctx, c) - if err != nil { - return nil, err - } - +func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) (*v1.PodList, error) { //Add predicate for image version and resources configuration predicates = append(predicates, NewRestartProxyPredicate(expectedImage, expectedResources)) - for _, predicate := range predicates { - evaluator, err := predicate.NewProxyRestartEvaluator(ctx) + continueToken := "" + podListToRestart := &v1.PodList{} + + for while := true; while; { + podList, err := getRunningPods(ctx, c, continueToken) if err != nil { - return &v1.PodList{}, err + return nil, err } - outputPodsList = &v1.PodList{} - for _, pod := range podList.Items { - if evaluator.RequiresProxyRestart(pod) { - outputPodsList.Items = append(outputPodsList.Items, pod) + for _, predicate := range predicates { + for _, pod := range podList.Items { + if predicate.RequiresProxyRestart(pod) { + podListToRestart.Items = append(podListToRestart.Items, pod) + } } } + + podListToRestart.Continue = podList.Continue + podListToRestart.RemainingItemCount = podList.RemainingItemCount + + while = len(podListToRestart.Items) < maxCountPodsToRestart && podListToRestart.Continue != "" } - if outputPodsList != nil { - logger.Info("Pods to restart", "number of pods", len(outputPodsList.Items)) + if len(podListToRestart.Items) > 0 { + logger.Info("Pods to restart", "number of pods", len(podListToRestart.Items), "has more pods", podListToRestart.Continue != "") } - return outputPodsList, nil + + return podListToRestart, nil } func containsSidecar(pod v1.Pod) bool { diff --git a/pkg/lib/sidecars/proxy.go b/pkg/lib/sidecars/proxy.go index a09171ba6..d3ba0f052 100644 --- a/pkg/lib/sidecars/proxy.go +++ b/pkg/lib/sidecars/proxy.go @@ -13,7 +13,7 @@ import ( ) type ProxyResetter interface { - ProxyReset(ctx context.Context, c client.Client, expectedImage pods.SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) ([]restart.RestartWarning, error) + ProxyReset(ctx context.Context, c client.Client, expectedImage pods.SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) ([]restart.RestartWarning, bool, error) } type ProxyReset struct { @@ -23,18 +23,28 @@ func NewProxyResetter() *ProxyReset { return &ProxyReset{} } -func (p *ProxyReset) ProxyReset(ctx context.Context, c client.Client, expectedImage pods.SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) ([]restart.RestartWarning, error) { +func (p *ProxyReset) ProxyReset(ctx context.Context, c client.Client, expectedImage pods.SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) ([]restart.RestartWarning, bool, error) { podListToRestart, err := pods.GetPodsToRestart(ctx, c, expectedImage, expectedResources, predicates, logger) if err != nil { - return nil, err + return nil, false, err } + // if there are more pods to restart there should be a continue token in the pod list + hasMorePodsToRestart := podListToRestart.Continue != "" warnings, err := restart.Restart(ctx, c, podListToRestart, logger) if err != nil { - return nil, err + return nil, hasMorePodsToRestart, err } - logger.Info("Proxy reset done") + if !hasMorePodsToRestart { + logger.Info("Proxy reset completed") + } else { + leftoverPodsToRestart := int64(0) + if podListToRestart.RemainingItemCount != nil { + leftoverPodsToRestart = *podListToRestart.RemainingItemCount + } + logger.Info("Proxy reset partially completed", "count of leftover pods", leftoverPodsToRestart) + } - return warnings, nil + return warnings, hasMorePodsToRestart, nil } diff --git a/pkg/lib/sidecars/test/cases.go b/pkg/lib/sidecars/test/cases.go index 3db839f6c..a857381d2 100644 --- a/pkg/lib/sidecars/test/cases.go +++ b/pkg/lib/sidecars/test/cases.go @@ -21,13 +21,14 @@ const restartAnnotationName = "istio-operator.kyma-project.io/restartedAt" func (s *scenario) aRestartHappens(sidecarImage string) error { pr := sidecars.NewProxyResetter() - warnings, err := pr.ProxyReset(context.TODO(), + warnings, hasMorePods, err := pr.ProxyReset(context.TODO(), s.Client, pods.SidecarImage{Repository: "istio/proxyv2", Tag: sidecarImage}, helpers.DefaultSidecarResources, []filter.SidecarProxyPredicate{}, &s.logger) s.restartWarnings = warnings + s.hasMorePodsToRestart = hasMorePods return err } @@ -46,13 +47,14 @@ func (s *scenario) aRestartHappensWithUpdatedResources(sidecarImage string, reso return fmt.Errorf("unknown resource type %s", resourceType) } pr := sidecars.NewProxyResetter() - warnings, err := pr.ProxyReset(context.TODO(), + warnings, hasMorePods, err := pr.ProxyReset(context.TODO(), s.Client, pods.SidecarImage{Repository: "istio/proxyv2", Tag: sidecarImage}, resources, []filter.SidecarProxyPredicate{}, &s.logger) s.restartWarnings = warnings + s.hasMorePodsToRestart = hasMorePods return err } diff --git a/pkg/lib/sidecars/test/test_client.go b/pkg/lib/sidecars/test/test_client.go index 27f5658e1..5dd5ee469 100644 --- a/pkg/lib/sidecars/test/test_client.go +++ b/pkg/lib/sidecars/test/test_client.go @@ -40,6 +40,7 @@ type scenario struct { istioVersion string injectionNamespaceSelector NamespaceSelector restartWarnings []restart.RestartWarning + hasMorePodsToRestart bool } func newScenario() (*scenario, error) { From 1fd5825addadc88744e88f8c46c45b39aac5eef1 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Tue, 9 Jul 2024 11:16:37 +0200 Subject: [PATCH 02/22] Refactor --- internal/compatibility/proxy_restart.go | 40 +++------ internal/compatibility/proxy_restart_test.go | 56 ++++-------- internal/restarter/sidecars.go | 13 ++- pkg/lib/sidecars/pods/filter_test.go | 1 - pkg/lib/sidecars/pods/get.go | 91 +++++++++----------- pkg/lib/sidecars/proxy.go | 13 ++- 6 files changed, 91 insertions(+), 123 deletions(-) diff --git a/internal/compatibility/proxy_restart.go b/internal/compatibility/proxy_restart.go index 40faa05bb..9f500e79d 100644 --- a/internal/compatibility/proxy_restart.go +++ b/internal/compatibility/proxy_restart.go @@ -1,51 +1,39 @@ package compatibility import ( - "context" "github.com/kyma-project/istio/operator/api/v1alpha2" - "github.com/kyma-project/istio/operator/internal/filter" "github.com/kyma-project/istio/operator/internal/reconciliations/istio" v1 "k8s.io/api/core/v1" ) type ProxyRestartPredicate struct { - istioCR *v1alpha2.Istio - config config -} - -func NewRestartPredicate(istioCR *v1alpha2.Istio) *ProxyRestartPredicate { - return &ProxyRestartPredicate{istioCR: istioCR, config: config{proxyMetadata: v1alpha2.ProxyMetaDataCompatibility}} -} - -type config struct { - proxyMetadata map[string]string -} - -func (c config) hasProxyMetadata() bool { - return len(c.proxyMetadata) > 0 + oldCompatibilityMode bool + newCompatibilityMode bool + config config } -func (p ProxyRestartPredicate) NewProxyRestartEvaluator(_ context.Context) (filter.ProxyRestartEvaluator, error) { - lastAppliedConfig, err := istio.GetLastAppliedConfiguration(p.istioCR) +func NewRestartPredicate(istioCR *v1alpha2.Istio) (*ProxyRestartPredicate, error) { + lastAppliedConfig, err := istio.GetLastAppliedConfiguration(istioCR) if err != nil { return nil, err } - return ProxiesRestartEvaluator{ + return &ProxyRestartPredicate{ oldCompatibilityMode: lastAppliedConfig.IstioSpec.CompatibilityMode, - newCompatibilityMode: p.istioCR.Spec.CompatibilityMode, - config: p.config, + newCompatibilityMode: istioCR.Spec.CompatibilityMode, + config: config{proxyMetadata: v1alpha2.ProxyMetaDataCompatibility}, }, nil } -type ProxiesRestartEvaluator struct { - oldCompatibilityMode bool - newCompatibilityMode bool - config config +type config struct { + proxyMetadata map[string]string } -func (p ProxiesRestartEvaluator) RequiresProxyRestart(_ v1.Pod) bool { +func (c config) hasProxyMetadata() bool { + return len(c.proxyMetadata) > 0 +} +func (p ProxyRestartPredicate) RequiresProxyRestart(_ v1.Pod) bool { if p.config.hasProxyMetadata() && p.oldCompatibilityMode != p.newCompatibilityMode { return true } diff --git a/internal/compatibility/proxy_restart_test.go b/internal/compatibility/proxy_restart_test.go index c710f5b3a..e74990072 100644 --- a/internal/compatibility/proxy_restart_test.go +++ b/internal/compatibility/proxy_restart_test.go @@ -1,7 +1,6 @@ package compatibility import ( - "context" "github.com/kyma-project/istio/operator/pkg/labels" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -14,105 +13,89 @@ import ( var _ = Describe("Proxy Restarter", func() { Context("RequiresProxyRestart", func() { It("Should evaluate to true when proxy metadata values exist and new and old compatibility mode is different", func() { - evaluator := ProxiesRestartEvaluator{ + predicate := ProxyRestartPredicate{ oldCompatibilityMode: true, newCompatibilityMode: false, config: config{ proxyMetadata: map[string]string{"key": "value"}, }, } - - Expect(evaluator.RequiresProxyRestart(v1.Pod{})).To(BeTrue()) + Expect(predicate.RequiresProxyRestart(v1.Pod{})).To(BeTrue()) }) It("Should evaluate to false when proxy metadata values exist and new and old compatibility mode is equal", func() { - evaluator := ProxiesRestartEvaluator{ + predicate := ProxyRestartPredicate{ oldCompatibilityMode: true, newCompatibilityMode: true, config: config{ proxyMetadata: map[string]string{"key": "value"}, }, } - - Expect(evaluator.RequiresProxyRestart(v1.Pod{})).To(BeFalse()) + Expect(predicate.RequiresProxyRestart(v1.Pod{})).To(BeFalse()) }) It("Should evaluate to false when no proxy metadata values exist and new and old compatibility mode is different", func() { - evaluator := ProxiesRestartEvaluator{ + predicate := ProxyRestartPredicate{ oldCompatibilityMode: true, newCompatibilityMode: false, } - - Expect(evaluator.RequiresProxyRestart(v1.Pod{})).To(BeFalse()) + Expect(predicate.RequiresProxyRestart(v1.Pod{})).To(BeFalse()) }) It("Should evaluate to false when no proxy metadata values exist and new and old compatibility mode is equal", func() { - evaluator := ProxiesRestartEvaluator{ + predicate := ProxyRestartPredicate{ oldCompatibilityMode: true, newCompatibilityMode: true, } - - Expect(evaluator.RequiresProxyRestart(v1.Pod{})).To(BeFalse()) - + Expect(predicate.RequiresProxyRestart(v1.Pod{})).To(BeFalse()) }) }) - Context("NewProxyRestartEvaluator", func() { - + Context("NewRestartPredicate", func() { It("Should return an error if getLastAppliedConfiguration fails", func() { - predicate := NewRestartPredicate(&operatorv1alpha2.Istio{ + _, err := NewRestartPredicate(&operatorv1alpha2.Istio{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ labels.LastAppliedConfiguration: `{"compatibilityMode":abc}`, }, }, }) - _, err := predicate.NewProxyRestartEvaluator(context.Background()) - Expect(err).To(HaveOccurred()) }) It("Should return false for old compatibility mode if lastAppliedConfiguration is empty", func() { - predicate := NewRestartPredicate(&operatorv1alpha2.Istio{ + predicate, err := NewRestartPredicate(&operatorv1alpha2.Istio{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{}, }, }) - evaluator, err := predicate.NewProxyRestartEvaluator(context.Background()) - Expect(err).NotTo(HaveOccurred()) - Expect(evaluator).NotTo(BeNil()) - Expect(evaluator.(ProxiesRestartEvaluator).oldCompatibilityMode).To(BeFalse()) + Expect(predicate).NotTo(BeNil()) + Expect(predicate.oldCompatibilityMode).To(BeFalse()) }) It("Should return value for old compatibility mode from lastAppliedConfiguration", func() { - predicate := NewRestartPredicate(&operatorv1alpha2.Istio{ + predicate, err := NewRestartPredicate(&operatorv1alpha2.Istio{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ labels.LastAppliedConfiguration: `{"compatibilityMode":true}`, }, }, }) - - evaluator, err := predicate.NewProxyRestartEvaluator(context.Background()) - Expect(err).NotTo(HaveOccurred()) - Expect(evaluator).NotTo(BeNil()) - Expect(evaluator.(ProxiesRestartEvaluator).oldCompatibilityMode).To(BeTrue()) + Expect(predicate).NotTo(BeNil()) + Expect(predicate.oldCompatibilityMode).To(BeTrue()) }) It("Should return value for new compatibility mode from istio CR", func() { - predicate := NewRestartPredicate(&operatorv1alpha2.Istio{ + predicate, err := NewRestartPredicate(&operatorv1alpha2.Istio{ Spec: operatorv1alpha2.IstioSpec{ CompatibilityMode: true, }, }) - - evaluator, err := predicate.NewProxyRestartEvaluator(context.Background()) - Expect(err).NotTo(HaveOccurred()) - Expect(evaluator).NotTo(BeNil()) - Expect(evaluator.(ProxiesRestartEvaluator).newCompatibilityMode).To(BeTrue()) + Expect(predicate).NotTo(BeNil()) + Expect(predicate.newCompatibilityMode).To(BeTrue()) }) }) @@ -127,7 +110,6 @@ var _ = Describe("Proxy Restarter", func() { It("Should return false if proxy metadata values do not exist", func() { config := config{} - Expect(config.hasProxyMetadata()).To(BeFalse()) }) }) diff --git a/internal/restarter/sidecars.go b/internal/restarter/sidecars.go index 81374b99f..45b9faec5 100644 --- a/internal/restarter/sidecars.go +++ b/internal/restarter/sidecars.go @@ -3,9 +3,10 @@ package restarter import ( "context" "fmt" - "github.com/kyma-project/istio/operator/internal/compatibility" "strings" + "github.com/kyma-project/istio/operator/internal/compatibility" + "github.com/kyma-project/istio/operator/api/v1alpha2" "github.com/kyma-project/istio/operator/internal/described_errors" "github.com/pkg/errors" @@ -82,8 +83,14 @@ func (s *SidecarsRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio return described_errors.NewDescribedError(err, errorDescription), false } - predicates := []filter.SidecarProxyPredicate{compatibility.NewRestartPredicate(istioCR)} - warnings, requeue, err := s.ProxyResetter.ProxyReset(ctx, s.Client, expectedImage, expectedResources, predicates, &s.Log) + compatibiltyPredicate, err := compatibility.NewRestartPredicate(istioCR) + if err != nil { + s.Log.Error(err, "Failed to create restart compatibility predicate") + s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartFailed)) + return described_errors.NewDescribedError(err, errorDescription), false + } + + warnings, requeue, err := s.ProxyResetter.ProxyReset(ctx, s.Client, expectedImage, expectedResources, []filter.SidecarProxyPredicate{compatibiltyPredicate}, &s.Log) if err != nil { s.Log.Error(err, "Failed to reset proxy") s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartFailed)) diff --git a/pkg/lib/sidecars/pods/filter_test.go b/pkg/lib/sidecars/pods/filter_test.go index 79dd46004..ebf1658a3 100644 --- a/pkg/lib/sidecars/pods/filter_test.go +++ b/pkg/lib/sidecars/pods/filter_test.go @@ -22,7 +22,6 @@ var _ = Describe("Evaluate restart", func() { predicate := pods.NewRestartProxyPredicate(pods.NewSidecarImage("istio", "1.22.0"), v1.ResourceRequirements{}) Expect(predicate.RequiresProxyRestart(pod)).To(BeTrue()) - }) }) diff --git a/pkg/lib/sidecars/pods/get.go b/pkg/lib/sidecars/pods/get.go index 4fc701623..4eabd66fb 100644 --- a/pkg/lib/sidecars/pods/get.go +++ b/pkg/lib/sidecars/pods/get.go @@ -13,6 +13,8 @@ import ( ) const ( + maxPodsToRestartAtOnce = 10 + istioSidecarContainerName string = "istio-proxy" ) @@ -36,81 +38,74 @@ func (r SidecarImage) matchesImageIn(container v1.Container) bool { return container.Image == r.String() } -func getAllRunningPods(ctx context.Context, c client.Client) (*v1.PodList, error) { +func listRunningPods(ctx context.Context, c client.Client, continueToken string) (*v1.PodList, error) { podList := &v1.PodList{} - isRunning := fields.OneTermEqualSelector("status.phase", string(v1.PodRunning)) - err := retry.RetryOnError(retry.DefaultRetry, func() error { - return c.List(ctx, podList, client.MatchingFieldsSelector{Selector: isRunning}) + listOps := []client.ListOption{ + client.MatchingFieldsSelector{Selector: fields.OneTermEqualSelector("status.phase", string(v1.PodRunning))}, + client.Limit(maxPodsToRestartAtOnce), + } + if continueToken != "" { + listOps = append(listOps, client.Continue(continueToken)) + } + return c.List(ctx, podList, listOps...) }) - if err != nil { - return podList, err - } - return podList, nil + return podList, err } -func getSidecarPods(ctx context.Context, c client.Client, logger *logr.Logger) (*[]v1.Pod, error) { - podList, err := getAllRunningPods(ctx, c) +func getSidecarPods(ctx context.Context, c client.Client, logger *logr.Logger, continueToken string) (*v1.PodList, error) { + podList, err := listRunningPods(ctx, c, continueToken) if err != nil { return nil, err } - logger.Info("Read all running pods for proxy restart", "number of pods", len(podList.Items)) - podsWithSidecar := make([]v1.Pod, 0, len(podList.Items)) + logger.Info("Retrieved running pods for proxy restart", "number of pods", len(podList.Items), "has more pods", podList.Continue != "") + + podsWithSidecar := &v1.PodList{} + podsWithSidecar.Continue = podList.Continue + for _, pod := range podList.Items { if isReadyWithIstioAnnotation(pod) { - podsWithSidecar = append(podsWithSidecar, pod) + podsWithSidecar.Items = append(podsWithSidecar.Items, pod) } } - logger.Info("Filtered pods with Istio sidecar", "number of pods", len(podsWithSidecar)) - return &podsWithSidecar, nil + logger.Info("Filtered pods with Istio sidecar", "number of pods", len(podsWithSidecar.Items)) + + return podsWithSidecar, nil } -func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) (outputPodsList *v1.PodList, err error) { +func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) (*v1.PodList, error) { //Add predicate for image version and resources configuration predicates = append(predicates, NewRestartProxyPredicate(expectedImage, expectedResources)) - evaluators, err := initRestartEvaluators(ctx, predicates) - if err != nil { - return &v1.PodList{}, err - } - pods, err := getSidecarPods(ctx, c, logger) - if err != nil { - return &v1.PodList{}, err - } - - outputPodsList = &v1.PodList{} - outputPodsList.Items = make([]v1.Pod, 0, len(*pods)) + podsToRestart := &v1.PodList{} + for while := true; while; { + podsWithSidecar, err := getSidecarPods(ctx, c, logger, podsToRestart.Continue) + if err != nil { + return nil, err + } - for _, pod := range *pods { - for _, evaluator := range evaluators { - if evaluator.RequiresProxyRestart(pod) { - outputPodsList.Items = append(outputPodsList.Items, pod) - // To avoid adding the same pod multiple times, we need to skip the remaining evaluators - break + for _, pod := range podsWithSidecar.Items { + for _, predicate := range predicates { + if predicate.RequiresProxyRestart(pod) { + podsToRestart.Items = append(podsToRestart.Items, pod) + break + } } } - } - - logger.Info("Pods to restart", "number of pods", len(outputPodsList.Items)) - return outputPodsList, nil -} -func initRestartEvaluators(ctx context.Context, predicates []filter.SidecarProxyPredicate) ([]filter.ProxyRestartEvaluator, error) { - var evaluators []filter.ProxyRestartEvaluator - - for _, predicate := range predicates { - e, err := predicate.NewProxyRestartEvaluator(ctx) - if err != nil { - return nil, err - } + podsToRestart.Continue = podsWithSidecar.Continue + while = len(podsToRestart.Items) < maxPodsToRestartAtOnce && podsToRestart.Continue != "" + } - evaluators = append(evaluators, e) + if len(podsToRestart.Items) > 0 { + logger.Info("Pods to restart", "number of pods", len(podsToRestart.Items), "has more pods", podsToRestart.Continue != "") } - return evaluators, nil + + return podsToRestart, nil } func containsSidecar(pod v1.Pod) bool { diff --git a/pkg/lib/sidecars/proxy.go b/pkg/lib/sidecars/proxy.go index d3ba0f052..c09cbbe45 100644 --- a/pkg/lib/sidecars/proxy.go +++ b/pkg/lib/sidecars/proxy.go @@ -24,14 +24,15 @@ func NewProxyResetter() *ProxyReset { } func (p *ProxyReset) ProxyReset(ctx context.Context, c client.Client, expectedImage pods.SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) ([]restart.RestartWarning, bool, error) { - podListToRestart, err := pods.GetPodsToRestart(ctx, c, expectedImage, expectedResources, predicates, logger) + podsToRestart, err := pods.GetPodsToRestart(ctx, c, expectedImage, expectedResources, predicates, logger) if err != nil { return nil, false, err } // if there are more pods to restart there should be a continue token in the pod list - hasMorePodsToRestart := podListToRestart.Continue != "" - warnings, err := restart.Restart(ctx, c, podListToRestart, logger) + hasMorePodsToRestart := podsToRestart.Continue != "" + + warnings, err := restart.Restart(ctx, c, podsToRestart, logger) if err != nil { return nil, hasMorePodsToRestart, err } @@ -39,11 +40,7 @@ func (p *ProxyReset) ProxyReset(ctx context.Context, c client.Client, expectedIm if !hasMorePodsToRestart { logger.Info("Proxy reset completed") } else { - leftoverPodsToRestart := int64(0) - if podListToRestart.RemainingItemCount != nil { - leftoverPodsToRestart = *podListToRestart.RemainingItemCount - } - logger.Info("Proxy reset partially completed", "count of leftover pods", leftoverPodsToRestart) + logger.Info("Proxy reset partially completed") } return warnings, hasMorePodsToRestart, nil From 32ff0689d015b4dfa78d750bca75f25a40a545a8 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Tue, 9 Jul 2024 16:12:39 +0200 Subject: [PATCH 03/22] State --- api/v1alpha2/conditions.go | 2 +- api/v1alpha2/istio_types.go | 4 +- internal/restarter/restart.go | 1 + internal/restarter/restart_test.go | 58 +++++++++++++++---- internal/restarter/sidecars.go | 7 ++- internal/restarter/sidecars_test.go | 43 ++++++++++++-- pkg/lib/sidecars/pods/filter_test.go | 23 +++++--- pkg/lib/sidecars/pods/get.go | 16 +++-- pkg/lib/sidecars/pods/get_test.go | 53 +++++++++-------- pkg/lib/sidecars/proxy.go | 8 ++- pkg/lib/sidecars/test/cases.go | 1 - pkg/lib/sidecars/test/features/update.feature | 2 +- 12 files changed, 157 insertions(+), 61 deletions(-) diff --git a/api/v1alpha2/conditions.go b/api/v1alpha2/conditions.go index e2fcb84f3..3a74d12b3 100644 --- a/api/v1alpha2/conditions.go +++ b/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}, diff --git a/api/v1alpha2/istio_types.go b/api/v1alpha2/istio_types.go index a0294f5b8..f154927e3 100644 --- a/api/v1alpha2/istio_types.go +++ b/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" diff --git a/internal/restarter/restart.go b/internal/restarter/restart.go index 861eef25f..d7bbe02b8 100644 --- a/internal/restarter/restart.go +++ b/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) } diff --git a/internal/restarter/restart_test.go b/internal/restarter/restart_test.go index 092e917e9..bcc6dba59 100644 --- a/internal/restarter/restart_test.go +++ b/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 { diff --git a/internal/restarter/sidecars.go b/internal/restarter/sidecars.go index 45b9faec5..373984f75 100644 --- a/internal/restarter/sidecars.go +++ b/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 } diff --git a/internal/restarter/sidecars_test.go b/internal/restarter/sidecars_test.go index fa46a120b..ffb75a8cb 100644 --- a/internal/restarter/sidecars_test.go +++ b/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 { diff --git a/pkg/lib/sidecars/pods/filter_test.go b/pkg/lib/sidecars/pods/filter_test.go index ebf1658a3..3f3c72200 100644 --- a/pkg/lib/sidecars/pods/filter_test.go +++ b/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()) }) }) diff --git a/pkg/lib/sidecars/pods/get.go b/pkg/lib/sidecars/pods/get.go index 4eabd66fb..2dfea8f48 100644 --- a/pkg/lib/sidecars/pods/get.go +++ b/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 { diff --git a/pkg/lib/sidecars/pods/get_test.go b/pkg/lib/sidecars/pods/get_test.go index 388508868..466014f67 100644 --- a/pkg/lib/sidecars/pods/get_test.go +++ b/pkg/lib/sidecars/pods/get_test.go @@ -48,7 +48,6 @@ var _ = ReportAfterSuite("custom reporter", func(report types.Report) { }) var _ = Describe("Get Pods", func() { - ctx := context.Background() logger := logr.Discard() @@ -59,22 +58,25 @@ var _ = Describe("Get Pods", func() { name string c client.Client predicates []filter.SidecarProxyPredicate + limit int assertFunc func(val interface{}) }{ { - name: "should not return pods without istio sidecar", + name: "Should not return pods without istio sidecar", c: createClientSet(helpers.FixPodWithoutSidecar("app", "custom")), + limit: 10, assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, }, { - name: "should not return any pod when pods have correct image", + name: "Should not return any pod when pods have correct image", c: createClientSet( helpers.NewSidecarPodBuilder().Build(), ), + limit: 10, assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, }, { - name: "should return pod with different image repository", + name: "Should return pod with different image repository", c: createClientSet( helpers.NewSidecarPodBuilder().Build(), helpers.NewSidecarPodBuilder(). @@ -82,6 +84,7 @@ var _ = Describe("Get Pods", func() { SetSidecarImageRepository("istio/different-proxy"). Build(), ), + limit: 10, assertFunc: func(val interface{}) { Expect(val).NotTo(BeEmpty()) resultPods := val.([]v1.Pod) @@ -89,7 +92,7 @@ var _ = Describe("Get Pods", func() { }, }, { - name: "should return pod with different image tag", + name: "Should return pod with different image tag", c: createClientSet( helpers.NewSidecarPodBuilder().Build(), helpers.NewSidecarPodBuilder(). @@ -97,6 +100,7 @@ var _ = Describe("Get Pods", func() { SetSidecarImageTag("1.11.0"). Build(), ), + limit: 10, assertFunc: func(val interface{}) { Expect(val).NotTo(BeEmpty()) resultPods := val.([]v1.Pod) @@ -105,53 +109,58 @@ var _ = Describe("Get Pods", func() { }, }, { - name: "should ignore pod that has different image tag when it has not all condition status as True", + name: "Should ignore pod that has different image tag when it has not all condition status as True", c: createClientSet( helpers.NewSidecarPodBuilder(). SetSidecarImageTag("1.12.0"). SetConditionStatus("False"). Build(), ), + limit: 10, assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, }, { - name: "should ignore pod that has different image tag when phase is not running", + name: "Should ignore pod that has different image tag when phase is not running", c: createClientSet( helpers.NewSidecarPodBuilder(). SetSidecarImageTag("1.12.0"). SetPodStatusPhase("Pending"). Build(), ), + limit: 10, assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, }, { - name: "should ignore pod that has different image tag when it has a deletion timestamp", + name: "Should ignore pod that has different image tag when it has a deletion timestamp", c: createClientSet( helpers.NewSidecarPodBuilder(). SetSidecarImageTag("1.12.0"). SetDeletionTimestamp(time.Now()). Build(), ), + limit: 10, assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, }, { - name: "should ignore pod that has different image tag when proxy container name is not in istio annotation", + name: "Should ignore pod that has different image tag when proxy container name is not in istio annotation", c: createClientSet( helpers.NewSidecarPodBuilder(). SetSidecarImageTag("1.12.0"). SetSidecarContainerName("custom-sidecar-proxy-container-name"). Build(), ), + limit: 10, assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, }, { - name: "should contain only one pod when there are multiple predicates that would restart the pod", + name: "Should contain only one pod when there are multiple predicates that would restart the pod", c: createClientSet( helpers.NewSidecarPodBuilder(). SetName("changedSidecarPod"). SetSidecarImageRepository("istio/different-proxy"). Build(), ), + limit: 10, predicates: []filter.SidecarProxyPredicate{pods.NewRestartProxyPredicate(expectedImage, helpers.DefaultSidecarResources)}, assertFunc: func(val interface{}) { Expect(val).NotTo(BeEmpty()) @@ -163,7 +172,7 @@ var _ = Describe("Get Pods", func() { for _, tt := range tests { It(tt.name, func() { - podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, tt.predicates, &logger) + podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, tt.predicates, tt.limit, &logger) Expect(err).NotTo(HaveOccurred()) tt.assertFunc(podList.Items) @@ -172,21 +181,20 @@ var _ = Describe("Get Pods", func() { }) When("Sidecar Resources changed", func() { - tests := []struct { name string c client.Client assertFunc func(val interface{}) }{ { - name: "should not return any pod when pods have same resources", + name: "Should not return any pod when pods have same resources", c: createClientSet( helpers.NewSidecarPodBuilder().Build(), ), assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, }, { - name: "should return pod with different sidecar resources", + name: "Should return pod with different sidecar resources", c: createClientSet( helpers.NewSidecarPodBuilder().Build(), helpers.NewSidecarPodBuilder(). @@ -201,7 +209,7 @@ var _ = Describe("Get Pods", func() { }, }, { - name: "should ignore pod that has different resources when it has not all condition status as True", + name: "Should ignore pod that has different resources when it has not all condition status as True", c: createClientSet( helpers.NewSidecarPodBuilder(). SetConditionStatus("False"). @@ -211,7 +219,7 @@ var _ = Describe("Get Pods", func() { assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, }, { - name: "should ignore pod that has different resources when phase is not running", + name: "Should ignore pod that has different resources when phase is not running", c: createClientSet( helpers.NewSidecarPodBuilder(). SetPodStatusPhase("Pending"). @@ -221,7 +229,7 @@ var _ = Describe("Get Pods", func() { assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, }, { - name: "should ignore pod that has different resources when it has a deletion timestamp", + name: "Should ignore pod that has different resources when it has a deletion timestamp", c: createClientSet( helpers.NewSidecarPodBuilder(). SetDeletionTimestamp(time.Now()). @@ -231,7 +239,7 @@ var _ = Describe("Get Pods", func() { assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, }, { - name: "should ignore pod that with different resources when proxy container name is not in istio annotation", + name: "Should ignore pod that with different resources when proxy container name is not in istio annotation", c: createClientSet( helpers.NewSidecarPodBuilder(). SetSidecarContainerName("custom-sidecar-proxy-container-name"). @@ -246,7 +254,7 @@ var _ = Describe("Get Pods", func() { It(tt.name, func() { expectedImage := pods.NewSidecarImage("istio", "1.10.0") - podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, []filter.SidecarProxyPredicate{}, &logger) + podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, []filter.SidecarProxyPredicate{}, 10, &logger) Expect(err).NotTo(HaveOccurred()) tt.assertFunc(podList.Items) @@ -256,7 +264,6 @@ var _ = Describe("Get Pods", func() { }) var _ = Describe("GetAllInjectedPods", func() { - ctx := context.Background() tests := []struct { @@ -265,19 +272,19 @@ var _ = Describe("GetAllInjectedPods", func() { assertFunc func(val interface{}) }{ { - name: "should not return pods without istio sidecar", + name: "Should not return pods without istio sidecar", c: createClientSet(helpers.FixPodWithoutSidecar("app", "custom")), assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, }, { - name: "should return pod with istio sidecar", + name: "Should return pod with istio sidecar", c: createClientSet( helpers.NewSidecarPodBuilder().Build(), ), assertFunc: func(val interface{}) { Expect(val).To(HaveLen(1)) }, }, { - name: "should not return pod with only istio sidecar", + name: "Should not return pod with only istio sidecar", c: createClientSet( helpers.FixPodWithOnlySidecar("app", "custom"), ), diff --git a/pkg/lib/sidecars/proxy.go b/pkg/lib/sidecars/proxy.go index c09cbbe45..5f88f887a 100644 --- a/pkg/lib/sidecars/proxy.go +++ b/pkg/lib/sidecars/proxy.go @@ -12,6 +12,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +const ( + podsToRestartLimit = 10 +) + type ProxyResetter interface { ProxyReset(ctx context.Context, c client.Client, expectedImage pods.SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) ([]restart.RestartWarning, bool, error) } @@ -24,7 +28,7 @@ func NewProxyResetter() *ProxyReset { } func (p *ProxyReset) ProxyReset(ctx context.Context, c client.Client, expectedImage pods.SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) ([]restart.RestartWarning, bool, error) { - podsToRestart, err := pods.GetPodsToRestart(ctx, c, expectedImage, expectedResources, predicates, logger) + podsToRestart, err := pods.GetPodsToRestart(ctx, c, expectedImage, expectedResources, predicates, podsToRestartLimit, logger) if err != nil { return nil, false, err } @@ -34,7 +38,7 @@ func (p *ProxyReset) ProxyReset(ctx context.Context, c client.Client, expectedIm warnings, err := restart.Restart(ctx, c, podsToRestart, logger) if err != nil { - return nil, hasMorePodsToRestart, err + return nil, false, err } if !hasMorePodsToRestart { diff --git a/pkg/lib/sidecars/test/cases.go b/pkg/lib/sidecars/test/cases.go index a857381d2..a6502502d 100644 --- a/pkg/lib/sidecars/test/cases.go +++ b/pkg/lib/sidecars/test/cases.go @@ -33,7 +33,6 @@ func (s *scenario) aRestartHappens(sidecarImage string) error { } func (s *scenario) aRestartHappensWithUpdatedResources(sidecarImage string, resourceType string, cpu string, memory string) error { - resources := helpers.DefaultSidecarResources switch resourceType { diff --git a/pkg/lib/sidecars/test/features/update.feature b/pkg/lib/sidecars/test/features/update.feature index b821b0cbe..ed7e07c72 100644 --- a/pkg/lib/sidecars/test/features/update.feature +++ b/pkg/lib/sidecars/test/features/update.feature @@ -18,7 +18,7 @@ Feature: Istio upgrade And no resource that is not supposed to be deleted is deleted And all required resources are restarted And all required resources are deleted - + Scenario: Standard reconciliation Given there is a cluster with Istio "1.14.4", default injection == "false" And there are Pods with Istio "1.14.4" sidecar From 0429d2d8bfd26364bab851119391f00efdd7b529 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Wed, 10 Jul 2024 17:52:36 +0200 Subject: [PATCH 04/22] State --- pkg/lib/sidecars/pods/get_integration_test.go | 82 +++++++++ pkg/lib/sidecars/pods/get_test.go | 149 +++++++++++------ pkg/lib/sidecars/pods/suite_test.go | 157 ++++++++++++++++++ pkg/lib/sidecars/test/helpers/helpers.go | 3 +- 4 files changed, 341 insertions(+), 50 deletions(-) create mode 100644 pkg/lib/sidecars/pods/get_integration_test.go create mode 100644 pkg/lib/sidecars/pods/suite_test.go diff --git a/pkg/lib/sidecars/pods/get_integration_test.go b/pkg/lib/sidecars/pods/get_integration_test.go new file mode 100644 index 000000000..44ea38b65 --- /dev/null +++ b/pkg/lib/sidecars/pods/get_integration_test.go @@ -0,0 +1,82 @@ +package pods_test + +import ( + "context" + + . "github.com/onsi/gomega" + + . "github.com/onsi/ginkgo/v2" + corev1 "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +var _ = Describe("Pods", Serial, func() { + Context("GetPodsToRestart", func() { + It("Should respect defined limit when getting pods to restart", func() { + // given + pod := createPod("test-pod") + Expect(k8sClient.Create(context.Background(), pod)).Should(Succeed()) + + // Eventually(func(g Gomega) { + // err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(pod), pod) + // g.Expect(err).To(Not(HaveOccurred())) + // fmt.Printf("Pod: %v\n", pod.Status) + // g.Expect(pod.Status.Phase).To(Equal(v1.PodRunning)) + // }, eventuallyTimeout).Should(Succeed()) + + // // when + // expectedImage := pods.NewSidecarImage("europe-docker.pkg.dev/kyma-project/prod/external/istio", "1.22.2-distroless") + // podsToRestart, err := pods.GetPodsToRestart(context.Background(), k8sClient, expectedImage, helpers.DefaultSidecarResources, []filter.SidecarProxyPredicate{}, 1, &logr.Logger{}) + + // // then + // Expect(err).ToNot(HaveOccurred()) + // Expect(podsToRestart.Items).To(HaveLen(1)) + + // // cleanup + // Expect(k8sClient.Delete(context.Background(), pod)).Should(Succeed()) + }) + }) +}) + +func createPod(name string) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testNamespace, + Annotations: map[string]string{ + "sidecar.istio.io/status": "injected", + }, + }, + TypeMeta: metav1.TypeMeta{ + Kind: "Pod", + APIVersion: "v1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "httpbin", + Image: "docker.io/kennethreitz/httpbin", + }, + { + Name: "istio-proxy", + Image: "europe-docker.pkg.dev/kyma-project/prod/external/istio/proxyv2:1.22.1-distroless", + Resources: v1.ResourceRequirements{ + Limits: v1.ResourceList{ + v1.ResourceCPU: resource.MustParse("100m"), + v1.ResourceMemory: resource.MustParse("200Mi"), + }, + Requests: v1.ResourceList{ + v1.ResourceCPU: resource.MustParse("50m"), + v1.ResourceMemory: resource.MustParse("100Mi"), + }, + }, + }, + }, + }, + } +} diff --git a/pkg/lib/sidecars/pods/get_test.go b/pkg/lib/sidecars/pods/get_test.go index 466014f67..5bbb06a0d 100644 --- a/pkg/lib/sidecars/pods/get_test.go +++ b/pkg/lib/sidecars/pods/get_test.go @@ -2,14 +2,12 @@ package pods_test import ( "context" - "testing" + "errors" "time" "github.com/kyma-project/istio/operator/internal/filter" - "github.com/kyma-project/istio/operator/internal/tests" . "github.com/onsi/ginkgo/v2" - "github.com/onsi/ginkgo/v2/types" . "github.com/onsi/gomega" "github.com/go-logr/logr" @@ -38,42 +36,36 @@ func createClientSet(objects ...client.Object) client.Client { return fakeClient } -func TestGetPods(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "Pods Get Suite") -} - -var _ = ReportAfterSuite("custom reporter", func(report types.Report) { - tests.GenerateGinkgoJunitReport("pods-get-suite", report) -}) - -var _ = Describe("Get Pods", func() { +var _ = Describe("GetPodsToRestart", func() { ctx := context.Background() logger := logr.Discard() When("Istio image changed", func() { expectedImage := pods.NewSidecarImage("istio", "1.10.0") - tests := []struct { name string c client.Client predicates []filter.SidecarProxyPredicate limit int - assertFunc func(val interface{}) + assertFunc func(podList *v1.PodList) }{ { - name: "Should not return pods without istio sidecar", - c: createClientSet(helpers.FixPodWithoutSidecar("app", "custom")), - limit: 10, - assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, + name: "Should not return pods without istio sidecar", + c: createClientSet(helpers.FixPodWithoutSidecar("app", "custom")), + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, }, { name: "Should not return any pod when pods have correct image", c: createClientSet( helpers.NewSidecarPodBuilder().Build(), ), - limit: 10, - assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, }, { name: "Should return pod with different image repository", @@ -85,10 +77,9 @@ var _ = Describe("Get Pods", func() { Build(), ), limit: 10, - assertFunc: func(val interface{}) { - Expect(val).NotTo(BeEmpty()) - resultPods := val.([]v1.Pod) - Expect(resultPods[0].Name).To(Equal("changedSidecarPod")) + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(HaveLen(1)) + Expect(podList.Items[0].Name).To(Equal("changedSidecarPod")) }, }, { @@ -101,11 +92,9 @@ var _ = Describe("Get Pods", func() { Build(), ), limit: 10, - assertFunc: func(val interface{}) { - Expect(val).NotTo(BeEmpty()) - resultPods := val.([]v1.Pod) - Expect(resultPods[0].Name).To(Equal("changedSidecarPod")) - + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(HaveLen(1)) + Expect(podList.Items[0].Name).To(Equal("changedSidecarPod")) }, }, { @@ -116,8 +105,10 @@ var _ = Describe("Get Pods", func() { SetConditionStatus("False"). Build(), ), - limit: 10, - assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, }, { name: "Should ignore pod that has different image tag when phase is not running", @@ -127,8 +118,10 @@ var _ = Describe("Get Pods", func() { SetPodStatusPhase("Pending"). Build(), ), - limit: 10, - assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, }, { name: "Should ignore pod that has different image tag when it has a deletion timestamp", @@ -138,8 +131,10 @@ var _ = Describe("Get Pods", func() { SetDeletionTimestamp(time.Now()). Build(), ), - limit: 10, - assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, }, { name: "Should ignore pod that has different image tag when proxy container name is not in istio annotation", @@ -149,8 +144,10 @@ var _ = Describe("Get Pods", func() { SetSidecarContainerName("custom-sidecar-proxy-container-name"). Build(), ), - limit: 10, - assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, }, { name: "Should contain only one pod when there are multiple predicates that would restart the pod", @@ -162,20 +159,37 @@ var _ = Describe("Get Pods", func() { ), limit: 10, predicates: []filter.SidecarProxyPredicate{pods.NewRestartProxyPredicate(expectedImage, helpers.DefaultSidecarResources)}, - assertFunc: func(val interface{}) { - Expect(val).NotTo(BeEmpty()) - resultPods := val.([]v1.Pod) - Expect(len(resultPods)).To(Equal(1)) + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(HaveLen(1)) + }, + }, + { + name: "Should respect limit set when getting pods to restart", + c: &fakeClientWithLimit{ + createClientSet( + helpers.NewSidecarPodBuilder(). + SetName("changedSidecarPod1"). + SetSidecarImageRepository("istio/different-proxy"). + Build(), + helpers.NewSidecarPodBuilder(). + SetName("changedSidecarPod2"). + SetSidecarImageRepository("istio/different-proxy"). + Build(), + ), + 1, + }, + limit: 1, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(HaveLen(1)) + Expect(podList.Continue).ToNot(BeEmpty()) }, }, } - for _, tt := range tests { It(tt.name, func() { podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, tt.predicates, tt.limit, &logger) - Expect(err).NotTo(HaveOccurred()) - tt.assertFunc(podList.Items) + tt.assertFunc(podList) }) } }) @@ -249,13 +263,10 @@ var _ = Describe("Get Pods", func() { assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, }, } - for _, tt := range tests { It(tt.name, func() { expectedImage := pods.NewSidecarImage("istio", "1.10.0") - podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, []filter.SidecarProxyPredicate{}, 10, &logger) - Expect(err).NotTo(HaveOccurred()) tt.assertFunc(podList.Items) }) @@ -291,7 +302,6 @@ var _ = Describe("GetAllInjectedPods", func() { assertFunc: func(val interface{}) { Expect(val).To(HaveLen(0)) }, }, } - for _, tt := range tests { It(tt.name, func() { podList, err := pods.GetAllInjectedPods(ctx, tt.c) @@ -301,3 +311,44 @@ var _ = Describe("GetAllInjectedPods", func() { }) } }) + +type fakeClientWithLimit struct { + client.Client + limit int64 +} + +func (p *fakeClientWithLimit) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + foundLimitOpt := false + + for _, opt := range opts { + switch opt.(type) { + case client.Limit: + limit := opt.(client.Limit) + if int64(limit) != p.limit { + return errors.New("limit not set as expected") + } + foundLimitOpt = true + } + } + + if !foundLimitOpt { + return errors.New("limit not set when listing pods") + } + + err := p.Client.List(ctx, list, opts...) + if err != nil { + return err + } + + podList, ok := list.(*v1.PodList) + if !ok { + return errors.New("list is not a pod list") + } + + if len(podList.Items) >= int(p.limit) { + podList.Continue = "continue" + podList.Items = podList.Items[:p.limit] + } + + return nil +} diff --git a/pkg/lib/sidecars/pods/suite_test.go b/pkg/lib/sidecars/pods/suite_test.go new file mode 100644 index 000000000..3a25b58b4 --- /dev/null +++ b/pkg/lib/sidecars/pods/suite_test.go @@ -0,0 +1,157 @@ +package pods_test + +import ( + "context" + "errors" + "path/filepath" + "testing" + "time" + + "github.com/kyma-project/istio/operator/internal/tests" + . "github.com/onsi/ginkgo/v2" + "github.com/onsi/ginkgo/v2/types" + . "github.com/onsi/gomega" + networkingv1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" + networkingv1beta1 "istio.io/client-go/pkg/apis/networking/v1beta1" + securityv1beta1 "istio.io/client-go/pkg/apis/security/v1beta1" + appsv1 "k8s.io/api/apps/v1" + "k8s.io/api/apps/v1beta1" + autoscalingv2 "k8s.io/api/autoscaling/v2" + schedulingv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + policyv1 "k8s.io/api/policy/v1" + rbacv1 "k8s.io/api/rbac/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/util/retry" + ctrl "sigs.k8s.io/controller-runtime" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + //+kubebuilder:scaffold:imports +) + +const ( + //eventuallyTimeout = time.Second * 30 + testNamespace = "test-namespace" +) + +var ( + cfg *rest.Config + k8sClient client.Client + testEnv *envtest.Environment + ctx context.Context + cancel context.CancelFunc +) + +func TestPods(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Pods Suite") +} + +var _ = ReportAfterSuite("custom reporter", func(report types.Report) { + tests.GenerateGinkgoJunitReport("pods-suite", report) +}) + +var _ = BeforeSuite(func() { + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + ctx, cancel = context.WithCancel(context.Background()) + + By("bootstrapping test environment") + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "..", "config", "crd", "bases"), filepath.Join("..", "..", "..", "..", "hack")}, + ErrorIfCRDPathMissing: true, + } + + var err error + // cfg is defined in this file globally. + cfg, err = testEnv.Start() + Expect(err).NotTo(HaveOccurred()) + Expect(cfg).NotTo(BeNil()) + + s := getTestScheme() + + //+kubebuilder:scaffold:scheme + + k8sClient, err = client.New(cfg, client.Options{Scheme: s}) + Expect(err).NotTo(HaveOccurred()) + Expect(k8sClient).NotTo(BeNil()) + + createCommonTestResources(k8sClient) + + mgr, err := ctrl.NewManager(cfg, ctrl.Options{ + Scheme: s, + Metrics: metricsserver.Options{ + BindAddress: "0", + }, + }) + Expect(err).NotTo(HaveOccurred()) + + go func() { + defer GinkgoRecover() + err := mgr.Start(ctx) + // A workaround for DeadlineExceeded error is introduced, since this started occurring during the teardown + // after adding Oathkeeper reconciliation. + if !errors.Is(err, context.DeadlineExceeded) { + Expect(err).Should(Succeed()) + } else { + println("Context deadline exceeded during tearing down", err.Error()) + } + }() +}) + +var _ = AfterSuite(func() { + /* + Provided solution for timeout issue waiting for kubeapiserver + https://github.com/kubernetes-sigs/controller-runtime/issues/1571#issuecomment-1005575071 + */ + cancel() + By("Tearing down the test environment") + err := retry.OnError(wait.Backoff{ + Duration: 500 * time.Millisecond, + Steps: 150, + }, func(err error) bool { + return true + }, func() error { + return testEnv.Stop() + }) + Expect(err).NotTo(HaveOccurred()) +}) + +func createCommonTestResources(k8sClient client.Client) { + kymaSystemNs := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: testNamespace}, + Spec: corev1.NamespaceSpec{}, + } + Expect(k8sClient.Create(context.TODO(), kymaSystemNs)).Should(Succeed()) + + istioSystemNs := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "istio-system"}, + Spec: corev1.NamespaceSpec{}, + } + Expect(k8sClient.Create(context.TODO(), istioSystemNs)).Should(Succeed()) +} + +func getTestScheme() *runtime.Scheme { + s := runtime.NewScheme() + utilruntime.Must(corev1.AddToScheme(s)) + utilruntime.Must(v1beta1.AddToScheme(s)) + utilruntime.Must(appsv1.AddToScheme(s)) + utilruntime.Must(rbacv1.AddToScheme(s)) + utilruntime.Must(policyv1.AddToScheme(s)) + utilruntime.Must(autoscalingv2.AddToScheme(s)) + utilruntime.Must(securityv1beta1.AddToScheme(s)) + utilruntime.Must(schedulingv1.AddToScheme(s)) + utilruntime.Must(apiextensionsv1.AddToScheme(s)) + utilruntime.Must(networkingv1alpha3.AddToScheme(s)) + utilruntime.Must(networkingv1beta1.AddToScheme(s)) + + return s +} diff --git a/pkg/lib/sidecars/test/helpers/helpers.go b/pkg/lib/sidecars/test/helpers/helpers.go index fa267cd5f..cc7b4a2ed 100644 --- a/pkg/lib/sidecars/test/helpers/helpers.go +++ b/pkg/lib/sidecars/test/helpers/helpers.go @@ -2,10 +2,11 @@ package helpers import ( "fmt" - "k8s.io/apimachinery/pkg/api/resource" "reflect" "time" + "k8s.io/apimachinery/pkg/api/resource" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) From 4fde4033d4dc2f4c7d141f8e85756aed1bbb0ac2 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Wed, 10 Jul 2024 18:57:50 +0200 Subject: [PATCH 05/22] State --- pkg/lib/sidecars/pods/get.go | 13 +- pkg/lib/sidecars/pods/get_integration_test.go | 82 ---- pkg/lib/sidecars/pods/get_test.go | 393 ++++++++++-------- pkg/lib/sidecars/pods/suite_test.go | 157 ------- 4 files changed, 242 insertions(+), 403 deletions(-) delete mode 100644 pkg/lib/sidecars/pods/get_integration_test.go delete mode 100644 pkg/lib/sidecars/pods/suite_test.go diff --git a/pkg/lib/sidecars/pods/get.go b/pkg/lib/sidecars/pods/get.go index 2dfea8f48..ab54afb75 100644 --- a/pkg/lib/sidecars/pods/get.go +++ b/pkg/lib/sidecars/pods/get.go @@ -81,11 +81,16 @@ func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage Sideca podsToRestart := &v1.PodList{} for while := true; while; { + fmt.Print("getting pods\n") + fmt.Printf("continue tocken beginning of loop: %v\n", podsToRestart.Continue) podsWithSidecar, err := getSidecarPods(ctx, c, logger, limit, podsToRestart.Continue) if err != nil { return nil, err } - + fmt.Print("got:\n") + for _, pod := range podsWithSidecar.Items { + fmt.Printf("pod: %s\n", pod.Name) + } for _, pod := range podsWithSidecar.Items { for _, predicate := range predicates { if predicate.RequiresProxyRestart(pod) { @@ -94,8 +99,12 @@ func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage Sideca } } } - + fmt.Print("needs to restart\n") + for _, pod := range podsToRestart.Items { + fmt.Printf("pod: %s\n", pod.Name) + } podsToRestart.Continue = podsWithSidecar.Continue + fmt.Printf("continue tocken end of loop: %v\n", podsToRestart.Continue) while = len(podsToRestart.Items) < limit && podsToRestart.Continue != "" } diff --git a/pkg/lib/sidecars/pods/get_integration_test.go b/pkg/lib/sidecars/pods/get_integration_test.go deleted file mode 100644 index 44ea38b65..000000000 --- a/pkg/lib/sidecars/pods/get_integration_test.go +++ /dev/null @@ -1,82 +0,0 @@ -package pods_test - -import ( - "context" - - . "github.com/onsi/gomega" - - . "github.com/onsi/ginkgo/v2" - corev1 "k8s.io/api/core/v1" - v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/resource" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -var _ = Describe("Pods", Serial, func() { - Context("GetPodsToRestart", func() { - It("Should respect defined limit when getting pods to restart", func() { - // given - pod := createPod("test-pod") - Expect(k8sClient.Create(context.Background(), pod)).Should(Succeed()) - - // Eventually(func(g Gomega) { - // err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(pod), pod) - // g.Expect(err).To(Not(HaveOccurred())) - // fmt.Printf("Pod: %v\n", pod.Status) - // g.Expect(pod.Status.Phase).To(Equal(v1.PodRunning)) - // }, eventuallyTimeout).Should(Succeed()) - - // // when - // expectedImage := pods.NewSidecarImage("europe-docker.pkg.dev/kyma-project/prod/external/istio", "1.22.2-distroless") - // podsToRestart, err := pods.GetPodsToRestart(context.Background(), k8sClient, expectedImage, helpers.DefaultSidecarResources, []filter.SidecarProxyPredicate{}, 1, &logr.Logger{}) - - // // then - // Expect(err).ToNot(HaveOccurred()) - // Expect(podsToRestart.Items).To(HaveLen(1)) - - // // cleanup - // Expect(k8sClient.Delete(context.Background(), pod)).Should(Succeed()) - }) - }) -}) - -func createPod(name string) *corev1.Pod { - return &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: testNamespace, - Annotations: map[string]string{ - "sidecar.istio.io/status": "injected", - }, - }, - TypeMeta: metav1.TypeMeta{ - Kind: "Pod", - APIVersion: "v1", - }, - Status: corev1.PodStatus{ - Phase: corev1.PodRunning, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "httpbin", - Image: "docker.io/kennethreitz/httpbin", - }, - { - Name: "istio-proxy", - Image: "europe-docker.pkg.dev/kyma-project/prod/external/istio/proxyv2:1.22.1-distroless", - Resources: v1.ResourceRequirements{ - Limits: v1.ResourceList{ - v1.ResourceCPU: resource.MustParse("100m"), - v1.ResourceMemory: resource.MustParse("200Mi"), - }, - Requests: v1.ResourceList{ - v1.ResourceCPU: resource.MustParse("50m"), - v1.ResourceMemory: resource.MustParse("100Mi"), - }, - }, - }, - }, - }, - } -} diff --git a/pkg/lib/sidecars/pods/get_test.go b/pkg/lib/sidecars/pods/get_test.go index 5bbb06a0d..42f539b7f 100644 --- a/pkg/lib/sidecars/pods/get_test.go +++ b/pkg/lib/sidecars/pods/get_test.go @@ -3,11 +3,15 @@ package pods_test import ( "context" "errors" + "fmt" + "testing" "time" "github.com/kyma-project/istio/operator/internal/filter" + "github.com/kyma-project/istio/operator/internal/tests" . "github.com/onsi/ginkgo/v2" + "github.com/onsi/ginkgo/v2/types" . "github.com/onsi/gomega" "github.com/go-logr/logr" @@ -21,21 +25,15 @@ import ( "github.com/kyma-project/istio/operator/pkg/lib/sidecars/test/helpers" ) -func createClientSet(objects ...client.Object) client.Client { - err := v1alpha1.AddToScheme(scheme.Scheme) - Expect(err).NotTo(HaveOccurred()) - err = v1.AddToScheme(scheme.Scheme) - Expect(err).NotTo(HaveOccurred()) - - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme.Scheme). - WithObjects(objects...). - WithIndex(&v1.Pod{}, "status.phase", helpers.FakePodStatusPhaseIndexer). - Build() - - return fakeClient +func TestPods(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Pods Get Suite") } +var _ = ReportAfterSuite("custom reporter", func(report types.Report) { + tests.GenerateGinkgoJunitReport("pods-get-suite", report) +}) + var _ = Describe("GetPodsToRestart", func() { ctx := context.Background() logger := logr.Discard() @@ -49,144 +47,165 @@ var _ = Describe("GetPodsToRestart", func() { limit int assertFunc func(podList *v1.PodList) }{ + // { + // name: "Should not return pods without istio sidecar", + // c: createClientSet(helpers.FixPodWithoutSidecar("app", "custom")), + // limit: 10, + // assertFunc: func(podList *v1.PodList) { + // Expect(podList.Items).To(BeEmpty()) + // }, + // }, + // { + // name: "Should not return any pod when pods have correct image", + // c: createClientSet( + // helpers.NewSidecarPodBuilder().Build(), + // ), + // limit: 10, + // assertFunc: func(podList *v1.PodList) { + // Expect(podList.Items).To(BeEmpty()) + // }, + // }, + // { + // name: "Should return pod with different image repository", + // c: createClientSet( + // helpers.NewSidecarPodBuilder().Build(), + // helpers.NewSidecarPodBuilder(). + // SetName("changedSidecarPod"). + // SetSidecarImageRepository("istio/different-proxy"). + // Build(), + // ), + // limit: 10, + // assertFunc: func(podList *v1.PodList) { + // Expect(podList.Items).To(HaveLen(1)) + // Expect(podList.Items[0].Name).To(Equal("changedSidecarPod")) + // }, + // }, + // { + // name: "Should return pod with different image tag", + // c: createClientSet( + // helpers.NewSidecarPodBuilder().Build(), + // helpers.NewSidecarPodBuilder(). + // SetName("changedSidecarPod"). + // SetSidecarImageTag("1.11.0"). + // Build(), + // ), + // limit: 10, + // assertFunc: func(podList *v1.PodList) { + // Expect(podList.Items).To(HaveLen(1)) + // Expect(podList.Items[0].Name).To(Equal("changedSidecarPod")) + // }, + // }, + // { + // name: "Should ignore pod that has different image tag when it has not all condition status as True", + // c: createClientSet( + // helpers.NewSidecarPodBuilder(). + // SetSidecarImageTag("1.12.0"). + // SetConditionStatus("False"). + // Build(), + // ), + // limit: 10, + // assertFunc: func(podList *v1.PodList) { + // Expect(podList.Items).To(BeEmpty()) + // }, + // }, + // { + // name: "Should ignore pod that has different image tag when phase is not running", + // c: createClientSet( + // helpers.NewSidecarPodBuilder(). + // SetSidecarImageTag("1.12.0"). + // SetPodStatusPhase("Pending"). + // Build(), + // ), + // limit: 10, + // assertFunc: func(podList *v1.PodList) { + // Expect(podList.Items).To(BeEmpty()) + // }, + // }, + // { + // name: "Should ignore pod that has different image tag when it has a deletion timestamp", + // c: createClientSet( + // helpers.NewSidecarPodBuilder(). + // SetSidecarImageTag("1.12.0"). + // SetDeletionTimestamp(time.Now()). + // Build(), + // ), + // limit: 10, + // assertFunc: func(podList *v1.PodList) { + // Expect(podList.Items).To(BeEmpty()) + // }, + // }, + // { + // name: "Should ignore pod that has different image tag when proxy container name is not in istio annotation", + // c: createClientSet( + // helpers.NewSidecarPodBuilder(). + // SetSidecarImageTag("1.12.0"). + // SetSidecarContainerName("custom-sidecar-proxy-container-name"). + // Build(), + // ), + // limit: 10, + // assertFunc: func(podList *v1.PodList) { + // Expect(podList.Items).To(BeEmpty()) + // }, + // }, + // { + // name: "Should contain only one pod when there are multiple predicates that would restart the pod", + // c: createClientSet( + // helpers.NewSidecarPodBuilder(). + // SetName("changedSidecarPod"). + // SetSidecarImageRepository("istio/different-proxy"). + // Build(), + // ), + // limit: 10, + // predicates: []filter.SidecarProxyPredicate{pods.NewRestartProxyPredicate(expectedImage, helpers.DefaultSidecarResources)}, + // assertFunc: func(podList *v1.PodList) { + // Expect(podList.Items).To(HaveLen(1)) + // }, + // }, + // { + // name: "Should respect limit set when getting pods to restart", + // c: NewFakeClientWithLimit( + // createClientSet( + // helpers.NewSidecarPodBuilder(). + // SetName("changedSidecarPod1"). + // SetSidecarImageRepository("istio/different-proxy"). + // Build(), + // helpers.NewSidecarPodBuilder(). + // SetName("changedSidecarPod2"). + // SetSidecarImageRepository("istio/different-proxy"). + // Build(), + // ), 1), + // limit: 1, + // assertFunc: func(podList *v1.PodList) { + // Expect(podList.Items).To(HaveLen(1)) + // Expect(podList.Items[0].Name).To(Equal("changedSidecarPod1")) + // Expect(podList.Continue).To(Equal("continue")) + // }, + // }, { - name: "Should not return pods without istio sidecar", - c: createClientSet(helpers.FixPodWithoutSidecar("app", "custom")), - limit: 10, - assertFunc: func(podList *v1.PodList) { - Expect(podList.Items).To(BeEmpty()) - }, - }, - { - name: "Should not return any pod when pods have correct image", - c: createClientSet( - helpers.NewSidecarPodBuilder().Build(), - ), - limit: 10, - assertFunc: func(podList *v1.PodList) { - Expect(podList.Items).To(BeEmpty()) - }, - }, - { - name: "Should return pod with different image repository", - c: createClientSet( - helpers.NewSidecarPodBuilder().Build(), - helpers.NewSidecarPodBuilder(). - SetName("changedSidecarPod"). - SetSidecarImageRepository("istio/different-proxy"). - Build(), - ), - limit: 10, - assertFunc: func(podList *v1.PodList) { - Expect(podList.Items).To(HaveLen(1)) - Expect(podList.Items[0].Name).To(Equal("changedSidecarPod")) - }, - }, - { - name: "Should return pod with different image tag", - c: createClientSet( - helpers.NewSidecarPodBuilder().Build(), - helpers.NewSidecarPodBuilder(). - SetName("changedSidecarPod"). - SetSidecarImageTag("1.11.0"). - Build(), - ), - limit: 10, - assertFunc: func(podList *v1.PodList) { - Expect(podList.Items).To(HaveLen(1)) - Expect(podList.Items[0].Name).To(Equal("changedSidecarPod")) - }, - }, - { - name: "Should ignore pod that has different image tag when it has not all condition status as True", - c: createClientSet( - helpers.NewSidecarPodBuilder(). - SetSidecarImageTag("1.12.0"). - SetConditionStatus("False"). - Build(), - ), - limit: 10, - assertFunc: func(podList *v1.PodList) { - Expect(podList.Items).To(BeEmpty()) - }, - }, - { - name: "Should ignore pod that has different image tag when phase is not running", - c: createClientSet( - helpers.NewSidecarPodBuilder(). - SetSidecarImageTag("1.12.0"). - SetPodStatusPhase("Pending"). - Build(), - ), - limit: 10, - assertFunc: func(podList *v1.PodList) { - Expect(podList.Items).To(BeEmpty()) - }, - }, - { - name: "Should ignore pod that has different image tag when it has a deletion timestamp", - c: createClientSet( - helpers.NewSidecarPodBuilder(). - SetSidecarImageTag("1.12.0"). - SetDeletionTimestamp(time.Now()). - Build(), - ), - limit: 10, - assertFunc: func(podList *v1.PodList) { - Expect(podList.Items).To(BeEmpty()) - }, - }, - { - name: "Should ignore pod that has different image tag when proxy container name is not in istio annotation", - c: createClientSet( - helpers.NewSidecarPodBuilder(). - SetSidecarImageTag("1.12.0"). - SetSidecarContainerName("custom-sidecar-proxy-container-name"). - Build(), - ), - limit: 10, - assertFunc: func(podList *v1.PodList) { - Expect(podList.Items).To(BeEmpty()) - }, - }, - { - name: "Should contain only one pod when there are multiple predicates that would restart the pod", - c: createClientSet( - helpers.NewSidecarPodBuilder(). - SetName("changedSidecarPod"). - SetSidecarImageRepository("istio/different-proxy"). - Build(), - ), - limit: 10, - predicates: []filter.SidecarProxyPredicate{pods.NewRestartProxyPredicate(expectedImage, helpers.DefaultSidecarResources)}, - assertFunc: func(podList *v1.PodList) { - Expect(podList.Items).To(HaveLen(1)) - }, - }, - { - name: "Should respect limit set when getting pods to restart", - c: &fakeClientWithLimit{ + name: "Should respect limit and use continue token to obtain rest of pods when getting pods to restart", + c: NewFakeClientWithLimit( createClientSet( helpers.NewSidecarPodBuilder(). SetName("changedSidecarPod1"). SetSidecarImageRepository("istio/different-proxy"). Build(), + helpers.NewSidecarPodBuilder().Build(), helpers.NewSidecarPodBuilder(). SetName("changedSidecarPod2"). SetSidecarImageRepository("istio/different-proxy"). Build(), - ), - 1, - }, - limit: 1, + ), 3), + limit: 3, assertFunc: func(podList *v1.PodList) { - Expect(podList.Items).To(HaveLen(1)) - Expect(podList.Continue).ToNot(BeEmpty()) + Expect(podList.Items).To(HaveLen(2)) + Expect(podList.Items[0].Name).To(Equal("changedSidecarPod1")) + Expect(podList.Items[1].Name).To(Equal("changedSidecarPod2")) + Expect(podList.Continue).To(BeEmpty()) }, }, } for _, tt := range tests { - It(tt.name, func() { + FIt(tt.name, func() { podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, tt.predicates, tt.limit, &logger) Expect(err).NotTo(HaveOccurred()) tt.assertFunc(podList) @@ -198,14 +217,14 @@ var _ = Describe("GetPodsToRestart", func() { tests := []struct { name string c client.Client - assertFunc func(val interface{}) + assertFunc func(podList *v1.PodList) }{ { name: "Should not return any pod when pods have same resources", c: createClientSet( helpers.NewSidecarPodBuilder().Build(), ), - assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, + assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(BeEmpty()) }, }, { name: "Should return pod with different sidecar resources", @@ -216,10 +235,9 @@ var _ = Describe("GetPodsToRestart", func() { SetCpuRequest("400m"). Build(), ), - assertFunc: func(val interface{}) { - Expect(val).NotTo(BeEmpty()) - resultPods := val.([]v1.Pod) - Expect(resultPods[0].Name).To(Equal("changedSidecarPod")) + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(HaveLen(1)) + Expect(podList.Items[0].Name).To(Equal("changedSidecarPod")) }, }, { @@ -230,7 +248,7 @@ var _ = Describe("GetPodsToRestart", func() { SetCpuRequest("400m"). Build(), ), - assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, + assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(BeEmpty()) }, }, { name: "Should ignore pod that has different resources when phase is not running", @@ -240,7 +258,7 @@ var _ = Describe("GetPodsToRestart", func() { SetCpuRequest("400m"). Build(), ), - assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, + assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(BeEmpty()) }, }, { name: "Should ignore pod that has different resources when it has a deletion timestamp", @@ -250,7 +268,7 @@ var _ = Describe("GetPodsToRestart", func() { SetCpuRequest("400m"). Build(), ), - assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, + assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(BeEmpty()) }, }, { name: "Should ignore pod that with different resources when proxy container name is not in istio annotation", @@ -260,7 +278,7 @@ var _ = Describe("GetPodsToRestart", func() { SetCpuRequest("400m"). Build(), ), - assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, + assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(BeEmpty()) }, }, } for _, tt := range tests { @@ -268,7 +286,7 @@ var _ = Describe("GetPodsToRestart", func() { expectedImage := pods.NewSidecarImage("istio", "1.10.0") podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, []filter.SidecarProxyPredicate{}, 10, &logger) Expect(err).NotTo(HaveOccurred()) - tt.assertFunc(podList.Items) + tt.assertFunc(podList) }) } }) @@ -280,45 +298,73 @@ var _ = Describe("GetAllInjectedPods", func() { tests := []struct { name string c client.Client - assertFunc func(val interface{}) + assertFunc func(podList *v1.PodList) }{ { name: "Should not return pods without istio sidecar", c: createClientSet(helpers.FixPodWithoutSidecar("app", "custom")), - assertFunc: func(val interface{}) { Expect(val).To(BeEmpty()) }, + assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(BeEmpty()) }, }, { name: "Should return pod with istio sidecar", c: createClientSet( helpers.NewSidecarPodBuilder().Build(), ), - assertFunc: func(val interface{}) { Expect(val).To(HaveLen(1)) }, + assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(HaveLen(1)) }, }, { name: "Should not return pod with only istio sidecar", c: createClientSet( helpers.FixPodWithOnlySidecar("app", "custom"), ), - assertFunc: func(val interface{}) { Expect(val).To(HaveLen(0)) }, + assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(HaveLen(0)) }, }, } for _, tt := range tests { It(tt.name, func() { podList, err := pods.GetAllInjectedPods(ctx, tt.c) - Expect(err).NotTo(HaveOccurred()) - tt.assertFunc(podList.Items) + tt.assertFunc(podList) }) } }) +func createClientSet(objects ...client.Object) client.Client { + err := v1alpha1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) + err = v1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithObjects(objects...). + WithIndex(&v1.Pod{}, "status.phase", helpers.FakePodStatusPhaseIndexer). + Build() + + return fakeClient +} + type fakeClientWithLimit struct { client.Client - limit int64 + limit int64 + callCount int + continueTokenOnNextCall bool +} + +func NewFakeClientWithLimit(c client.Client, limit int64) *fakeClientWithLimit { + return &fakeClientWithLimit{ + Client: c, + limit: limit, + callCount: 0, + continueTokenOnNextCall: false, + } } func (p *fakeClientWithLimit) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + p.callCount++ + foundLimitOpt := false + continueToken := "" for _, opt := range opts { switch opt.(type) { @@ -328,6 +374,8 @@ func (p *fakeClientWithLimit) List(ctx context.Context, list client.ObjectList, return errors.New("limit not set as expected") } foundLimitOpt = true + case client.Continue: + continueToken = string(opt.(client.Continue)) } } @@ -335,19 +383,40 @@ func (p *fakeClientWithLimit) List(ctx context.Context, list client.ObjectList, return errors.New("limit not set when listing pods") } - err := p.Client.List(ctx, list, opts...) - if err != nil { - return err - } - podList, ok := list.(*v1.PodList) if !ok { return errors.New("list is not a pod list") } - if len(podList.Items) >= int(p.limit) { - podList.Continue = "continue" - podList.Items = podList.Items[:p.limit] + fmt.Printf("call number: %d\n", p.callCount) + fmt.Printf("continue token: %s\n", continueToken) + + switch p.callCount { + case 1: + if podList.Continue != "" { + return errors.New("continue token should be empty on the first call") + } + case 2: + if p.continueTokenOnNextCall && continueToken != "continue" { + return errors.New("continue token should be set correctly on the second call") + } + } + + err := p.Client.List(ctx, list, opts...) + if err != nil { + return err + } + + if len(podList.Items) > int(p.limit) { + if podList.Continue == "" { + podList.Continue = "continue" + podList.Items = podList.Items[:p.limit] + p.continueTokenOnNextCall = true + } else { + podList.Items = podList.Items[p.limit:] + podList.Continue = "" + p.continueTokenOnNextCall = false + } } return nil diff --git a/pkg/lib/sidecars/pods/suite_test.go b/pkg/lib/sidecars/pods/suite_test.go deleted file mode 100644 index 3a25b58b4..000000000 --- a/pkg/lib/sidecars/pods/suite_test.go +++ /dev/null @@ -1,157 +0,0 @@ -package pods_test - -import ( - "context" - "errors" - "path/filepath" - "testing" - "time" - - "github.com/kyma-project/istio/operator/internal/tests" - . "github.com/onsi/ginkgo/v2" - "github.com/onsi/ginkgo/v2/types" - . "github.com/onsi/gomega" - networkingv1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" - networkingv1beta1 "istio.io/client-go/pkg/apis/networking/v1beta1" - securityv1beta1 "istio.io/client-go/pkg/apis/security/v1beta1" - appsv1 "k8s.io/api/apps/v1" - "k8s.io/api/apps/v1beta1" - autoscalingv2 "k8s.io/api/autoscaling/v2" - schedulingv1 "k8s.io/api/batch/v1" - corev1 "k8s.io/api/core/v1" - policyv1 "k8s.io/api/policy/v1" - rbacv1 "k8s.io/api/rbac/v1" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - utilruntime "k8s.io/apimachinery/pkg/util/runtime" - "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/client-go/util/retry" - ctrl "sigs.k8s.io/controller-runtime" - metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" - - "k8s.io/client-go/rest" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/envtest" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/log/zap" - //+kubebuilder:scaffold:imports -) - -const ( - //eventuallyTimeout = time.Second * 30 - testNamespace = "test-namespace" -) - -var ( - cfg *rest.Config - k8sClient client.Client - testEnv *envtest.Environment - ctx context.Context - cancel context.CancelFunc -) - -func TestPods(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "Pods Suite") -} - -var _ = ReportAfterSuite("custom reporter", func(report types.Report) { - tests.GenerateGinkgoJunitReport("pods-suite", report) -}) - -var _ = BeforeSuite(func() { - logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) - ctx, cancel = context.WithCancel(context.Background()) - - By("bootstrapping test environment") - testEnv = &envtest.Environment{ - CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "..", "config", "crd", "bases"), filepath.Join("..", "..", "..", "..", "hack")}, - ErrorIfCRDPathMissing: true, - } - - var err error - // cfg is defined in this file globally. - cfg, err = testEnv.Start() - Expect(err).NotTo(HaveOccurred()) - Expect(cfg).NotTo(BeNil()) - - s := getTestScheme() - - //+kubebuilder:scaffold:scheme - - k8sClient, err = client.New(cfg, client.Options{Scheme: s}) - Expect(err).NotTo(HaveOccurred()) - Expect(k8sClient).NotTo(BeNil()) - - createCommonTestResources(k8sClient) - - mgr, err := ctrl.NewManager(cfg, ctrl.Options{ - Scheme: s, - Metrics: metricsserver.Options{ - BindAddress: "0", - }, - }) - Expect(err).NotTo(HaveOccurred()) - - go func() { - defer GinkgoRecover() - err := mgr.Start(ctx) - // A workaround for DeadlineExceeded error is introduced, since this started occurring during the teardown - // after adding Oathkeeper reconciliation. - if !errors.Is(err, context.DeadlineExceeded) { - Expect(err).Should(Succeed()) - } else { - println("Context deadline exceeded during tearing down", err.Error()) - } - }() -}) - -var _ = AfterSuite(func() { - /* - Provided solution for timeout issue waiting for kubeapiserver - https://github.com/kubernetes-sigs/controller-runtime/issues/1571#issuecomment-1005575071 - */ - cancel() - By("Tearing down the test environment") - err := retry.OnError(wait.Backoff{ - Duration: 500 * time.Millisecond, - Steps: 150, - }, func(err error) bool { - return true - }, func() error { - return testEnv.Stop() - }) - Expect(err).NotTo(HaveOccurred()) -}) - -func createCommonTestResources(k8sClient client.Client) { - kymaSystemNs := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{Name: testNamespace}, - Spec: corev1.NamespaceSpec{}, - } - Expect(k8sClient.Create(context.TODO(), kymaSystemNs)).Should(Succeed()) - - istioSystemNs := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{Name: "istio-system"}, - Spec: corev1.NamespaceSpec{}, - } - Expect(k8sClient.Create(context.TODO(), istioSystemNs)).Should(Succeed()) -} - -func getTestScheme() *runtime.Scheme { - s := runtime.NewScheme() - utilruntime.Must(corev1.AddToScheme(s)) - utilruntime.Must(v1beta1.AddToScheme(s)) - utilruntime.Must(appsv1.AddToScheme(s)) - utilruntime.Must(rbacv1.AddToScheme(s)) - utilruntime.Must(policyv1.AddToScheme(s)) - utilruntime.Must(autoscalingv2.AddToScheme(s)) - utilruntime.Must(securityv1beta1.AddToScheme(s)) - utilruntime.Must(schedulingv1.AddToScheme(s)) - utilruntime.Must(apiextensionsv1.AddToScheme(s)) - utilruntime.Must(networkingv1alpha3.AddToScheme(s)) - utilruntime.Must(networkingv1beta1.AddToScheme(s)) - - return s -} From 542bb3fa65f270f5adf811e397f29704a7a119bb Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 11 Jul 2024 09:51:15 +0200 Subject: [PATCH 06/22] Pods get refined with mocked test with limit --- pkg/lib/sidecars/pods/get.go | 11 - pkg/lib/sidecars/pods/get_test.go | 321 +++++++++++++++--------------- 2 files changed, 158 insertions(+), 174 deletions(-) diff --git a/pkg/lib/sidecars/pods/get.go b/pkg/lib/sidecars/pods/get.go index ab54afb75..ab2f4d1e7 100644 --- a/pkg/lib/sidecars/pods/get.go +++ b/pkg/lib/sidecars/pods/get.go @@ -81,16 +81,10 @@ func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage Sideca podsToRestart := &v1.PodList{} for while := true; while; { - fmt.Print("getting pods\n") - fmt.Printf("continue tocken beginning of loop: %v\n", podsToRestart.Continue) podsWithSidecar, err := getSidecarPods(ctx, c, logger, limit, podsToRestart.Continue) if err != nil { return nil, err } - fmt.Print("got:\n") - for _, pod := range podsWithSidecar.Items { - fmt.Printf("pod: %s\n", pod.Name) - } for _, pod := range podsWithSidecar.Items { for _, predicate := range predicates { if predicate.RequiresProxyRestart(pod) { @@ -99,12 +93,7 @@ func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage Sideca } } } - fmt.Print("needs to restart\n") - for _, pod := range podsToRestart.Items { - fmt.Printf("pod: %s\n", pod.Name) - } podsToRestart.Continue = podsWithSidecar.Continue - fmt.Printf("continue tocken end of loop: %v\n", podsToRestart.Continue) while = len(podsToRestart.Items) < limit && podsToRestart.Continue != "" } diff --git a/pkg/lib/sidecars/pods/get_test.go b/pkg/lib/sidecars/pods/get_test.go index 42f539b7f..1d973b83e 100644 --- a/pkg/lib/sidecars/pods/get_test.go +++ b/pkg/lib/sidecars/pods/get_test.go @@ -3,7 +3,6 @@ package pods_test import ( "context" "errors" - "fmt" "testing" "time" @@ -47,155 +46,154 @@ var _ = Describe("GetPodsToRestart", func() { limit int assertFunc func(podList *v1.PodList) }{ - // { - // name: "Should not return pods without istio sidecar", - // c: createClientSet(helpers.FixPodWithoutSidecar("app", "custom")), - // limit: 10, - // assertFunc: func(podList *v1.PodList) { - // Expect(podList.Items).To(BeEmpty()) - // }, - // }, - // { - // name: "Should not return any pod when pods have correct image", - // c: createClientSet( - // helpers.NewSidecarPodBuilder().Build(), - // ), - // limit: 10, - // assertFunc: func(podList *v1.PodList) { - // Expect(podList.Items).To(BeEmpty()) - // }, - // }, - // { - // name: "Should return pod with different image repository", - // c: createClientSet( - // helpers.NewSidecarPodBuilder().Build(), - // helpers.NewSidecarPodBuilder(). - // SetName("changedSidecarPod"). - // SetSidecarImageRepository("istio/different-proxy"). - // Build(), - // ), - // limit: 10, - // assertFunc: func(podList *v1.PodList) { - // Expect(podList.Items).To(HaveLen(1)) - // Expect(podList.Items[0].Name).To(Equal("changedSidecarPod")) - // }, - // }, - // { - // name: "Should return pod with different image tag", - // c: createClientSet( - // helpers.NewSidecarPodBuilder().Build(), - // helpers.NewSidecarPodBuilder(). - // SetName("changedSidecarPod"). - // SetSidecarImageTag("1.11.0"). - // Build(), - // ), - // limit: 10, - // assertFunc: func(podList *v1.PodList) { - // Expect(podList.Items).To(HaveLen(1)) - // Expect(podList.Items[0].Name).To(Equal("changedSidecarPod")) - // }, - // }, - // { - // name: "Should ignore pod that has different image tag when it has not all condition status as True", - // c: createClientSet( - // helpers.NewSidecarPodBuilder(). - // SetSidecarImageTag("1.12.0"). - // SetConditionStatus("False"). - // Build(), - // ), - // limit: 10, - // assertFunc: func(podList *v1.PodList) { - // Expect(podList.Items).To(BeEmpty()) - // }, - // }, - // { - // name: "Should ignore pod that has different image tag when phase is not running", - // c: createClientSet( - // helpers.NewSidecarPodBuilder(). - // SetSidecarImageTag("1.12.0"). - // SetPodStatusPhase("Pending"). - // Build(), - // ), - // limit: 10, - // assertFunc: func(podList *v1.PodList) { - // Expect(podList.Items).To(BeEmpty()) - // }, - // }, - // { - // name: "Should ignore pod that has different image tag when it has a deletion timestamp", - // c: createClientSet( - // helpers.NewSidecarPodBuilder(). - // SetSidecarImageTag("1.12.0"). - // SetDeletionTimestamp(time.Now()). - // Build(), - // ), - // limit: 10, - // assertFunc: func(podList *v1.PodList) { - // Expect(podList.Items).To(BeEmpty()) - // }, - // }, - // { - // name: "Should ignore pod that has different image tag when proxy container name is not in istio annotation", - // c: createClientSet( - // helpers.NewSidecarPodBuilder(). - // SetSidecarImageTag("1.12.0"). - // SetSidecarContainerName("custom-sidecar-proxy-container-name"). - // Build(), - // ), - // limit: 10, - // assertFunc: func(podList *v1.PodList) { - // Expect(podList.Items).To(BeEmpty()) - // }, - // }, - // { - // name: "Should contain only one pod when there are multiple predicates that would restart the pod", - // c: createClientSet( - // helpers.NewSidecarPodBuilder(). - // SetName("changedSidecarPod"). - // SetSidecarImageRepository("istio/different-proxy"). - // Build(), - // ), - // limit: 10, - // predicates: []filter.SidecarProxyPredicate{pods.NewRestartProxyPredicate(expectedImage, helpers.DefaultSidecarResources)}, - // assertFunc: func(podList *v1.PodList) { - // Expect(podList.Items).To(HaveLen(1)) - // }, - // }, - // { - // name: "Should respect limit set when getting pods to restart", - // c: NewFakeClientWithLimit( - // createClientSet( - // helpers.NewSidecarPodBuilder(). - // SetName("changedSidecarPod1"). - // SetSidecarImageRepository("istio/different-proxy"). - // Build(), - // helpers.NewSidecarPodBuilder(). - // SetName("changedSidecarPod2"). - // SetSidecarImageRepository("istio/different-proxy"). - // Build(), - // ), 1), - // limit: 1, - // assertFunc: func(podList *v1.PodList) { - // Expect(podList.Items).To(HaveLen(1)) - // Expect(podList.Items[0].Name).To(Equal("changedSidecarPod1")) - // Expect(podList.Continue).To(Equal("continue")) - // }, - // }, { - name: "Should respect limit and use continue token to obtain rest of pods when getting pods to restart", + name: "Should not return pods without istio sidecar", + c: createClientSet(helpers.FixPodWithoutSidecar("app", "custom")), + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, + }, + { + name: "Should not return any pod when pods have correct image", + c: createClientSet( + helpers.NewSidecarPodBuilder().Build(), + ), + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, + }, + { + name: "Should return pod with different image repository", + c: createClientSet( + helpers.NewSidecarPodBuilder().Build(), + helpers.NewSidecarPodBuilder(). + SetName("changedSidecarPod"). + SetSidecarImageRepository("istio/different-proxy"). + Build(), + ), + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(HaveLen(1)) + Expect(podList.Items[0].Name).To(Equal("changedSidecarPod")) + }, + }, + { + name: "Should return pod with different image tag", + c: createClientSet( + helpers.NewSidecarPodBuilder().Build(), + helpers.NewSidecarPodBuilder(). + SetName("changedSidecarPod"). + SetSidecarImageTag("1.11.0"). + Build(), + ), + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(HaveLen(1)) + Expect(podList.Items[0].Name).To(Equal("changedSidecarPod")) + }, + }, + { + name: "Should ignore pod that has different image tag when it has not all condition status as True", + c: createClientSet( + helpers.NewSidecarPodBuilder(). + SetSidecarImageTag("1.12.0"). + SetConditionStatus("False"). + Build(), + ), + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, + }, + { + name: "Should ignore pod that has different image tag when phase is not running", + c: createClientSet( + helpers.NewSidecarPodBuilder(). + SetSidecarImageTag("1.12.0"). + SetPodStatusPhase("Pending"). + Build(), + ), + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, + }, + { + name: "Should ignore pod that has different image tag when it has a deletion timestamp", + c: createClientSet( + helpers.NewSidecarPodBuilder(). + SetSidecarImageTag("1.12.0"). + SetDeletionTimestamp(time.Now()). + Build(), + ), + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, + }, + { + name: "Should ignore pod that has different image tag when proxy container name is not in istio annotation", + c: createClientSet( + helpers.NewSidecarPodBuilder(). + SetSidecarImageTag("1.12.0"). + SetSidecarContainerName("custom-sidecar-proxy-container-name"). + Build(), + ), + limit: 10, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, + }, + { + name: "Should contain only one pod when there are multiple predicates that would restart the pod", + c: createClientSet( + helpers.NewSidecarPodBuilder(). + SetName("changedSidecarPod"). + SetSidecarImageRepository("istio/different-proxy"). + Build(), + ), + limit: 10, + predicates: []filter.SidecarProxyPredicate{pods.NewRestartProxyPredicate(expectedImage, helpers.DefaultSidecarResources)}, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(HaveLen(1)) + }, + }, + { + name: "Should respect limit set when getting pods to restart", c: NewFakeClientWithLimit( createClientSet( helpers.NewSidecarPodBuilder(). SetName("changedSidecarPod1"). SetSidecarImageRepository("istio/different-proxy"). Build(), - helpers.NewSidecarPodBuilder().Build(), helpers.NewSidecarPodBuilder(). SetName("changedSidecarPod2"). SetSidecarImageRepository("istio/different-proxy"). Build(), - ), 3), - limit: 3, + ), 1), + limit: 1, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(HaveLen(1)) + Expect(podList.Items[0].Name).To(Equal("changedSidecarPod1")) + Expect(podList.Continue).To(Equal("continue")) + }, + }, + { + name: "Should respect limit and use continue token to obtain rest of pods when getting pods to restart", + c: NewFakeClientWithLimit(createClientSet( + helpers.NewSidecarPodBuilder(). + SetName("changedSidecarPod1"). + SetSidecarImageRepository("istio/different-proxy"). + Build(), + helpers.NewSidecarPodBuilder().Build(), + helpers.NewSidecarPodBuilder(). + SetName("changedSidecarPod2"). + SetSidecarImageRepository("istio/different-proxy"). + Build(), + ), 2), + limit: 2, assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(HaveLen(2)) Expect(podList.Items[0].Name).To(Equal("changedSidecarPod1")) @@ -205,7 +203,7 @@ var _ = Describe("GetPodsToRestart", func() { }, } for _, tt := range tests { - FIt(tt.name, func() { + It(tt.name, func() { podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, tt.predicates, tt.limit, &logger) Expect(err).NotTo(HaveOccurred()) tt.assertFunc(podList) @@ -346,24 +344,24 @@ func createClientSet(objects ...client.Object) client.Client { type fakeClientWithLimit struct { client.Client - limit int64 - callCount int - continueTokenOnNextCall bool + limit int64 + callCount int + expectContinueNext bool } func NewFakeClientWithLimit(c client.Client, limit int64) *fakeClientWithLimit { return &fakeClientWithLimit{ - Client: c, - limit: limit, - callCount: 0, - continueTokenOnNextCall: false, + Client: c, + limit: limit, + callCount: 0, + expectContinueNext: false, } } func (p *fakeClientWithLimit) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { p.callCount++ - foundLimitOpt := false + limitOptFound := false continueToken := "" for _, opt := range opts { @@ -373,31 +371,23 @@ func (p *fakeClientWithLimit) List(ctx context.Context, list client.ObjectList, if int64(limit) != p.limit { return errors.New("limit not set as expected") } - foundLimitOpt = true + limitOptFound = true case client.Continue: continueToken = string(opt.(client.Continue)) } } - if !foundLimitOpt { + if !limitOptFound { return errors.New("limit not set when listing pods") } - podList, ok := list.(*v1.PodList) - if !ok { - return errors.New("list is not a pod list") - } - - fmt.Printf("call number: %d\n", p.callCount) - fmt.Printf("continue token: %s\n", continueToken) - switch p.callCount { case 1: - if podList.Continue != "" { + if continueToken != "" { return errors.New("continue token should be empty on the first call") } case 2: - if p.continueTokenOnNextCall && continueToken != "continue" { + if p.expectContinueNext && continueToken != "continue" { return errors.New("continue token should be set correctly on the second call") } } @@ -407,15 +397,20 @@ func (p *fakeClientWithLimit) List(ctx context.Context, list client.ObjectList, return err } + podList, ok := list.(*v1.PodList) + if !ok { + return errors.New("list is not a pod list") + } + if len(podList.Items) > int(p.limit) { - if podList.Continue == "" { + if continueToken == "" { podList.Continue = "continue" podList.Items = podList.Items[:p.limit] - p.continueTokenOnNextCall = true + p.expectContinueNext = true } else { podList.Items = podList.Items[p.limit:] podList.Continue = "" - p.continueTokenOnNextCall = false + p.expectContinueNext = false } } From df6608e34042241927e2d027359f6f288f51c039 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 11 Jul 2024 13:09:14 +0200 Subject: [PATCH 07/22] Tests --- controllers/istio_controller.go | 7 +- controllers/istio_controller_test.go | 66 +++++++++++++++---- .../adr/0002-istio-cr-status-improvements.md | 3 +- docs/user/00-10-overview-istio-controller.md | 1 + internal/clusterconfig/clusterconfig_test.go | 7 +- internal/compatibility/proxy_restart_test.go | 20 +++--- internal/restarter/restart_test.go | 12 ++-- internal/restarter/sidecars.go | 6 +- internal/restarter/sidecars_test.go | 23 +++++-- main.go | 7 +- pkg/lib/ingressgateway/ingressgateway_test.go | 17 ++--- pkg/lib/sidecars/pods/filter_test.go | 4 +- pkg/lib/sidecars/proxy.go | 2 +- 13 files changed, 121 insertions(+), 54 deletions(-) diff --git a/controllers/istio_controller.go b/controllers/istio_controller.go index 4b906890e..dcf7ca69e 100644 --- a/controllers/istio_controller.go +++ b/controllers/istio_controller.go @@ -156,7 +156,7 @@ func (r *IstioReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl } return ctrl.Result{}, err } else if requeue { - return r.requeueReconciliation(ctx, &istioCR, installationErr, operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonIstioInstallUninstallFailed)) + return r.requeueReconciliationWithoutError() } return r.finishReconcile(ctx, &istioCR, istioImageVersion.Tag()) @@ -176,6 +176,11 @@ func (r *IstioReconciler) requeueReconciliation(ctx context.Context, istioCR *op return ctrl.Result{}, err } +func (r *IstioReconciler) requeueReconciliationWithoutError() (ctrl.Result, error) { + r.log.Info("Reconcile requeued") + return ctrl.Result{Requeue: true}, 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() { diff --git a/controllers/istio_controller_test.go b/controllers/istio_controller_test.go index 04f7a2d46..01b2d0071 100644 --- a/controllers/istio_controller_test.go +++ b/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,6 +951,59 @@ 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(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 Unknown") + 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.ConditionReasonReconcileUnknown))) + Expect((*updatedIstioCR.Status.Conditions)[0].Message).To(Equal(operatorv1alpha2.ConditionReasonReconcileUnknownMessage)) + Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionUnknown)) + }) }) }) }) diff --git a/docs/contributor/adr/0002-istio-cr-status-improvements.md b/docs/contributor/adr/0002-istio-cr-status-improvements.md index 3e22cb8f6..aeeefe682 100644 --- a/docs/contributor/adr/0002-istio-cr-status-improvements.md +++ b/docs/contributor/adr/0002-istio-cr-status-improvements.md @@ -25,7 +25,7 @@ Conditions: | Error | Ready | False | OlderCRExists | This Istio custom resource is not the oldest one and does not represent the module state | | Processing | Ready | False | IstioInstallNotNeeded | Istio installation is not needed | | Processing | Ready | False | IstioInstallSucceeded | Istio installation succeeded | -| Processing | Ready | False | IstioUninstallSucceeded | Istio uninstallation succeeded | +| Processing | Ready | False | IstioUninstallSucceeded | Istio uninstallation succeeded | | Error | Ready | False | IstioInstallUninstallFailed | Istio install or uninstall failed | | Error | Ready | False | IstioCustomResourceMisconfigured | Istio custom resource has invalid configuration | | Warning | Ready | False | IstioCustomResourcesDangling | Istio deletion blocked because of existing Istio custom resources | @@ -33,6 +33,7 @@ Conditions: | Error | Ready | False | CustomResourcesReconcileFailed | Custom resources reconciliation failed | | Processing | ProxySidecarRestartSucceeded | True | ProxySidecarRestartSucceeded | Proxy sidecar restart succeeded | | Error | ProxySidecarRestartSucceeded | False | ProxySidecarRestartFailed | Proxy sidecar restart failed | +| Processing | ProxySidecarRestartSucceeded | False | ProxySidecarPartiallySucceeded | Proxy sidecar restart partially succeeded | | Warning | ProxySidecarRestartSucceeded | False | ProxySidecarManualRestartRequired | Proxy sidecar manual restart is required for some workloads | | Processing | Ready | False | IngressGatewayReconcileSucceeded | Istio Ingress Gateway reconciliation succeeded | | Error | Ready | False | IngressGatewayReconcileFailed | Istio Ingress Gateway reconciliation failed | diff --git a/docs/user/00-10-overview-istio-controller.md b/docs/user/00-10-overview-istio-controller.md index 29e036ee4..46200b9b0 100644 --- a/docs/user/00-10-overview-istio-controller.md +++ b/docs/user/00-10-overview-istio-controller.md @@ -54,6 +54,7 @@ Conditions: | Error | Ready | False | CustomResourcesReconcileFailed | Custom resources reconciliation failed | | Processing | ProxySidecarRestartSucceeded | True | ProxySidecarRestartSucceeded | Proxy sidecar restart succeeded | | Error | ProxySidecarRestartSucceeded | False | ProxySidecarRestartFailed | Proxy sidecar restart failed | +| Processing | ProxySidecarRestartSucceeded | False | ProxySidecarPartiallySucceeded | Proxy sidecar restart partially succeeded | | Warning | ProxySidecarRestartSucceeded | False | ProxySidecarManualRestartRequired | Proxy sidecar manual restart is required for some workloads | | Processing | Ready | False | IngressGatewayReconcileSucceeded | Istio Ingress Gateway reconciliation succeeded | | Error | Ready | False | IngressGatewayReconcileFailed | Istio Ingress Gateway reconciliation failed | diff --git a/internal/clusterconfig/clusterconfig_test.go b/internal/clusterconfig/clusterconfig_test.go index 9d0ab774c..e949196f3 100644 --- a/internal/clusterconfig/clusterconfig_test.go +++ b/internal/clusterconfig/clusterconfig_test.go @@ -2,6 +2,7 @@ package clusterconfig_test import ( "context" + "k8s.io/apimachinery/pkg/api/resource" "github.com/kyma-project/istio/operator/internal/clusterconfig" @@ -23,7 +24,7 @@ const ( ) var _ = Describe("GetClusterProvider", func() { - It("Should return other when cluster provider is unknown", func() { + It("should return other when cluster provider is unknown", func() { //given node := corev1.Node{ ObjectMeta: v1.ObjectMeta{ @@ -41,7 +42,7 @@ var _ = Describe("GetClusterProvider", func() { Expect(err).To(BeNil()) Expect(p).To(Equal("other")) }) - It("Should return 'aws' for clusters provisioned on AWS nodes", func() { + It("should return 'aws' for clusters provisioned on AWS nodes", func() { //given node := corev1.Node{ ObjectMeta: v1.ObjectMeta{ @@ -54,7 +55,7 @@ var _ = Describe("GetClusterProvider", func() { Expect(err).To(BeNil()) Expect(p).To(Equal("aws")) }) - It("Should return 'other' for clusters without nodes", func() { + It("should return 'other' for clusters without nodes", func() { //given client := createFakeClient() p, err := clusterconfig.GetClusterProvider(context.TODO(), client) diff --git a/internal/compatibility/proxy_restart_test.go b/internal/compatibility/proxy_restart_test.go index e74990072..4cc4de9fd 100644 --- a/internal/compatibility/proxy_restart_test.go +++ b/internal/compatibility/proxy_restart_test.go @@ -12,7 +12,7 @@ import ( var _ = Describe("Proxy Restarter", func() { Context("RequiresProxyRestart", func() { - It("Should evaluate to true when proxy metadata values exist and new and old compatibility mode is different", func() { + It("should evaluate to true when proxy metadata values exist and new and old compatibility mode is different", func() { predicate := ProxyRestartPredicate{ oldCompatibilityMode: true, newCompatibilityMode: false, @@ -23,7 +23,7 @@ var _ = Describe("Proxy Restarter", func() { Expect(predicate.RequiresProxyRestart(v1.Pod{})).To(BeTrue()) }) - It("Should evaluate to false when proxy metadata values exist and new and old compatibility mode is equal", func() { + It("should evaluate to false when proxy metadata values exist and new and old compatibility mode is equal", func() { predicate := ProxyRestartPredicate{ oldCompatibilityMode: true, newCompatibilityMode: true, @@ -34,7 +34,7 @@ var _ = Describe("Proxy Restarter", func() { Expect(predicate.RequiresProxyRestart(v1.Pod{})).To(BeFalse()) }) - It("Should evaluate to false when no proxy metadata values exist and new and old compatibility mode is different", func() { + It("should evaluate to false when no proxy metadata values exist and new and old compatibility mode is different", func() { predicate := ProxyRestartPredicate{ oldCompatibilityMode: true, newCompatibilityMode: false, @@ -42,7 +42,7 @@ var _ = Describe("Proxy Restarter", func() { Expect(predicate.RequiresProxyRestart(v1.Pod{})).To(BeFalse()) }) - It("Should evaluate to false when no proxy metadata values exist and new and old compatibility mode is equal", func() { + It("should evaluate to false when no proxy metadata values exist and new and old compatibility mode is equal", func() { predicate := ProxyRestartPredicate{ oldCompatibilityMode: true, newCompatibilityMode: true, @@ -52,7 +52,7 @@ var _ = Describe("Proxy Restarter", func() { }) Context("NewRestartPredicate", func() { - It("Should return an error if getLastAppliedConfiguration fails", func() { + It("should return an error if getLastAppliedConfiguration fails", func() { _, err := NewRestartPredicate(&operatorv1alpha2.Istio{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -63,7 +63,7 @@ var _ = Describe("Proxy Restarter", func() { Expect(err).To(HaveOccurred()) }) - It("Should return false for old compatibility mode if lastAppliedConfiguration is empty", func() { + It("should return false for old compatibility mode if lastAppliedConfiguration is empty", func() { predicate, err := NewRestartPredicate(&operatorv1alpha2.Istio{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{}, @@ -74,7 +74,7 @@ var _ = Describe("Proxy Restarter", func() { Expect(predicate.oldCompatibilityMode).To(BeFalse()) }) - It("Should return value for old compatibility mode from lastAppliedConfiguration", func() { + It("should return value for old compatibility mode from lastAppliedConfiguration", func() { predicate, err := NewRestartPredicate(&operatorv1alpha2.Istio{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -87,7 +87,7 @@ var _ = Describe("Proxy Restarter", func() { Expect(predicate.oldCompatibilityMode).To(BeTrue()) }) - It("Should return value for new compatibility mode from istio CR", func() { + It("should return value for new compatibility mode from istio CR", func() { predicate, err := NewRestartPredicate(&operatorv1alpha2.Istio{ Spec: operatorv1alpha2.IstioSpec{ CompatibilityMode: true, @@ -100,7 +100,7 @@ var _ = Describe("Proxy Restarter", func() { }) Context("config", func() { - It("Should return true if proxy metadata values exist", func() { + It("should return true if proxy metadata values exist", func() { config := config{ proxyMetadata: map[string]string{"key": "value"}, } @@ -108,7 +108,7 @@ var _ = Describe("Proxy Restarter", func() { Expect(config.hasProxyMetadata()).To(BeTrue()) }) - It("Should return false if proxy metadata values do not exist", func() { + It("should return false if proxy metadata values do not exist", func() { config := config{} Expect(config.hasProxyMetadata()).To(BeFalse()) }) diff --git a/internal/restarter/restart_test.go b/internal/restarter/restart_test.go index bcc6dba59..ce8512684 100644 --- a/internal/restarter/restart_test.go +++ b/internal/restarter/restart_test.go @@ -12,7 +12,7 @@ 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{} @@ -25,7 +25,7 @@ var _ = Describe("Restart", func() { Expect(requeue).To(BeFalse()) }) - It("Should return nil if no restarters are provided", func() { + It("should return nil if no restarters are provided", func() { // when err, requeue := restarter.Restart(context.Background(), &operatorv1alpha2.Istio{}, nil) @@ -34,7 +34,7 @@ var _ = Describe("Restart", func() { Expect(requeue).To(BeFalse()) }) - It("Should invoke Restart", func() { + It("should invoke Restart", func() { // given r := &restarterMock{} @@ -47,7 +47,7 @@ var _ = Describe("Restart", func() { 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"), "")} @@ -59,7 +59,7 @@ var _ = Describe("Restart", func() { 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()} @@ -72,7 +72,7 @@ var _ = Describe("Restart", func() { Expect(requeue).To(BeFalse()) }) - It("Should respect requeue condition if one of the restarters return it", func() { + It("should respect requeue condition if one of the restarters return it", func() { // given r1 := &restarterMock{requeue: false} r2 := &restarterMock{requeue: true} diff --git a/internal/restarter/sidecars.go b/internal/restarter/sidecars.go index 373984f75..909a79fb3 100644 --- a/internal/restarter/sidecars.go +++ b/internal/restarter/sidecars.go @@ -90,7 +90,7 @@ func (s *SidecarsRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio return described_errors.NewDescribedError(err, errorDescription), false } - warnings, requeue, err := s.ProxyResetter.ProxyReset(ctx, s.Client, expectedImage, expectedResources, []filter.SidecarProxyPredicate{compatibiltyPredicate}, &s.Log) + warnings, hasMorePods, err := s.ProxyResetter.ProxyReset(ctx, s.Client, expectedImage, expectedResources, []filter.SidecarProxyPredicate{compatibiltyPredicate}, &s.Log) if err != nil { s.Log.Error(err, "Failed to reset proxy") s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartFailed)) @@ -118,11 +118,11 @@ func (s *SidecarsRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio return warningErr, false } - if !requeue { + if !hasMorePods { s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartSucceeded)) } else { s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartPartiallySucceeded)) } - return nil, requeue + return nil, hasMorePods } diff --git a/internal/restarter/sidecars_test.go b/internal/restarter/sidecars_test.go index ffb75a8cb..88db8d815 100644 --- a/internal/restarter/sidecars_test.go +++ b/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{ @@ -58,10 +58,13 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { Expect(err.Level()).To(Equal(described_errors.Error)) Expect(err.Error()).To(ContainSubstring("istio-system Pods version 1.16.0 do not match istio operator version 1.16.1")) Expect(requeue).To(BeFalse()) + Expect((*istioCr.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeProxySidecarRestartSucceeded))) + Expect((*istioCr.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonProxySidecarRestartFailed))) Expect((*istioCr.Status.Conditions)[0].Message).To(Equal("Proxy sidecar restart failed")) + Expect((*istioCr.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse)) }) - 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{ @@ -117,10 +120,13 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { Expect(err).Should(HaveOccurred()) Expect(err.Level()).To(Equal(described_errors.Warning)) Expect(requeue).To(BeFalse()) + Expect((*istioCr.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeProxySidecarRestartSucceeded))) + Expect((*istioCr.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonProxySidecarManualRestartRequired))) 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)")) + Expect((*istioCr.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse)) }) - 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{ @@ -160,10 +166,13 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { Expect(err).Should(HaveOccurred()) Expect(err.Level()).To(Equal(described_errors.Warning)) Expect(requeue).To(BeFalse()) + Expect((*istioCr.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeProxySidecarRestartSucceeded))) + Expect((*istioCr.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonProxySidecarManualRestartRequired))) Expect((*istioCr.Status.Conditions)[0].Message).To(Equal("The sidecars of the following workloads could not be restarted: ns1/name1, ns2/name2")) + Expect((*istioCr.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse)) }) - 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{ @@ -190,11 +199,13 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { // then Expect(err).Should(Not(HaveOccurred())) Expect(requeue).To(BeFalse()) + Expect((*istioCr.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeProxySidecarRestartSucceeded))) Expect((*istioCr.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonProxySidecarRestartSucceeded))) Expect((*istioCr.Status.Conditions)[0].Message).To(Equal(operatorv1alpha2.ConditionReasonProxySidecarRestartSucceededMessage)) + Expect((*istioCr.Status.Conditions)[0].Status).To(Equal(metav1.ConditionTrue)) }) - It("Should succeed proxy reset even if not all proxies are reset and requeue is required", func() { + 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{ @@ -223,8 +234,10 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { // then Expect(err).ToNot(HaveOccurred()) Expect(requeue).To(BeTrue()) + Expect((*istioCr.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeProxySidecarRestartSucceeded))) Expect((*istioCr.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonProxySidecarRestartPartiallySucceeded))) Expect((*istioCr.Status.Conditions)[0].Message).To(Equal(operatorv1alpha2.ConditionReasonProxySidecarRestartPartiallySucceededMessage)) + Expect((*istioCr.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse)) }) }) diff --git a/main.go b/main.go index e23d04cfb..2023e4cc1 100644 --- a/main.go +++ b/main.go @@ -18,13 +18,14 @@ package main import ( "flag" + "os" + "time" + "github.com/kyma-project/istio/operator/internal/reconciliations/istio" networkingv1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" v1 "k8s.io/api/apps/v1" - "os" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/manager" - "time" networkingv1 "istio.io/client-go/pkg/apis/networking/v1" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -153,7 +154,7 @@ func createManager(flagVar *FlagVar) (manager.Manager, error) { Cache: &client.CacheOptions{ // The cache is disabled for these objects to avoid huge memory usage. // Having the cache enabled had previously caused memory usage - //to have a significant peak when sidecar restart was triggered. + // to have a significant peak when sidecar restart was triggered. DisableFor: []client.Object{ &v1.DaemonSet{}, &v1.Deployment{}, diff --git a/pkg/lib/ingressgateway/ingressgateway_test.go b/pkg/lib/ingressgateway/ingressgateway_test.go index 9c5a10982..e40a3d65b 100644 --- a/pkg/lib/ingressgateway/ingressgateway_test.go +++ b/pkg/lib/ingressgateway/ingressgateway_test.go @@ -3,6 +3,7 @@ package ingressgateway_test import ( "context" "fmt" + "github.com/kyma-project/istio/operator/pkg/labels" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -15,7 +16,7 @@ import ( var _ = Describe("Ingress Gateway Restarter", func() { Context("RequiresIngressGatewayRestart", func() { - It("Should evaluate to true if new is nil and old is not nil", func() { + It("should evaluate to true if new is nil and old is not nil", func() { evaluator := ingressgateway.NumTrustedProxiesRestartEvaluator{ NewNumTrustedProxies: nil, OldNumTrustedProxies: new(int), @@ -24,7 +25,7 @@ var _ = Describe("Ingress Gateway Restarter", func() { Expect(evaluator.RequiresIngressGatewayRestart()).To(BeTrue()) }) - It("Should evaluate to true if new is not nil and old is nil", func() { + It("should evaluate to true if new is not nil and old is nil", func() { evaluator := ingressgateway.NumTrustedProxiesRestartEvaluator{ NewNumTrustedProxies: new(int), OldNumTrustedProxies: nil, @@ -33,7 +34,7 @@ var _ = Describe("Ingress Gateway Restarter", func() { Expect(evaluator.RequiresIngressGatewayRestart()).To(BeTrue()) }) - It("Should evaluate to true if numTrustedProxies is different", func() { + It("should evaluate to true if numTrustedProxies is different", func() { newNumTrustedProxies := 1 oldNumTrustedProxies := 2 @@ -45,7 +46,7 @@ var _ = Describe("Ingress Gateway Restarter", func() { Expect(evaluator.RequiresIngressGatewayRestart()).To(BeTrue()) }) - It("Should evaluate to false if numTrustedProxies is the same", func() { + It("should evaluate to false if numTrustedProxies is the same", func() { numTrustedProxies := 1 oldNumTrustedProxies := 1 @@ -65,7 +66,7 @@ var _ = Describe("Ingress Gateway Restarter", func() { lastAppliedConfiguration = labels.LastAppliedConfiguration ) - It("Should return an error if getLastAppliedConfiguration fails", func() { + It("should return an error if getLastAppliedConfiguration fails", func() { predicate := ingressgateway.NewRestartPredicate(&operatorv1alpha2.Istio{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -78,7 +79,7 @@ var _ = Describe("Ingress Gateway Restarter", func() { Expect(err).To(HaveOccurred()) }) - It("Should return nil for old numTrustedProxies if lastAppliedConfiguration is empty", func() { + It("should return nil for old numTrustedProxies if lastAppliedConfiguration is empty", func() { predicate := ingressgateway.NewRestartPredicate(&operatorv1alpha2.Istio{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{}, @@ -91,7 +92,7 @@ var _ = Describe("Ingress Gateway Restarter", func() { Expect(evaluator.(ingressgateway.NumTrustedProxiesRestartEvaluator).OldNumTrustedProxies).To(BeNil()) }) - It("Should return correct not nil value for new and old numTrustedProxies", func() { + It("should return correct not nil value for new and old numTrustedProxies", func() { istio := &operatorv1alpha2.Istio{ Spec: operatorv1alpha2.IstioSpec{ Config: operatorv1alpha2.Config{ @@ -112,7 +113,7 @@ var _ = Describe("Ingress Gateway Restarter", func() { Expect(*(evaluator.(ingressgateway.NumTrustedProxiesRestartEvaluator).OldNumTrustedProxies)).To(Equal(2)) }) - It("Should return correct nil value for new and old numTrustedProxies", func() { + It("should return correct nil value for new and old numTrustedProxies", func() { istio := &operatorv1alpha2.Istio{ Spec: operatorv1alpha2.IstioSpec{ Config: operatorv1alpha2.Config{ diff --git a/pkg/lib/sidecars/pods/filter_test.go b/pkg/lib/sidecars/pods/filter_test.go index 3f3c72200..f7fb33d30 100644 --- a/pkg/lib/sidecars/pods/filter_test.go +++ b/pkg/lib/sidecars/pods/filter_test.go @@ -9,7 +9,7 @@ import ( ) var _ = Describe("RequiresProxyRestart", func() { - It("Should should return false when pod has custom image annotation", 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{}) @@ -21,7 +21,7 @@ var _ = Describe("RequiresProxyRestart", func() { 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{}) diff --git a/pkg/lib/sidecars/proxy.go b/pkg/lib/sidecars/proxy.go index 5f88f887a..3336e9194 100644 --- a/pkg/lib/sidecars/proxy.go +++ b/pkg/lib/sidecars/proxy.go @@ -44,7 +44,7 @@ func (p *ProxyReset) ProxyReset(ctx context.Context, c client.Client, expectedIm if !hasMorePodsToRestart { logger.Info("Proxy reset completed") } else { - logger.Info("Proxy reset partially completed") + logger.Info("Proxy reset only partially completed") } return warnings, hasMorePodsToRestart, nil From 7bb11fea712ea0f3293c13a45abbae4913f2531b Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 11 Jul 2024 13:26:58 +0200 Subject: [PATCH 08/22] Fix lint --- pkg/lib/sidecars/pods/get_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/lib/sidecars/pods/get_test.go b/pkg/lib/sidecars/pods/get_test.go index 1d973b83e..62974807d 100644 --- a/pkg/lib/sidecars/pods/get_test.go +++ b/pkg/lib/sidecars/pods/get_test.go @@ -365,15 +365,14 @@ func (p *fakeClientWithLimit) List(ctx context.Context, list client.ObjectList, continueToken := "" for _, opt := range opts { - switch opt.(type) { + switch opt := opt.(type) { case client.Limit: - limit := opt.(client.Limit) - if int64(limit) != p.limit { + if int64(opt) != p.limit { return errors.New("limit not set as expected") } limitOptFound = true case client.Continue: - continueToken = string(opt.(client.Continue)) + continueToken = string(opt) } } From 187ed0473d0d11eccd75a7d1307400251f85b5e3 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 11 Jul 2024 14:04:35 +0200 Subject: [PATCH 09/22] Release notes --- docs/release-notes/1.9.0.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/release-notes/1.9.0.md diff --git a/docs/release-notes/1.9.0.md b/docs/release-notes/1.9.0.md new file mode 100644 index 000000000..372b4d02b --- /dev/null +++ b/docs/release-notes/1.9.0.md @@ -0,0 +1,5 @@ +## New Features + +- Pods with proxy sidecars will be restarted in chunks on Istio upgrade in multiple reconciliations and not all at once. This will increase stability and reliability of the reconciliation for the Istio module operator. [issue #155](https://github.com/kyma-project/istio/issues/155) + + From 3fcdb36ecb379dbdda2b44f814344c9b59ffd55c Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 11 Jul 2024 15:31:01 +0200 Subject: [PATCH 10/22] Have separate limits to list and restart --- controllers/istio_controller.go | 6 +-- main.go | 2 + pkg/lib/sidecars/pods/filter.go | 2 +- pkg/lib/sidecars/pods/get.go | 29 +++++++++---- pkg/lib/sidecars/pods/get_test.go | 70 ++++++++++++++++++++----------- pkg/lib/sidecars/proxy.go | 6 ++- 6 files changed, 77 insertions(+), 38 deletions(-) diff --git a/controllers/istio_controller.go b/controllers/istio_controller.go index dcf7ca69e..b65dfd2da 100644 --- a/controllers/istio_controller.go +++ b/controllers/istio_controller.go @@ -156,7 +156,7 @@ func (r *IstioReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl } return ctrl.Result{}, err } else if requeue { - return r.requeueReconciliationWithoutError() + return r.requeueReconciliationWithoutError(ctx, &istioCR) } return r.finishReconcile(ctx, &istioCR, istioImageVersion.Tag()) @@ -171,12 +171,12 @@ 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() (ctrl.Result, error) { +func (r *IstioReconciler) requeueReconciliationWithoutError(ctx context.Context, istioCR *operatorv1alpha2.Istio) (ctrl.Result, error) { + r.statusHandler.UpdateToProcessing(ctx, istioCR) r.log.Info("Reconcile requeued") return ctrl.Result{Requeue: true}, nil } diff --git a/main.go b/main.go index 2023e4cc1..5476d70b7 100644 --- a/main.go +++ b/main.go @@ -24,6 +24,7 @@ import ( "github.com/kyma-project/istio/operator/internal/reconciliations/istio" networkingv1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" v1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -160,6 +161,7 @@ func createManager(flagVar *FlagVar) (manager.Manager, error) { &v1.Deployment{}, &v1.StatefulSet{}, &v1.ReplicaSet{}, + &corev1.Pod{}, // this is required for the sidecar restart when listing pods with limit }, }, }, diff --git a/pkg/lib/sidecars/pods/filter.go b/pkg/lib/sidecars/pods/filter.go index f62ee53a6..5f56e9a1d 100644 --- a/pkg/lib/sidecars/pods/filter.go +++ b/pkg/lib/sidecars/pods/filter.go @@ -39,7 +39,7 @@ func HasIstioSidecarStatusAnnotation(pod v1.Pod) bool { func IsPodReady(pod v1.Pod) bool { isMarkedForDeletion := pod.ObjectMeta.DeletionTimestamp != nil - return !isMarkedForDeletion && hasTrueStatusConditions(pod) && isPodRunning(pod) + return !isMarkedForDeletion && isPodRunning(pod) && hasTrueStatusConditions(pod) } func hasTrueStatusConditions(pod v1.Pod) bool { diff --git a/pkg/lib/sidecars/pods/get.go b/pkg/lib/sidecars/pods/get.go index ab2f4d1e7..aebed8bf8 100644 --- a/pkg/lib/sidecars/pods/get.go +++ b/pkg/lib/sidecars/pods/get.go @@ -36,13 +36,25 @@ func (r SidecarImage) matchesImageIn(container v1.Container) bool { return container.Image == r.String() } -func listRunningPods(ctx context.Context, c client.Client, limit int, continueToken string) (*v1.PodList, error) { +type PodsRestartLimits struct { + podsToRestartLimit int + podsToListLimit int +} + +func NewPodsRestartLimits(restartLimit, listLimit int) *PodsRestartLimits { + return &PodsRestartLimits{ + podsToRestartLimit: restartLimit, + podsToListLimit: listLimit, + } +} + +func listRunningPods(ctx context.Context, c client.Client, listLimit 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(limit), + client.Limit(listLimit), } if continueToken != "" { listOps = append(listOps, client.Continue(continueToken)) @@ -53,8 +65,8 @@ func listRunningPods(ctx context.Context, c client.Client, limit int, continueTo return podList, err } -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) +func getSidecarPods(ctx context.Context, c client.Client, logger *logr.Logger, listLimit int, continueToken string) (*v1.PodList, error) { + podList, err := listRunningPods(ctx, c, listLimit, continueToken) if err != nil { return nil, err } @@ -75,13 +87,13 @@ func getSidecarPods(ctx context.Context, c client.Client, logger *logr.Logger, l return podsWithSidecar, nil } -func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, limit int, logger *logr.Logger) (*v1.PodList, error) { +func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, limits *PodsRestartLimits, 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, limit, podsToRestart.Continue) + podsWithSidecar, err := getSidecarPods(ctx, c, logger, limits.podsToListLimit, podsToRestart.Continue) if err != nil { return nil, err } @@ -92,9 +104,12 @@ func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage Sideca break } } + if len(podsToRestart.Items) >= limits.podsToRestartLimit { + break + } } podsToRestart.Continue = podsWithSidecar.Continue - while = len(podsToRestart.Items) < limit && podsToRestart.Continue != "" + while = len(podsToRestart.Items) < limits.podsToRestartLimit && podsToRestart.Continue != "" } if len(podsToRestart.Items) > 0 { diff --git a/pkg/lib/sidecars/pods/get_test.go b/pkg/lib/sidecars/pods/get_test.go index 62974807d..0964ad9d5 100644 --- a/pkg/lib/sidecars/pods/get_test.go +++ b/pkg/lib/sidecars/pods/get_test.go @@ -43,13 +43,13 @@ var _ = Describe("GetPodsToRestart", func() { name string c client.Client predicates []filter.SidecarProxyPredicate - limit int + limits *pods.PodsRestartLimits assertFunc func(podList *v1.PodList) }{ { - name: "Should not return pods without istio sidecar", - c: createClientSet(helpers.FixPodWithoutSidecar("app", "custom")), - limit: 10, + name: "Should not return pods without istio sidecar", + c: createClientSet(helpers.FixPodWithoutSidecar("app", "custom")), + limits: pods.NewPodsRestartLimits(5, 5), assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(BeEmpty()) }, @@ -59,7 +59,7 @@ var _ = Describe("GetPodsToRestart", func() { c: createClientSet( helpers.NewSidecarPodBuilder().Build(), ), - limit: 10, + limits: pods.NewPodsRestartLimits(5, 5), assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(BeEmpty()) }, @@ -73,7 +73,7 @@ var _ = Describe("GetPodsToRestart", func() { SetSidecarImageRepository("istio/different-proxy"). Build(), ), - limit: 10, + limits: pods.NewPodsRestartLimits(5, 5), assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(HaveLen(1)) Expect(podList.Items[0].Name).To(Equal("changedSidecarPod")) @@ -88,7 +88,7 @@ var _ = Describe("GetPodsToRestart", func() { SetSidecarImageTag("1.11.0"). Build(), ), - limit: 10, + limits: pods.NewPodsRestartLimits(5, 5), assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(HaveLen(1)) Expect(podList.Items[0].Name).To(Equal("changedSidecarPod")) @@ -102,7 +102,7 @@ var _ = Describe("GetPodsToRestart", func() { SetConditionStatus("False"). Build(), ), - limit: 10, + limits: pods.NewPodsRestartLimits(5, 5), assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(BeEmpty()) }, @@ -115,7 +115,7 @@ var _ = Describe("GetPodsToRestart", func() { SetPodStatusPhase("Pending"). Build(), ), - limit: 10, + limits: pods.NewPodsRestartLimits(5, 5), assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(BeEmpty()) }, @@ -128,7 +128,7 @@ var _ = Describe("GetPodsToRestart", func() { SetDeletionTimestamp(time.Now()). Build(), ), - limit: 10, + limits: pods.NewPodsRestartLimits(5, 5), assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(BeEmpty()) }, @@ -141,7 +141,7 @@ var _ = Describe("GetPodsToRestart", func() { SetSidecarContainerName("custom-sidecar-proxy-container-name"). Build(), ), - limit: 10, + limits: pods.NewPodsRestartLimits(5, 5), assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(BeEmpty()) }, @@ -154,14 +154,34 @@ var _ = Describe("GetPodsToRestart", func() { SetSidecarImageRepository("istio/different-proxy"). Build(), ), - limit: 10, + limits: pods.NewPodsRestartLimits(5, 5), predicates: []filter.SidecarProxyPredicate{pods.NewRestartProxyPredicate(expectedImage, helpers.DefaultSidecarResources)}, assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(HaveLen(1)) }, }, { - name: "Should respect limit set when getting pods to restart", + name: "Should respect limit set when getting pods to restart if all pods listed", + c: NewFakeClientWithLimit( + createClientSet( + helpers.NewSidecarPodBuilder(). + SetName("changedSidecarPod1"). + SetSidecarImageRepository("istio/different-proxy"). + Build(), + helpers.NewSidecarPodBuilder(). + SetName("changedSidecarPod2"). + SetSidecarImageRepository("istio/different-proxy"). + Build(), + ), 5), + limits: pods.NewPodsRestartLimits(1, 5), + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(HaveLen(1)) + Expect(podList.Items[0].Name).To(Equal("changedSidecarPod1")) + Expect(podList.Continue).To(BeEmpty()) + }, + }, + { + name: "Should respect limit set when getting pods to restart and set continue token if there are more pods to list", c: NewFakeClientWithLimit( createClientSet( helpers.NewSidecarPodBuilder(). @@ -173,7 +193,7 @@ var _ = Describe("GetPodsToRestart", func() { SetSidecarImageRepository("istio/different-proxy"). Build(), ), 1), - limit: 1, + limits: pods.NewPodsRestartLimits(1, 1), assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(HaveLen(1)) Expect(podList.Items[0].Name).To(Equal("changedSidecarPod1")) @@ -181,7 +201,7 @@ var _ = Describe("GetPodsToRestart", func() { }, }, { - name: "Should respect limit and use continue token to obtain rest of pods when getting pods to restart", + name: "Should respect limit and use continue token to obtain rest of pods when listing pods", c: NewFakeClientWithLimit(createClientSet( helpers.NewSidecarPodBuilder(). SetName("changedSidecarPod1"). @@ -192,8 +212,8 @@ var _ = Describe("GetPodsToRestart", func() { SetName("changedSidecarPod2"). SetSidecarImageRepository("istio/different-proxy"). Build(), - ), 2), - limit: 2, + ), 1), + limits: pods.NewPodsRestartLimits(2, 1), assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(HaveLen(2)) Expect(podList.Items[0].Name).To(Equal("changedSidecarPod1")) @@ -204,7 +224,7 @@ var _ = Describe("GetPodsToRestart", func() { } for _, tt := range tests { It(tt.name, func() { - podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, tt.predicates, tt.limit, &logger) + podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, tt.predicates, tt.limits, &logger) Expect(err).NotTo(HaveOccurred()) tt.assertFunc(podList) }) @@ -282,7 +302,7 @@ var _ = Describe("GetPodsToRestart", func() { for _, tt := range tests { It(tt.name, func() { expectedImage := pods.NewSidecarImage("istio", "1.10.0") - podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, []filter.SidecarProxyPredicate{}, 10, &logger) + podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, []filter.SidecarProxyPredicate{}, pods.NewPodsRestartLimits(5, 5), &logger) Expect(err).NotTo(HaveOccurred()) tt.assertFunc(podList) }) @@ -344,7 +364,7 @@ func createClientSet(objects ...client.Object) client.Client { type fakeClientWithLimit struct { client.Client - limit int64 + listLimit int64 callCount int expectContinueNext bool } @@ -352,7 +372,7 @@ type fakeClientWithLimit struct { func NewFakeClientWithLimit(c client.Client, limit int64) *fakeClientWithLimit { return &fakeClientWithLimit{ Client: c, - limit: limit, + listLimit: limit, callCount: 0, expectContinueNext: false, } @@ -367,7 +387,7 @@ func (p *fakeClientWithLimit) List(ctx context.Context, list client.ObjectList, for _, opt := range opts { switch opt := opt.(type) { case client.Limit: - if int64(opt) != p.limit { + if int64(opt) != p.listLimit { return errors.New("limit not set as expected") } limitOptFound = true @@ -401,13 +421,13 @@ func (p *fakeClientWithLimit) List(ctx context.Context, list client.ObjectList, return errors.New("list is not a pod list") } - if len(podList.Items) > int(p.limit) { + if len(podList.Items) > int(p.listLimit) { if continueToken == "" { podList.Continue = "continue" - podList.Items = podList.Items[:p.limit] + podList.Items = podList.Items[:p.listLimit] p.expectContinueNext = true } else { - podList.Items = podList.Items[p.limit:] + podList.Items = podList.Items[p.listLimit:] podList.Continue = "" p.expectContinueNext = false } diff --git a/pkg/lib/sidecars/proxy.go b/pkg/lib/sidecars/proxy.go index 3336e9194..8e6315aae 100644 --- a/pkg/lib/sidecars/proxy.go +++ b/pkg/lib/sidecars/proxy.go @@ -13,7 +13,8 @@ import ( ) const ( - podsToRestartLimit = 10 + podsToRestartLimit = 30 + podsToListLimit = 100 ) type ProxyResetter interface { @@ -28,7 +29,8 @@ func NewProxyResetter() *ProxyReset { } func (p *ProxyReset) ProxyReset(ctx context.Context, c client.Client, expectedImage pods.SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) ([]restart.RestartWarning, bool, error) { - podsToRestart, err := pods.GetPodsToRestart(ctx, c, expectedImage, expectedResources, predicates, podsToRestartLimit, logger) + limits := pods.NewPodsRestartLimits(podsToRestartLimit, podsToListLimit) + podsToRestart, err := pods.GetPodsToRestart(ctx, c, expectedImage, expectedResources, predicates, limits, logger) if err != nil { return nil, false, err } From 492ad8dc7a9623177e7a39dc6d599e61bf1d9179 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 11 Jul 2024 15:39:44 +0200 Subject: [PATCH 11/22] Fix lint --- controllers/istio_controller.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/controllers/istio_controller.go b/controllers/istio_controller.go index b65dfd2da..5f4c03987 100644 --- a/controllers/istio_controller.go +++ b/controllers/istio_controller.go @@ -176,7 +176,10 @@ func (r *IstioReconciler) requeueReconciliation(ctx context.Context, istioCR *op } func (r *IstioReconciler) requeueReconciliationWithoutError(ctx context.Context, istioCR *operatorv1alpha2.Istio) (ctrl.Result, error) { - r.statusHandler.UpdateToProcessing(ctx, istioCR) + 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}, nil } From 4cfdc56885d23db63a2d495f53db19fc64eff066 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 11 Jul 2024 17:12:59 +0200 Subject: [PATCH 12/22] Docs --- docs/contributor/04-10-technical-design.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/docs/contributor/04-10-technical-design.md b/docs/contributor/04-10-technical-design.md index 938d57a9d..7ccc0c759 100644 --- a/docs/contributor/04-10-technical-design.md +++ b/docs/contributor/04-10-technical-design.md @@ -103,7 +103,7 @@ 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. @@ -111,9 +111,8 @@ The predicate initiates a restart of the sidecar and Ingress Gateway if the targ ### 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 running state and are part of the service mesh having the annotation `sidecar.istio.io/status`. +The Istio CR and [Istio Version](#istio-version) represent the desired state. Pods will be restarted in chunks with limits on how many to restart with one reconciliation and how many to list when requesting from Kubernetes API Server. If more Pods need to be restarted this will happen in next reconciliation. This happens with requeuing the reconciliation request, which will be imediately rescheduled. This component covers the following restart triggers: From 19262f95049b9edf43b63a4b535d48fafb43877a Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Mon, 15 Jul 2024 15:32:44 +0200 Subject: [PATCH 13/22] Conditions update --- api/v1alpha2/conditions.go | 11 ++++++----- api/v1alpha2/istio_types.go | 22 ++++++++++++---------- controllers/istio_controller.go | 1 + controllers/istio_controller_test.go | 8 ++++---- docs/contributor/04-10-technical-design.md | 4 ++++ pkg/lib/sidecars/pods/get.go | 3 +-- 6 files changed, 28 insertions(+), 21 deletions(-) diff --git a/api/v1alpha2/conditions.go b/api/v1alpha2/conditions.go index 3a74d12b3..8d640d3f1 100644 --- a/api/v1alpha2/conditions.go +++ b/api/v1alpha2/conditions.go @@ -20,11 +20,12 @@ func ConditionFromReason(reason ReasonWithMessage) *metav1.Condition { } 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}, - ConditionReasonValidationFailed: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonValidationFailedMessage}, - ConditionReasonOlderCRExists: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonOlderCRExistsMessage}, + ConditionReasonReconcileSucceeded: {Type: ConditionTypeReady, Status: metav1.ConditionTrue, Message: ConditionReasonReconcileSucceededMessage}, + ConditionReasonReconcileFailed: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonReconcileFailedMessage}, + ConditionReasonReconcileUnknown: {Type: ConditionTypeReady, Status: metav1.ConditionUnknown, Message: ConditionReasonReconcileUnknownMessage}, + ConditionReasonReconcileProxyResetRequeued: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonReconcileProxyResetRequeuedMessage}, + ConditionReasonValidationFailed: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonValidationFailedMessage}, + ConditionReasonOlderCRExists: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonOlderCRExistsMessage}, ConditionReasonIstioInstallNotNeeded: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonIstioInstallNotNeededMessage}, ConditionReasonIstioInstallSucceeded: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonIstioInstallSucceededMessage}, diff --git a/api/v1alpha2/istio_types.go b/api/v1alpha2/istio_types.go index f154927e3..cbaca5a98 100644 --- a/api/v1alpha2/istio_types.go +++ b/api/v1alpha2/istio_types.go @@ -36,16 +36,18 @@ const ( ConditionTypeProxySidecarRestartSucceeded ConditionType = "ProxySidecarRestartSucceeded" // general - ConditionReasonReconcileSucceeded ConditionReason = "ReconcileSucceeded" - ConditionReasonReconcileSucceededMessage = "Reconciliation succeeded" - ConditionReasonReconcileUnknown ConditionReason = "ReconcileUnknown" - ConditionReasonReconcileUnknownMessage = "Module readiness is unknown. Either a reconciliation is progressing, or failed previously. Check status of other conditions" - ConditionReasonReconcileFailed ConditionReason = "ReconcileFailed" - ConditionReasonReconcileFailedMessage = "Reconciliation failed" - ConditionReasonValidationFailed ConditionReason = "ValidationFailed" - ConditionReasonValidationFailedMessage = "Reconciliation did not happen as Istio Custom Resource failed to validate" - ConditionReasonOlderCRExists ConditionReason = "OlderCRExists" - ConditionReasonOlderCRExistsMessage = "This Istio custom resource is not the oldest one and does not represent the module state" + ConditionReasonReconcileSucceeded ConditionReason = "ReconcileSucceeded" + ConditionReasonReconcileSucceededMessage = "Reconciliation succeeded" + ConditionReasonReconcileUnknown ConditionReason = "ReconcileUnknown" + ConditionReasonReconcileUnknownMessage = "Module readiness is unknown. Either a reconciliation is progressing, or failed previously. Check status of other conditions" + ConditionReasonReconcileProxyResetRequeued ConditionReason = "ProxyResetRequeued" + ConditionReasonReconcileProxyResetRequeuedMessage = "Proxy reset is still ongoing. Reconciliation requeued" + ConditionReasonReconcileFailed ConditionReason = "ReconcileFailed" + ConditionReasonReconcileFailedMessage = "Reconciliation failed" + ConditionReasonValidationFailed ConditionReason = "ValidationFailed" + ConditionReasonValidationFailedMessage = "Reconciliation did not happen as Istio Custom Resource failed to validate" + ConditionReasonOlderCRExists ConditionReason = "OlderCRExists" + ConditionReasonOlderCRExistsMessage = "This Istio custom resource is not the oldest one and does not represent the module state" // install / uninstall ConditionReasonIstioInstallNotNeeded ConditionReason = "IstioInstallNotNeeded" diff --git a/controllers/istio_controller.go b/controllers/istio_controller.go index 5f4c03987..0997a8fe2 100644 --- a/controllers/istio_controller.go +++ b/controllers/istio_controller.go @@ -156,6 +156,7 @@ func (r *IstioReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl } return ctrl.Result{}, err } else if requeue { + r.statusHandler.SetCondition(&istioCR, operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonReconcileProxyResetRequeued)) return r.requeueReconciliationWithoutError(ctx, &istioCR) } diff --git a/controllers/istio_controller_test.go b/controllers/istio_controller_test.go index 01b2d0071..e8fa65e24 100644 --- a/controllers/istio_controller_test.go +++ b/controllers/istio_controller_test.go @@ -996,13 +996,13 @@ var _ = Describe("Istio Controller", func() { Expect(updatedIstioCR.Status.State).To(Equal(operatorv1alpha2.Processing)) - By("Verifying that Istio CR has Condition Ready status with Unknown") + By("Verifying that Istio CR has Condition Ready status with ProxyResetRequeued 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.ConditionReasonReconcileUnknown))) - Expect((*updatedIstioCR.Status.Conditions)[0].Message).To(Equal(operatorv1alpha2.ConditionReasonReconcileUnknownMessage)) - Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionUnknown)) + Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonReconcileProxyResetRequeued))) + Expect((*updatedIstioCR.Status.Conditions)[0].Message).To(Equal(operatorv1alpha2.ConditionReasonReconcileProxyResetRequeuedMessage)) + Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse)) }) }) }) diff --git a/docs/contributor/04-10-technical-design.md b/docs/contributor/04-10-technical-design.md index 7ccc0c759..8b57f4ad0 100644 --- a/docs/contributor/04-10-technical-design.md +++ b/docs/contributor/04-10-technical-design.md @@ -114,6 +114,10 @@ The predicate initiates a restart of the sidecar and Ingress Gateway if the targ The SidecarsRestarter is responsible for keeping the proxy sidecars in the desired state. It restarts Pods that are in running state and are part of the service mesh having the annotation `sidecar.istio.io/status`. The Istio CR and [Istio Version](#istio-version) represent the desired state. Pods will be restarted in chunks with limits on how many to restart with one reconciliation and how many to list when requesting from Kubernetes API Server. If more Pods need to be restarted this will happen in next reconciliation. This happens with requeuing the reconciliation request, which will be imediately rescheduled. +During proxy sidecars restarting phase, Istio CR will be kept in `Processing` state having following status conditions: +- `Ready` condition set to `false` with `ProxyResetRequeued` reason. +- `ProxySidecarRestartSucceeded` condition set to `false` with `ProxySidecarPartiallySucceeded` reason. + This component covers the following restart triggers: - Restart Pods with proxy sidecar when CNI config changes. diff --git a/pkg/lib/sidecars/pods/get.go b/pkg/lib/sidecars/pods/get.go index aebed8bf8..82c7b61d4 100644 --- a/pkg/lib/sidecars/pods/get.go +++ b/pkg/lib/sidecars/pods/get.go @@ -71,7 +71,7 @@ func getSidecarPods(ctx context.Context, c client.Client, logger *logr.Logger, l return nil, err } - logger.Info("Retrieved running pods for proxy restart", "number of pods", len(podList.Items), "has more pods", podList.Continue != "") + logger.Info("Got running pods for proxy restart", "number of pods", len(podList.Items), "has more pods", podList.Continue != "") podsWithSidecar := &v1.PodList{} podsWithSidecar.Continue = podList.Continue @@ -83,7 +83,6 @@ func getSidecarPods(ctx context.Context, c client.Client, logger *logr.Logger, l } logger.Info("Filtered pods with Istio sidecar", "number of pods", len(podsWithSidecar.Items)) - return podsWithSidecar, nil } From 25fcfb82dbe052052606de94e2e642f0ee3b7c82 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Mon, 15 Jul 2024 15:42:38 +0200 Subject: [PATCH 14/22] Condition renamed --- api/v1alpha2/conditions.go | 12 +++++------ api/v1alpha2/istio_types.go | 24 +++++++++++----------- controllers/istio_controller.go | 2 +- controllers/istio_controller_test.go | 6 +++--- docs/contributor/04-10-technical-design.md | 2 +- 5 files changed, 23 insertions(+), 23 deletions(-) diff --git a/api/v1alpha2/conditions.go b/api/v1alpha2/conditions.go index 8d640d3f1..a23b08dab 100644 --- a/api/v1alpha2/conditions.go +++ b/api/v1alpha2/conditions.go @@ -20,12 +20,12 @@ func ConditionFromReason(reason ReasonWithMessage) *metav1.Condition { } 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}, - ConditionReasonReconcileProxyResetRequeued: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonReconcileProxyResetRequeuedMessage}, - ConditionReasonValidationFailed: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonValidationFailedMessage}, - ConditionReasonOlderCRExists: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonOlderCRExistsMessage}, + 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}, ConditionReasonIstioInstallNotNeeded: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonIstioInstallNotNeededMessage}, ConditionReasonIstioInstallSucceeded: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonIstioInstallSucceededMessage}, diff --git a/api/v1alpha2/istio_types.go b/api/v1alpha2/istio_types.go index cbaca5a98..db3826ebb 100644 --- a/api/v1alpha2/istio_types.go +++ b/api/v1alpha2/istio_types.go @@ -36,18 +36,18 @@ const ( ConditionTypeProxySidecarRestartSucceeded ConditionType = "ProxySidecarRestartSucceeded" // general - ConditionReasonReconcileSucceeded ConditionReason = "ReconcileSucceeded" - ConditionReasonReconcileSucceededMessage = "Reconciliation succeeded" - ConditionReasonReconcileUnknown ConditionReason = "ReconcileUnknown" - ConditionReasonReconcileUnknownMessage = "Module readiness is unknown. Either a reconciliation is progressing, or failed previously. Check status of other conditions" - ConditionReasonReconcileProxyResetRequeued ConditionReason = "ProxyResetRequeued" - ConditionReasonReconcileProxyResetRequeuedMessage = "Proxy reset is still ongoing. Reconciliation requeued" - ConditionReasonReconcileFailed ConditionReason = "ReconcileFailed" - ConditionReasonReconcileFailedMessage = "Reconciliation failed" - ConditionReasonValidationFailed ConditionReason = "ValidationFailed" - ConditionReasonValidationFailedMessage = "Reconciliation did not happen as Istio Custom Resource failed to validate" - ConditionReasonOlderCRExists ConditionReason = "OlderCRExists" - ConditionReasonOlderCRExistsMessage = "This Istio custom resource is not the oldest one and does not represent the module state" + ConditionReasonReconcileSucceeded ConditionReason = "ReconcileSucceeded" + 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" + ConditionReasonValidationFailedMessage = "Reconciliation did not happen as Istio Custom Resource failed to validate" + ConditionReasonOlderCRExists ConditionReason = "OlderCRExists" + ConditionReasonOlderCRExistsMessage = "This Istio custom resource is not the oldest one and does not represent the module state" // install / uninstall ConditionReasonIstioInstallNotNeeded ConditionReason = "IstioInstallNotNeeded" diff --git a/controllers/istio_controller.go b/controllers/istio_controller.go index 0997a8fe2..00c813802 100644 --- a/controllers/istio_controller.go +++ b/controllers/istio_controller.go @@ -156,7 +156,7 @@ func (r *IstioReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl } return ctrl.Result{}, err } else if requeue { - r.statusHandler.SetCondition(&istioCR, operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonReconcileProxyResetRequeued)) + r.statusHandler.SetCondition(&istioCR, operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonReconcileRequeued)) return r.requeueReconciliationWithoutError(ctx, &istioCR) } diff --git a/controllers/istio_controller_test.go b/controllers/istio_controller_test.go index e8fa65e24..adf5f0d39 100644 --- a/controllers/istio_controller_test.go +++ b/controllers/istio_controller_test.go @@ -996,12 +996,12 @@ var _ = Describe("Istio Controller", func() { Expect(updatedIstioCR.Status.State).To(Equal(operatorv1alpha2.Processing)) - By("Verifying that Istio CR has Condition Ready status with ProxyResetRequeued reason") + 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.ConditionReasonReconcileProxyResetRequeued))) - Expect((*updatedIstioCR.Status.Conditions)[0].Message).To(Equal(operatorv1alpha2.ConditionReasonReconcileProxyResetRequeuedMessage)) + 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)) }) }) diff --git a/docs/contributor/04-10-technical-design.md b/docs/contributor/04-10-technical-design.md index 8b57f4ad0..9d6656902 100644 --- a/docs/contributor/04-10-technical-design.md +++ b/docs/contributor/04-10-technical-design.md @@ -115,7 +115,7 @@ The SidecarsRestarter is responsible for keeping the proxy sidecars in the desir The Istio CR and [Istio Version](#istio-version) represent the desired state. Pods will be restarted in chunks with limits on how many to restart with one reconciliation and how many to list when requesting from Kubernetes API Server. If more Pods need to be restarted this will happen in next reconciliation. This happens with requeuing the reconciliation request, which will be imediately rescheduled. During proxy sidecars restarting phase, Istio CR will be kept in `Processing` state having following status conditions: -- `Ready` condition set to `false` with `ProxyResetRequeued` reason. +- `Ready` condition set to `false` with `Requeued` reason. - `ProxySidecarRestartSucceeded` condition set to `false` with `ProxySidecarPartiallySucceeded` reason. This component covers the following restart triggers: From fa15968e5539bfd6755994f64d12cdf8a4d2372c Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Mon, 15 Jul 2024 15:47:08 +0200 Subject: [PATCH 15/22] Reason name corrected --- docs/contributor/04-10-technical-design.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributor/04-10-technical-design.md b/docs/contributor/04-10-technical-design.md index 9d6656902..3218cb4b6 100644 --- a/docs/contributor/04-10-technical-design.md +++ b/docs/contributor/04-10-technical-design.md @@ -115,7 +115,7 @@ The SidecarsRestarter is responsible for keeping the proxy sidecars in the desir The Istio CR and [Istio Version](#istio-version) represent the desired state. Pods will be restarted in chunks with limits on how many to restart with one reconciliation and how many to list when requesting from Kubernetes API Server. If more Pods need to be restarted this will happen in next reconciliation. This happens with requeuing the reconciliation request, which will be imediately rescheduled. During proxy sidecars restarting phase, Istio CR will be kept in `Processing` state having following status conditions: -- `Ready` condition set to `false` with `Requeued` reason. +- `Ready` condition set to `false` with `ReconcileRequeued` reason. - `ProxySidecarRestartSucceeded` condition set to `false` with `ProxySidecarPartiallySucceeded` reason. This component covers the following restart triggers: From 1d0cc884a61c8221c4915461999e5d440e73461c Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Wed, 17 Jul 2024 12:49:46 +0200 Subject: [PATCH 16/22] Add grace period for next reconciliation when more pods need to be restarted --- controllers/istio_controller.go | 2 +- controllers/istio_controller_test.go | 1 + docs/contributor/04-10-technical-design.md | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/controllers/istio_controller.go b/controllers/istio_controller.go index 00c813802..0d1250a78 100644 --- a/controllers/istio_controller.go +++ b/controllers/istio_controller.go @@ -182,7 +182,7 @@ func (r *IstioReconciler) requeueReconciliationWithoutError(ctx context.Context, r.log.Error(statusUpdateErr, "Error during updating status to error") } r.log.Info("Reconcile requeued") - return ctrl.Result{Requeue: true}, nil + return ctrl.Result{Requeue: true, RequeueAfter: time.Minute * 1}, nil } // terminateReconciliation stops the reconciliation and does not requeue the request. diff --git a/controllers/istio_controller_test.go b/controllers/istio_controller_test.go index adf5f0d39..cc6e17be9 100644 --- a/controllers/istio_controller_test.go +++ b/controllers/istio_controller_test.go @@ -986,6 +986,7 @@ var _ = Describe("Istio Controller", func() { //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()) diff --git a/docs/contributor/04-10-technical-design.md b/docs/contributor/04-10-technical-design.md index 3218cb4b6..38106803b 100644 --- a/docs/contributor/04-10-technical-design.md +++ b/docs/contributor/04-10-technical-design.md @@ -112,7 +112,7 @@ The predicate initiates a restart of the sidecar and Ingress Gateway if the targ ### SidecarsRestarter The SidecarsRestarter is responsible for keeping the proxy sidecars in the desired state. It restarts Pods that are in running state and are part of the service mesh having the annotation `sidecar.istio.io/status`. -The Istio CR and [Istio Version](#istio-version) represent the desired state. Pods will be restarted in chunks with limits on how many to restart with one reconciliation and how many to list when requesting from Kubernetes API Server. If more Pods need to be restarted this will happen in next reconciliation. This happens with requeuing the reconciliation request, which will be imediately rescheduled. +The Istio CR and [Istio Version](#istio-version) represent the desired state. Pods will be restarted in chunks with limits on how many to restart with one reconciliation and how many to list when requesting from Kubernetes API Server. If more Pods need to be restarted this will happen in next reconciliation. This happens with requeuing the reconciliation request with a delay of 1 minute to give time for the Kubernetes scheduler to restart respective deployments. During proxy sidecars restarting phase, Istio CR will be kept in `Processing` state having following status conditions: - `Ready` condition set to `false` with `ReconcileRequeued` reason. From a2818b74dc8c18f4efa169a542701d07a9722866 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 18 Jul 2024 09:29:32 +0200 Subject: [PATCH 17/22] Update docs/contributor/04-10-technical-design.md Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com> --- docs/contributor/04-10-technical-design.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributor/04-10-technical-design.md b/docs/contributor/04-10-technical-design.md index 38106803b..5098b1a89 100644 --- a/docs/contributor/04-10-technical-design.md +++ b/docs/contributor/04-10-technical-design.md @@ -111,7 +111,7 @@ The predicate initiates a restart of the sidecar and Ingress Gateway if the targ ### SidecarsRestarter -The SidecarsRestarter is responsible for keeping the proxy sidecars in the desired state. It restarts Pods that are in running state and are part of the service mesh having the annotation `sidecar.istio.io/status`. +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 [Istio Version](#istio-version) represent the desired state. Pods will be restarted in chunks with limits on how many to restart with one reconciliation and how many to list when requesting from Kubernetes API Server. If more Pods need to be restarted this will happen in next reconciliation. This happens with requeuing the reconciliation request with a delay of 1 minute to give time for the Kubernetes scheduler to restart respective deployments. During proxy sidecars restarting phase, Istio CR will be kept in `Processing` state having following status conditions: From 9f36ac78767fb16ffc507ef5308f020f570a647a Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 18 Jul 2024 09:29:38 +0200 Subject: [PATCH 18/22] Update docs/contributor/04-10-technical-design.md Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com> --- docs/contributor/04-10-technical-design.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributor/04-10-technical-design.md b/docs/contributor/04-10-technical-design.md index 5098b1a89..c85bd722e 100644 --- a/docs/contributor/04-10-technical-design.md +++ b/docs/contributor/04-10-technical-design.md @@ -112,7 +112,7 @@ The predicate initiates a restart of the sidecar and Ingress Gateway if the targ ### SidecarsRestarter 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 [Istio Version](#istio-version) represent the desired state. Pods will be restarted in chunks with limits on how many to restart with one reconciliation and how many to list when requesting from Kubernetes API Server. If more Pods need to be restarted this will happen in next reconciliation. This happens with requeuing the reconciliation request with a delay of 1 minute to give time for the Kubernetes scheduler to restart respective deployments. +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 proxy sidecars restarting phase, Istio CR will be kept in `Processing` state having following status conditions: - `Ready` condition set to `false` with `ReconcileRequeued` reason. From 947f28ca00aa6fd0b3cbbe3624549f4339834889 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 18 Jul 2024 09:29:46 +0200 Subject: [PATCH 19/22] Update docs/release-notes/1.9.0.md Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com> --- docs/release-notes/1.9.0.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/release-notes/1.9.0.md b/docs/release-notes/1.9.0.md index 372b4d02b..05b2a0b7f 100644 --- a/docs/release-notes/1.9.0.md +++ b/docs/release-notes/1.9.0.md @@ -1,5 +1,5 @@ ## New Features -- Pods with proxy sidecars will be restarted in chunks on Istio upgrade in multiple reconciliations and not all at once. This will increase stability and reliability of the reconciliation for the Istio module operator. [issue #155](https://github.com/kyma-project/istio/issues/155) +- During the Istio upgrade, Pods with the Istio sidecar proxies will now be divided into smaller groups and restarted in multiple reconciliations instead of all at once. This will increase the stability and reliability of the reconciliation for the Istio module's operator. See the [issue #155](https://github.com/kyma-project/istio/issues/155). From 4c17ae885fa627635e33a2e31a985615f660b1eb Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 18 Jul 2024 09:29:52 +0200 Subject: [PATCH 20/22] Update docs/contributor/04-10-technical-design.md Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com> --- docs/contributor/04-10-technical-design.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributor/04-10-technical-design.md b/docs/contributor/04-10-technical-design.md index c85bd722e..afd0463f5 100644 --- a/docs/contributor/04-10-technical-design.md +++ b/docs/contributor/04-10-technical-design.md @@ -116,7 +116,7 @@ The Istio CR and the [Istio version](#istio-version) represent the desired state During proxy sidecars restarting phase, Istio CR will be kept in `Processing` state having following status conditions: - `Ready` condition set to `false` with `ReconcileRequeued` reason. -- `ProxySidecarRestartSucceeded` condition set to `false` with `ProxySidecarPartiallySucceeded` reason. +- The `ProxySidecarRestartSucceeded` condition is set to `false` with the reason `ProxySidecarPartiallySucceeded`. This component covers the following restart triggers: From ae7d10ec009a48b630d3cb65dbb1a000d1e06200 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 18 Jul 2024 09:29:57 +0200 Subject: [PATCH 21/22] Update docs/contributor/04-10-technical-design.md Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com> --- docs/contributor/04-10-technical-design.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributor/04-10-technical-design.md b/docs/contributor/04-10-technical-design.md index afd0463f5..4dfc362da 100644 --- a/docs/contributor/04-10-technical-design.md +++ b/docs/contributor/04-10-technical-design.md @@ -115,7 +115,7 @@ The SidecarsRestarter is responsible for keeping the proxy sidecars in the desir 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 proxy sidecars restarting phase, Istio CR will be kept in `Processing` state having following status conditions: -- `Ready` condition set to `false` with `ReconcileRequeued` reason. +- 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: From e305fd7381d4a9d3351668ccb9c8e40bf2193a9a Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 18 Jul 2024 09:52:11 +0200 Subject: [PATCH 22/22] Update docs/contributor/04-10-technical-design.md Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com> --- docs/contributor/04-10-technical-design.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributor/04-10-technical-design.md b/docs/contributor/04-10-technical-design.md index 4dfc362da..a9b8f023b 100644 --- a/docs/contributor/04-10-technical-design.md +++ b/docs/contributor/04-10-technical-design.md @@ -114,7 +114,7 @@ The predicate initiates a restart of the sidecar and Ingress Gateway if the targ 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 proxy sidecars restarting phase, Istio CR will be kept in `Processing` state having following status conditions: +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`.