Skip to content

Commit

Permalink
Proxy reset with pagination and reconciliation requeue (#926)
Browse files Browse the repository at this point in the history 
* Initial

* Refactor

* State

* State

* State

* Pods get refined with mocked test with limit

* Tests

* Fix lint

* Release notes

* Have separate limits to list and restart

* Fix lint

* Docs

* Conditions update

* Condition renamed

* Reason name corrected

* Add grace period for next reconciliation when more pods need to be restarted

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

Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com>

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

Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com>

* Update docs/release-notes/1.9.0.md

Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com>

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

Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com>

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

Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com>

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

Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com>

---------

Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com>
  • Loading branch information
@videlov @nataliasitko
videlov and nataliasitko authored Jul 18, 2024
1 parent 730fc36 commit 4d26ebd
Show file tree
Hide file tree
Showing 29 changed files with 649 additions and 339 deletions.
8 changes: 5 additions & 3 deletions api/v1alpha2/conditions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ var conditionReasons = map[ConditionReason]conditionMeta{
ConditionReasonReconcileSucceeded: {Type: ConditionTypeReady, Status: metav1.ConditionTrue, Message: ConditionReasonReconcileSucceededMessage},
ConditionReasonReconcileFailed: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonReconcileFailedMessage},
ConditionReasonReconcileUnknown: {Type: ConditionTypeReady, Status: metav1.ConditionUnknown, Message: ConditionReasonReconcileUnknownMessage},
ConditionReasonReconcileRequeued: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonReconcileRequeuedMessage},
ConditionReasonValidationFailed: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonValidationFailedMessage},
ConditionReasonOlderCRExists: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonOlderCRExistsMessage},

Expand All @@ -37,9 +38,10 @@ var conditionReasons = map[ConditionReason]conditionMeta{
ConditionReasonCRsReconcileSucceeded: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonCRsReconcileSucceededMessage},
ConditionReasonCRsReconcileFailed: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonCRsReconcileFailedMessage},

ConditionReasonProxySidecarRestartSucceeded: {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionTrue, Message: ConditionReasonProxySidecarRestartSucceededMessage},
ConditionReasonProxySidecarRestartFailed: {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarRestartFailedMessage},
ConditionReasonProxySidecarManualRestartRequired: {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarManualRestartRequiredMessage},
ConditionReasonProxySidecarRestartSucceeded: {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionTrue, Message: ConditionReasonProxySidecarRestartSucceededMessage},
ConditionReasonProxySidecarRestartFailed: {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarRestartFailedMessage},
ConditionReasonProxySidecarRestartPartiallySucceeded: {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarRestartPartiallySucceededMessage},
ConditionReasonProxySidecarManualRestartRequired: {Type: ConditionTypeProxySidecarRestartSucceeded, Status: metav1.ConditionFalse, Message: ConditionReasonProxySidecarManualRestartRequiredMessage},

ConditionReasonIngressGatewayRestartSucceeded: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonIngressGatewayRestartSucceededMessage},
ConditionReasonIngressGatewayRestartFailed: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonIngressGatewayRestartFailedMessage},
Expand Down
16 changes: 10 additions & 6 deletions api/v1alpha2/istio_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ const (
ConditionReasonReconcileSucceededMessage = "Reconciliation succeeded"
ConditionReasonReconcileUnknown ConditionReason = "ReconcileUnknown"
ConditionReasonReconcileUnknownMessage = "Module readiness is unknown. Either a reconciliation is progressing, or failed previously. Check status of other conditions"
ConditionReasonReconcileRequeued ConditionReason = "ReconcileRequeued"
ConditionReasonReconcileRequeuedMessage = "Proxy reset is still ongoing. Reconciliation requeued"
ConditionReasonReconcileFailed ConditionReason = "ReconcileFailed"
ConditionReasonReconcileFailedMessage = "Reconciliation failed"
ConditionReasonValidationFailed ConditionReason = "ValidationFailed"
Expand Down Expand Up @@ -70,12 +72,14 @@ const (
ConditionReasonCRsReconcileFailedMessage = "Custom resources reconciliation failed"

// proxy reset
ConditionReasonProxySidecarRestartSucceeded ConditionReason = "ProxySidecarRestartSucceeded"
ConditionReasonProxySidecarRestartSucceededMessage = "Proxy sidecar restart succeeded"
ConditionReasonProxySidecarRestartFailed ConditionReason = "ProxySidecarRestartFailed"
ConditionReasonProxySidecarRestartFailedMessage = "Proxy sidecar restart failed"
ConditionReasonProxySidecarManualRestartRequired ConditionReason = "ProxySidecarManualRestartRequired"
ConditionReasonProxySidecarManualRestartRequiredMessage = "Proxy sidecar manual restart is required for some workloads"
ConditionReasonProxySidecarRestartSucceeded ConditionReason = "ProxySidecarRestartSucceeded"
ConditionReasonProxySidecarRestartSucceededMessage = "Proxy sidecar restart succeeded"
ConditionReasonProxySidecarRestartFailed ConditionReason = "ProxySidecarRestartFailed"
ConditionReasonProxySidecarRestartFailedMessage = "Proxy sidecar restart failed"
ConditionReasonProxySidecarRestartPartiallySucceeded ConditionReason = "ProxySidecarRestartPartiallySucceeded"
ConditionReasonProxySidecarRestartPartiallySucceededMessage = "Proxy sidecar restart partially succeeded"
ConditionReasonProxySidecarManualRestartRequired ConditionReason = "ProxySidecarManualRestartRequired"
ConditionReasonProxySidecarManualRestartRequiredMessage = "Proxy sidecar manual restart is required for some workloads"

// ingress gateway
ConditionReasonIngressGatewayRestartSucceeded ConditionReason = "IngressGatewayRestartSucceeded"
Expand Down
16 changes: 14 additions & 2 deletions controllers/istio_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,14 +146,18 @@ func (r *IstioReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
return r.requeueReconciliation(ctx, &istioCR, resourcesErr, operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonCRsReconcileFailed))
}

if err := restarter.Restart(ctx, &istioCR, r.restarters); err != nil {
err, requeue := restarter.Restart(ctx, &istioCR, r.restarters)
if err != nil {
// We don't want to use the requeueReconciliation function here, since there is condition handling in this function, and we
// need to clean this up, before we can use it here as conditions are already handled in the restarters.
statusUpdateErr := r.statusHandler.UpdateToError(ctx, &istioCR, err)
if statusUpdateErr != nil {
r.log.Error(statusUpdateErr, "Error during updating status to error")
}
return ctrl.Result{}, err
} else if requeue {
r.statusHandler.SetCondition(&istioCR, operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonReconcileRequeued))
return r.requeueReconciliationWithoutError(ctx, &istioCR)
}

return r.finishReconcile(ctx, &istioCR, istioImageVersion.Tag())
Expand All @@ -168,11 +172,19 @@ func (r *IstioReconciler) requeueReconciliation(ctx context.Context, istioCR *op
if statusUpdateErr != nil {
r.log.Error(statusUpdateErr, "Error during updating status to error")
}

r.log.Error(err, "Reconcile failed")
return ctrl.Result{}, err
}

func (r *IstioReconciler) requeueReconciliationWithoutError(ctx context.Context, istioCR *operatorv1alpha2.Istio) (ctrl.Result, error) {
statusUpdateErr := r.statusHandler.UpdateToProcessing(ctx, istioCR)
if statusUpdateErr != nil {
r.log.Error(statusUpdateErr, "Error during updating status to error")
}
r.log.Info("Reconcile requeued")
return ctrl.Result{Requeue: true, RequeueAfter: time.Minute * 1}, nil
}

// terminateReconciliation stops the reconciliation and does not requeue the request.
func (r *IstioReconciler) terminateReconciliation(ctx context.Context, istioCR *operatorv1alpha2.Istio, err described_errors.DescribedError, reason operatorv1alpha2.ReasonWithMessage) (ctrl.Result, error) {
if err.ShouldSetCondition() {
Expand Down
72 changes: 59 additions & 13 deletions controllers/istio_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand All @@ -766,7 +758,6 @@ var _ = Describe("Istio Controller", func() {
})

Context("Restarters", func() {

It("should restart if reconciliations are successful", func() {
//given
istioCR := &operatorv1alpha2.Istio{
Expand Down Expand Up @@ -960,22 +951,77 @@ var _ = Describe("Istio Controller", func() {
Expect(updatedIstioCR.Status.State).Should(Equal(operatorv1alpha2.Warning))
Expect(updatedIstioCR.Status.Description).To(ContainSubstring("Restart Warning description"))
})

It("should requeue reconcile request when a restarter needs to finish work on the next reconcile", func() {
//given
istioCR := &operatorv1alpha2.Istio{
ObjectMeta: metav1.ObjectMeta{
Name: istioCrName,
Namespace: testNamespace,
UID: "1",
CreationTimestamp: metav1.Unix(1494505756, 0),
Finalizers: []string{
"istios.operator.kyma-project.io/istio-installation",
},
},
}

fakeClient := createFakeClient(istioCR)
ingressGatewayRestarter := &restarterMock{restarted: false}
proxySidecarsRestarter := &restarterMock{restarted: false, requeue: true}
sut := &IstioReconciler{
Client: fakeClient,
Scheme: getTestScheme(),
istioInstallation: &istioInstallationReconciliationMock{},
istioResources: &istioResourcesReconciliationMock{},
restarters: []restarter.Restarter{ingressGatewayRestarter, proxySidecarsRestarter},
log: logr.Discard(),
statusHandler: status.NewStatusHandler(fakeClient),
reconciliationInterval: testReconciliationInterval,
}

//when
reconcileResult, err := sut.Reconcile(context.Background(), reconcile.Request{NamespacedName: types.NamespacedName{Namespace: testNamespace, Name: istioCrName}})

//then
Expect(err).ToNot(HaveOccurred())
Expect(reconcileResult.Requeue).To(BeTrue())
Expect(reconcileResult.RequeueAfter).To(Equal(time.Minute * 1))

Expect(ingressGatewayRestarter.RestartCalled()).To(BeTrue())
Expect(proxySidecarsRestarter.RestartCalled()).To(BeTrue())

updatedIstioCR := operatorv1alpha2.Istio{}
err = fakeClient.Get(context.Background(), client.ObjectKeyFromObject(istioCR), &updatedIstioCR)
Expect(err).To(Not(HaveOccurred()))

Expect(updatedIstioCR.Status.State).To(Equal(operatorv1alpha2.Processing))

By("Verifying that Istio CR has Condition Ready status with Requeued reason")
Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(1))
Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonReconcileRequeued)))
Expect((*updatedIstioCR.Status.Conditions)[0].Message).To(Equal(operatorv1alpha2.ConditionReasonReconcileRequeuedMessage))
Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse))
})
})
})
})

type restarterMock struct {
err described_errors.DescribedError
requeue bool
restarted bool
}

func (i *restarterMock) RestartCalled() bool {
return i.restarted
}

func (i *restarterMock) Restart(_ context.Context, _ *operatorv1alpha2.Istio) described_errors.DescribedError {
func (i *restarterMock) Restart(_ context.Context, _ *operatorv1alpha2.Istio) (described_errors.DescribedError, bool) {
i.restarted = true
return i.err
return i.err, i.requeue
}

type istioResourcesReconciliationMock struct {
Expand Down
11 changes: 7 additions & 4 deletions docs/contributor/04-10-technical-design.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,17 +103,20 @@ Additionally, clear status reporting and the ability to provide detailed informa

### Restart Predicates

The [SidecarRestarter](#sidecarsrestarter) and [IngressGatewayRestarter](#ingressgatewayrestarter) components use Restart Predicates.
The [SidecarRestarter](#sidecarsrestarter) and [IngressGatewayRestarter](#ingressgatewayrestarter) components use Restart Predicates.
Depending on the implemented interfaces, a predicate can trigger a restart of Ingress Gateways, Proxy Sidecars, or both Ingress Gateways and Proxy Sidecars.

For cases where it isn't trivial to check whether the configuration has been applied to the cluster state, Restart Predicates use a timestamp-based approach. For example, the `envoy_filter_allow_partial_referer` resource has the `istios.operator.kyma-project.io/updatedAt` annotation, which includes the timestamp of its last update.
The predicate initiates a restart of the sidecar and Ingress Gateway if the target was created before this timestamp.

### SidecarsRestarter

The SidecarsRestarter is responsible for keeping the proxy sidecars in the desired state. It restarts Pods that are part of the service mesh or
that must be added to the service mesh.
The Istio CR and [Istio Version](#istio-version) represent the desired state.
The SidecarsRestarter is responsible for keeping the proxy sidecars in the desired state. It restarts Pods that are in the `Running` state, are part of the service mesh, and have the annotation `sidecar.istio.io/status`.
The Istio CR and the [Istio version](#istio-version) represent the desired state. Pods are restarted in chunks with limits on the number that can be restarted in one reconciliation and the number that can be listed when requesting from the Kubernetes API Server. If the number of Pods that must be restarted exceeds the limits, it happens in the next reconciliation. In such a case, the reconciliation request is requeued with a 1-minute delay to allow time for the Kubernetes scheduler to restart the Deployments.

During the proxy sidecars restarting phase, the Istio CR remains in the `Processing` state having the following status conditions:
- The `Ready` condition is set to `false` with the reason `ReconcileRequeued`.
- The `ProxySidecarRestartSucceeded` condition is set to `false` with the reason `ProxySidecarPartiallySucceeded`.

This component covers the following restart triggers:

Expand Down
Loading

0 comments on commit 4d26ebd

Please sign in to comment.