Skip to content
This repository has been archived by the owner on Jul 4, 2024. It is now read-only.

Commit

Permalink
Fix operation processing for already deleted application (#2084)
Browse files Browse the repository at this point in the history 
* Do not return error when app is not found

* Set status to success when app is nil

* Bump component versions

* Add check for op state

* Improve impl

* Add unit test

* Improve nil handling

* Improve test name

* Return not found err in director client

* Improve handling

* Simplify error check

* Add clarification comment

* Improve reconcile for finished operations with missing apps

Co-authored-by: Nikolay Mateev <bgdunkers@gmail.com>
  • Loading branch information
@desislavaa @NickyMateev
desislavaa and NickyMateev committed Nov 22, 2021
1 parent 8cc8c7f commit c488346
Show file tree
Hide file tree
Showing 9 changed files with 229 additions and 37 deletions.
4 changes: 2 additions & 2 deletions chart/compass/values.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ global:
version: "PR-2077"
operations_controller:
dir:
version: "PR-2063"
version: "PR-2084"
ord_service:
dir:
version: "PR-47"
Expand All @@ -94,7 +94,7 @@ global:
version: "PR-2105"
system_broker:
dir:
version: "PR-2082"
version: "PR-2084"
certs_setup_job:
containerRegistry:
path: eu.gcr.io/kyma-project
Expand Down
33 changes: 25 additions & 8 deletions components/operations-controller/controllers/operation_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"strings"
"time"

"github.com/kyma-incubator/compass/components/director/pkg/str"
"github.com/kyma-incubator/compass/components/operations-controller/internal/metrics"

"github.com/kyma-incubator/compass/components/operations-controller/internal/errors"
Expand Down Expand Up @@ -186,7 +187,7 @@ func (r *OperationReconciler) handleGetOperationError(ctx context.Context, req *
}

func (r *OperationReconciler) handleFetchApplicationError(ctx context.Context, operation *v1alpha1.Operation, err error) (ctrl.Result, error) {
log.C(ctx).Error(err, "Unable to fetch application")
log.C(ctx).Error(err, fmt.Sprintf("Unable to fetch application with ID %s", operation.Spec.ResourceID))
if operation.TimeoutReached(time.Duration(r.config.TimeoutFactor) * r.config.WebhookTimeout) {
if err := r.k8sClient.Delete(ctx, operation); err != nil {
return ctrl.Result{}, err
Expand All @@ -196,6 +197,21 @@ func (r *OperationReconciler) handleFetchApplicationError(ctx context.Context, o
return ctrl.Result{}, nil
}

if isNotFoundError(err) {
if operation.Status.Phase == v1alpha1.StateSuccess || operation.Status.Phase == v1alpha1.StateFailed {
log.C(ctx).Info(fmt.Sprintf("Last state of operation for application with ID %s is %s, will not requeue", operation.Spec.ResourceID, operation.Status.Phase))
return ctrl.Result{}, nil
}

if operation.Spec.OperationType == v1alpha1.OperationTypeDelete && operation.Status.Phase == v1alpha1.StateInProgress {
return r.finalizeStatus(ctx, operation, nil, nil)
}
if operation.Spec.OperationType == v1alpha1.OperationTypeUpdate && operation.Status.Phase == v1alpha1.StateInProgress {
return r.finalizeStatus(ctx, operation, str.Ptr("Application not found in director"), nil)
}
}

// requeue in case of async create (app is not created in director yet), or a glitch in controller<->director communication
return ctrl.Result{}, err
}

Expand Down Expand Up @@ -267,7 +283,6 @@ func (r *OperationReconciler) finalizeStatus(ctx context.Context, operation *v1a
}

if errorMsg != nil && *errorMsg != "" {

r.metricsCollector.RecordError(operation.ObjectMeta.Name,
operation.Spec.CorrelationID,
string(operation.Spec.OperationType),
Expand All @@ -278,15 +293,17 @@ func (r *OperationReconciler) finalizeStatus(ctx context.Context, operation *v1a
if err := r.statusManager.FailedStatus(ctx, operation, *errorMsg); err != nil {
return ctrl.Result{}, err
}
} else {
if err := r.statusManager.SuccessStatus(ctx, operation); err != nil {
return ctrl.Result{}, err
}

duration := time.Now().Sub(operation.Status.InitializedAt.Time)
r.metricsCollector.RecordLatency(string(operation.Spec.OperationType), duration)
return ctrl.Result{}, nil
}

if err := r.statusManager.SuccessStatus(ctx, operation); err != nil {
return ctrl.Result{}, err
}

duration := time.Now().Sub(operation.Status.InitializedAt.Time)
r.metricsCollector.RecordLatency(string(operation.Spec.OperationType), duration)

return ctrl.Result{}, nil
}

Expand Down
145 changes: 145 additions & 0 deletions components/operations-controller/controllers/operation_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ const (

var (
mockedErr = errors.New("mocked error")
notFoundErr = &director.NotFoundError{}
mockedLocationURL = "https://test-domain.com/operation"
ctrlRequest = ctrl.Request{
NamespacedName: types.NamespacedName{
Expand Down Expand Up @@ -466,6 +467,150 @@ func TestReconcile_FailureToFetchApplication_And_ReconciliationTimeoutNotReached
statusMgrClient.InProgressWithPollURLAndLastPollTimestampCallCount, statusMgrClient.SuccessStatusCallCount, statusMgrClient.FailedStatusCallCount)
}

func TestReconcile_FetchApplicationReturnsNotFoundErr_And_OperationIsDeleteAndInProgress_ShouldResultSuccessOperationNoRequeueNoError(t *testing.T) {
// GIVEN:
stubLoggerAssertion(t, notFoundErr.Error(), fmt.Sprintf("Unable to fetch application with ID %s", appGUID))
defer func() { ctrl.Log = &originalLogger }()
operation := *mockedOperation
operation.Spec.OperationType = v1alpha1.OperationTypeDelete
operation.Status = v1alpha1.OperationStatus{Phase: v1alpha1.StateInProgress}

k8sClient := &controllersfakes.FakeKubernetesClient{}
k8sClient.GetReturns(&operation, nil)
k8sClient.DeleteReturns(nil)

statusMgrClient := &controllersfakes.FakeStatusManager{}
statusMgrClient.InitializeReturns(nil)

directorClient := &controllersfakes.FakeDirectorClient{}
directorClient.FetchApplicationReturns(nil, notFoundErr)

// WHEN:
controller := controllers.NewOperationReconciler(webhook.DefaultConfig(), statusMgrClient, k8sClient, directorClient, nil, collector.NewCollector())
res, err := controller.Reconcile(context.Background(), ctrlRequest)

// THEN:
// GENERAL ASSERTIONS:
require.False(t, res.Requeue)
require.Zero(t, res.RequeueAfter)

require.NoError(t, err)

// SPECIFIC CLIENT ASSERTIONS:
assertK8sGetCalledWithName(t, k8sClient, ctrlRequest.NamespacedName)
assertStatusManagerInitializeCalledWithOperation(t, statusMgrClient, &operation)
assertDirectorFetchApplicationCalled(t, directorClient, operation.Spec.ResourceID, tenantGUID)
assertStatusManagerSuccessStatusCalledWithOperation(t, statusMgrClient, &operation)
assertZeroInvocations(t, directorClient.UpdateOperationCallCount, statusMgrClient.InProgressWithPollURLCallCount,
statusMgrClient.InProgressWithPollURLAndLastPollTimestampCallCount, statusMgrClient.FailedStatusCallCount)
}

func TestReconcile_FetchApplicationReturnsNilApplication_And_OperationIsUpdateAndInProgress_ShouldResultFailedOperationNoRequeueNoError(t *testing.T) {
// GIVEN:
stubLoggerAssertion(t, notFoundErr.Error(), fmt.Sprintf("Unable to fetch application with ID %s", appGUID))
defer func() { ctrl.Log = &originalLogger }()
operation := *mockedOperation
operation.Spec.OperationType = v1alpha1.OperationTypeUpdate
operation.Status = v1alpha1.OperationStatus{Phase: v1alpha1.StateInProgress}

k8sClient := &controllersfakes.FakeKubernetesClient{}
k8sClient.GetReturns(&operation, nil)

statusMgrClient := &controllersfakes.FakeStatusManager{}
statusMgrClient.InitializeReturns(nil)

directorClient := &controllersfakes.FakeDirectorClient{}
directorClient.FetchApplicationReturns(nil, notFoundErr)

// WHEN:
controller := controllers.NewOperationReconciler(webhook.DefaultConfig(), statusMgrClient, k8sClient, directorClient, nil, collector.NewCollector())
res, err := controller.Reconcile(context.Background(), ctrlRequest)

// THEN:
// GENERAL ASSERTIONS:
require.False(t, res.Requeue)
require.Zero(t, res.RequeueAfter)

require.NoError(t, err)

// SPECIFIC CLIENT ASSERTIONS:
assertK8sGetCalledWithName(t, k8sClient, ctrlRequest.NamespacedName)
assertStatusManagerInitializeCalledWithOperation(t, statusMgrClient, &operation)
assertDirectorFetchApplicationCalled(t, directorClient, mockedOperation.Spec.ResourceID, tenantGUID)
assertStatusManagerFailedStatusCalledWithOperation(t, statusMgrClient, &operation, "Application not found in director")
assertZeroInvocations(t, k8sClient.DeleteCallCount, directorClient.UpdateOperationCallCount, statusMgrClient.InProgressWithPollURLCallCount,
statusMgrClient.InProgressWithPollURLAndLastPollTimestampCallCount, statusMgrClient.SuccessStatusCallCount)
}

func TestReconcile_FetchApplicationReturnsNilApplication_And_OperationIsNotDeleteNorInProgress_ShouldResultNoRequeueError(t *testing.T) {
// GIVEN:
stubLoggerAssertion(t, notFoundErr.Error(), fmt.Sprintf("Unable to fetch application with ID %s", appGUID))
defer func() { ctrl.Log = &originalLogger }()

k8sClient := &controllersfakes.FakeKubernetesClient{}
k8sClient.GetReturns(mockedOperation, nil)

statusMgrClient := &controllersfakes.FakeStatusManager{}
statusMgrClient.InitializeReturns(nil)

directorClient := &controllersfakes.FakeDirectorClient{}
directorClient.FetchApplicationReturns(nil, notFoundErr)

// WHEN:
controller := controllers.NewOperationReconciler(webhook.DefaultConfig(), statusMgrClient, k8sClient, directorClient, nil, collector.NewCollector())
res, err := controller.Reconcile(context.Background(), ctrlRequest)

// THEN:
// GENERAL ASSERTIONS:
require.False(t, res.Requeue)
require.Zero(t, res.RequeueAfter)

require.Error(t, err)

// SPECIFIC CLIENT ASSERTIONS:
assertK8sGetCalledWithName(t, k8sClient, ctrlRequest.NamespacedName)
assertStatusManagerInitializeCalledWithOperation(t, statusMgrClient, mockedOperation)
assertDirectorFetchApplicationCalled(t, directorClient, mockedOperation.Spec.ResourceID, tenantGUID)
assertZeroInvocations(t, k8sClient.DeleteCallCount, directorClient.UpdateOperationCallCount, statusMgrClient.InProgressWithPollURLCallCount,
statusMgrClient.InProgressWithPollURLAndLastPollTimestampCallCount, statusMgrClient.SuccessStatusCallCount, statusMgrClient.FailedStatusCallCount)
}

func TestReconcile_FetchApplicationReturnsNilApplication_And_OperationIsSucceeded_ShouldResultNoRequeueNoError(t *testing.T) {
// GIVEN:
stubLoggerAssertion(t, notFoundErr.Error(), fmt.Sprintf("Unable to fetch application with ID %s", appGUID))
defer func() { ctrl.Log = &originalLogger }()

operation := *mockedOperation
operation.Status = v1alpha1.OperationStatus{Phase: v1alpha1.StateSuccess}

k8sClient := &controllersfakes.FakeKubernetesClient{}
k8sClient.GetReturns(&operation, nil)

statusMgrClient := &controllersfakes.FakeStatusManager{}
statusMgrClient.InitializeReturns(nil)

directorClient := &controllersfakes.FakeDirectorClient{}
directorClient.FetchApplicationReturns(nil, notFoundErr)

// WHEN:
controller := controllers.NewOperationReconciler(webhook.DefaultConfig(), statusMgrClient, k8sClient, directorClient, nil, collector.NewCollector())
res, err := controller.Reconcile(context.Background(), ctrlRequest)

// THEN:
// GENERAL ASSERTIONS:
require.False(t, res.Requeue)
require.Zero(t, res.RequeueAfter)

require.NoError(t, err)

// SPECIFIC CLIENT ASSERTIONS:
assertK8sGetCalledWithName(t, k8sClient, ctrlRequest.NamespacedName)
assertStatusManagerInitializeCalledWithOperation(t, statusMgrClient, &operation)
assertDirectorFetchApplicationCalled(t, directorClient, mockedOperation.Spec.ResourceID, tenantGUID)
assertZeroInvocations(t, k8sClient.DeleteCallCount, directorClient.UpdateOperationCallCount, statusMgrClient.InProgressWithPollURLCallCount,
statusMgrClient.InProgressWithPollURLAndLastPollTimestampCallCount, statusMgrClient.SuccessStatusCallCount, statusMgrClient.FailedStatusCallCount)
}

func TestReconcile_ApplicationIsReady_And_ApplicationHasError_When_StatusManagerFailedStatusFails_ShouldResultNoRequeueError(t *testing.T) {
// GIVEN:
k8sClient := &controllersfakes.FakeKubernetesClient{}
Expand Down
8 changes: 8 additions & 0 deletions components/operations-controller/controllers/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,13 @@ package controllers
import (
"context"

"errors"

webhookdir "github.com/kyma-incubator/compass/components/director/pkg/webhook"
"github.com/kyma-incubator/compass/components/operations-controller/api/v1alpha1"
"github.com/kyma-incubator/compass/components/operations-controller/internal/director"
"github.com/kyma-incubator/compass/components/operations-controller/internal/webhook"
directorclient "github.com/kyma-incubator/compass/components/system-broker/pkg/director"
typesbroker "github.com/kyma-incubator/compass/components/system-broker/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
)
Expand Down Expand Up @@ -58,3 +61,8 @@ type WebhookClient interface {
Do(ctx context.Context, request *webhook.Request) (*webhookdir.Response, error)
Poll(ctx context.Context, request *webhook.PollRequest) (*webhookdir.ResponseStatus, error)
}

func isNotFoundError(err error) bool {
expected := &directorclient.NotFoundError{}
return errors.As(err, &expected)
}
2 changes: 1 addition & 1 deletion components/operations-controller/go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ require (
github.com/imdario/mergo v0.3.11 // indirect
github.com/json-iterator/go v1.1.11 // indirect
github.com/kyma-incubator/compass/components/director v0.0.0-20211015131944-501a5435ac7a
github.com/kyma-incubator/compass/components/system-broker v0.0.0-20210922132333-3deb7cf90637
github.com/kyma-incubator/compass/components/system-broker v0.0.0-20211020121059-e1767123c58e
github.com/machinebox/graphql v0.2.3-0.20181106130121-3a9253180225 // indirect
github.com/magiconair/properties v1.8.5 // indirect
github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 // indirect
Expand Down
Loading

0 comments on commit c488346

Please sign in to comment.