diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 04ab542b6..fac1726db 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -12,7 +12,7 @@ configMapGenerator: - GCP_CLIENT_RENEW_DURATION=5m - GCP_RETRY_WAIT_DURATION=5s - GCP_OPERATION_WAIT_DURATION=5s - - GCP_API_TIMEOUT_DURATION=3s + - GCP_API_TIMEOUT_DURATION=8s name: manager-env secretGenerator: diff --git a/pkg/kcp/provider/gcp/client/gcpConstants.go b/pkg/kcp/provider/gcp/client/gcpConstants.go index d8a60143b..c35f15f6e 100644 --- a/pkg/kcp/provider/gcp/client/gcpConstants.go +++ b/pkg/kcp/provider/gcp/client/gcpConstants.go @@ -20,7 +20,7 @@ const fileBackupPattern = "projects/%s/locations/%s/backups/%s" const GcpRetryWaitTime = time.Second * 3 const GcpOperationWaitTime = time.Second * 5 -const GcpApiTimeout = time.Second * 3 +const GcpApiTimeout = time.Second * 8 type GcpConfig struct { GcpRetryWaitTime time.Duration @@ -95,6 +95,7 @@ type FilestoreState string const ( READY FilestoreState = "READY" DELETING FilestoreState = "DELETING" + ERROR FilestoreState = "ERROR" ) type OperationType int diff --git a/pkg/kcp/provider/gcp/nfsinstance/checkNUpdateState.go b/pkg/kcp/provider/gcp/nfsinstance/checkNUpdateState.go index 3a4c78929..5448fb41e 100644 --- a/pkg/kcp/provider/gcp/nfsinstance/checkNUpdateState.go +++ b/pkg/kcp/provider/gcp/nfsinstance/checkNUpdateState.go @@ -35,18 +35,24 @@ func checkNUpdateState(ctx context.Context, st composed.State) (error, context.C //If filestore doesn't exist, add it. state.operation = client.ADD state.curState = client.Creating + } else if state.fsInstance.State == string(client.ERROR) { + state.curState = v1beta1.ErrorState } else if !state.doesFilestoreMatch() { //If the filestore exists, but does not match, update it. state.operation = client.MODIFY state.curState = client.Updating } else if state.fsInstance.State == string(client.READY) { state.curState = v1beta1.ReadyState + } else { + //If the filestore exists but is not READY or in ERROR, it is in a transient state. + return composed.StopWithRequeueDelay(state.gcpConfig.GcpRetryWaitTime), ctx } } //Update State Info prevState := nfsInstance.Status.State nfsInstance.Status.State = state.curState + logger.Info("State Info", "curState", state.curState, "Operation", state.operation) if state.curState == v1beta1.ReadyState { nfsInstance.Status.Hosts = state.fsInstance.Networks[0].IpAddresses @@ -63,6 +69,17 @@ func checkNUpdateState(ctx context.Context, st composed.State) (error, context.C SuccessError(composed.StopAndForget). Run(ctx, state) } else if prevState != state.curState { + if state.curState == v1beta1.ErrorState { + return composed.UpdateStatus(nfsInstance). + SetExclusiveConditions(metav1.Condition{ + Type: v1beta1.ConditionTypeError, + Status: metav1.ConditionTrue, + Reason: v1beta1.ReasonGcpError, + Message: state.fsInstance.StatusMessage, + }). + SuccessError(composed.StopWithRequeueDelay(state.gcpConfig.GcpRetryWaitTime)). + Run(ctx, state) + } return composed.UpdateStatus(nfsInstance). RemoveConditions(v1beta1.ConditionTypeReady). SuccessError(composed.StopWithRequeue). diff --git a/pkg/kcp/provider/gcp/nfsinstance/checkNUpdateState_test.go b/pkg/kcp/provider/gcp/nfsinstance/checkNUpdateState_test.go index 2692b2d34..beacaaeed 100644 --- a/pkg/kcp/provider/gcp/nfsinstance/checkNUpdateState_test.go +++ b/pkg/kcp/provider/gcp/nfsinstance/checkNUpdateState_test.go @@ -227,6 +227,82 @@ func (suite *checkNUpdateStateSuite) TestCheckNUpdateStateReady() { assert.Equal(suite.T(), v1beta1.ReasonReady, updatedObject.Status.Conditions[0].Reason) } +func (suite *checkNUpdateStateSuite) TestCheckNUpdateStateError() { + fakeHttpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Fail(suite.T(), "unexpected request: "+r.URL.String()) + })) + gcpNfsInstance := getGcpNfsInstanceWithoutStatus() + + factory, err := newTestStateFactory(fakeHttpServer, gcpNfsInstance) + assert.Nil(suite.T(), err) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + //Get state object with GcpNfsVolume + + testState, err := factory.newStateWith(ctx, gcpNfsInstance, "") + testState.fsInstance = &file.Instance{ + Name: "test-gcp-nfs-volume-2", + State: string(client.ERROR), + StatusMessage: "Some error", + } + assert.Nil(suite.T(), err) + defer testState.FakeHttpServer.Close() + err, _ = checkNUpdateState(ctx, testState.State) + assert.NotNil(suite.T(), err) + assert.Equal(suite.T(), composed.StopWithRequeueDelay(testState.State.gcpConfig.GcpRetryWaitTime), err) + assert.Equal(suite.T(), v1beta1.ErrorState, testState.State.curState) + assert.Equal(suite.T(), v1beta1.ErrorState, testState.State.ObjAsNfsInstance().Status.State) + + updatedObject := &v1beta1.NfsInstance{} + err = factory.kcpCluster.K8sClient().Get(ctx, types.NamespacedName{Name: gcpNfsInstance.Name, Namespace: gcpNfsInstance.Namespace}, updatedObject) + assert.Nil(suite.T(), err) + assert.Equal(suite.T(), v1beta1.ErrorState, updatedObject.Status.State) + // validate status conditions + assert.Equal(suite.T(), v1beta1.ConditionTypeError, updatedObject.Status.Conditions[0].Type) + assert.Equal(suite.T(), metav1.ConditionTrue, updatedObject.Status.Conditions[0].Status) + assert.Equal(suite.T(), v1beta1.ReasonGcpError, updatedObject.Status.Conditions[0].Reason) +} + +func (suite *checkNUpdateStateSuite) TestCheckNUpdateFilestoreStateTransient() { + fakeHttpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Fail(suite.T(), "unexpected request: "+r.URL.String()) + })) + gcpNfsInstance := getGcpNfsInstanceWithoutStatus() + + factory, err := newTestStateFactory(fakeHttpServer, gcpNfsInstance) + assert.Nil(suite.T(), err) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + //Get state object with GcpNfsVolume + + testState, err := factory.newStateWith(ctx, gcpNfsInstance, "") + testState.fsInstance = &file.Instance{ + Name: "test-gcp-nfs-volume-2", + State: "somethingElse", + Networks: []*file.NetworkConfig{ + { + IpAddresses: []string{"10.2.74.33"}, + }, + }, + FileShares: []*file.FileShareConfig{ + { + CapacityGb: int64(gcpNfsInstance.Spec.Instance.Gcp.CapacityGb), + }, + }, + } + assert.Nil(suite.T(), err) + defer testState.FakeHttpServer.Close() + err, _ = checkNUpdateState(ctx, testState.State) + assert.NotNil(suite.T(), err) + assert.Equal(suite.T(), composed.StopWithRequeueDelay(testState.State.gcpConfig.GcpRetryWaitTime), err) + assert.Equal(suite.T(), gcpNfsInstance.Status.State, testState.State.curState) + assert.Equal(suite.T(), client.NONE, testState.State.operation) +} + func TestCheckNUpdateState(t *testing.T) { suite.Run(t, new(checkNUpdateStateSuite)) } diff --git a/pkg/kcp/provider/gcp/nfsinstance/state.go b/pkg/kcp/provider/gcp/nfsinstance/state.go index ac357aa8f..40eb65047 100644 --- a/pkg/kcp/provider/gcp/nfsinstance/state.go +++ b/pkg/kcp/provider/gcp/nfsinstance/state.go @@ -64,7 +64,7 @@ func newState(nfsInstanceState types.State, fc client.FilestoreClient, gcpConfig func (s State) doesFilestoreMatch() bool { nfsInstance := s.ObjAsNfsInstance() - return s.fsInstance != nil && + return s.fsInstance != nil && len(s.fsInstance.FileShares) > 0 && s.fsInstance.FileShares[0].CapacityGb == int64(nfsInstance.Spec.Instance.Gcp.CapacityGb) } diff --git a/pkg/kcp/provider/gcp/nfsinstance/syncNfsInstance.go b/pkg/kcp/provider/gcp/nfsinstance/syncNfsInstance.go index 1560f1330..70af89284 100644 --- a/pkg/kcp/provider/gcp/nfsinstance/syncNfsInstance.go +++ b/pkg/kcp/provider/gcp/nfsinstance/syncNfsInstance.go @@ -36,6 +36,9 @@ func syncNfsInstance(ctx context.Context, st composed.State) (error, context.Con operation, err = state.filestoreClient.PatchFilestoreInstance(ctx, project, location, name, mask, state.toInstance()) case client.DELETE: operation, err = state.filestoreClient.DeleteFilestoreInstance(ctx, project, location, name) + case client.NONE: + //If the operation is not ADD, MODIFY or DELETE, continue. + return nil, nil } if err != nil { diff --git a/pkg/skr/gcpnfsvolume/deletePersistenceVolume.go b/pkg/skr/gcpnfsvolume/deletePersistenceVolume.go index ba7bc86d1..d9d021895 100644 --- a/pkg/skr/gcpnfsvolume/deletePersistenceVolume.go +++ b/pkg/skr/gcpnfsvolume/deletePersistenceVolume.go @@ -27,7 +27,7 @@ func deletePersistenceVolume(ctx context.Context, st composed.State) (error, con if state.PV.Status.Phase != "Released" && state.PV.Status.Phase != "Available" { // Only PV in Released or Available state can be deleted state.ObjAsGcpNfsVolume().Status.State = cloudresourcesv1beta1.GcpNfsVolumeError - return composed.PatchStatus(state.ObjAsGcpNfsVolume()). + return composed.UpdateStatus(state.ObjAsGcpNfsVolume()). SetExclusiveConditions(metav1.Condition{ Type: cloudresourcesv1beta1.ConditionTypeError, Status: metav1.ConditionTrue, @@ -41,7 +41,7 @@ func deletePersistenceVolume(ctx context.Context, st composed.State) (error, con for i := range state.ObjAsGcpNfsVolume().Status.Conditions { condition := (state.ObjAsGcpNfsVolume().Status.Conditions)[i] if condition.Type == cloudresourcesv1beta1.ConditionTypeError && condition.Reason == cloudresourcesv1beta1.ConditionReasonPVNotReadyForDeletion { - return composed.PatchStatus(state.ObjAsGcpNfsVolume()). + return composed.UpdateStatus(state.ObjAsGcpNfsVolume()). RemoveConditions(cloudresourcesv1beta1.ConditionTypeError). ErrorLogMessage("Error removing conditionType Error"). OnUpdateSuccess(func(ctx context.Context) (error, context.Context) {