Feature group delete failed requeue (#52)

This PR assumes feature group testing code from PR #50 is already in place, and that the feature group create failed terminal condition code from PR #51 is already in place.

Summary:
Added requeue on delete if the feature group "Delete Failed" state is reached. This is because the design decision was made to retry to delete if a delete ever fails / does not complete successfully.

Files Added:
- pkg/resource/feature_group/hooks.go - Contains helper function isDeleteFailed, and ackrequeue variable requeueWaitWhileDeleteFailed which are referenced from the feature group sdk_delete template files.
- pkg/common/custom_pre_build_delete_requeue_logic.go - contains functionality of requeueUntilCanModify from old model package group hooks.go.
- pkg/common/custom_utilities.go - Contains helper function IsModifyingStatus that is used for both this PR and [PR #51](#51). Derived from previous model package group hooks.go.

Files Edited:
- generator.yaml - Added feature group hooks for sdk_delete_pre_build_request sdk_delete_post_request.
- test/e2e/tests/test_feature_group.py - Removed delete failed TODO comments.
- pkg/resource/model_package_group/hooks.go - refactored to use common code from pkg/common directory.
- templates/common/sdk_delete_post_request.go.tpl - fixed indentation and missing line at EOF.
- templates/common/sdk_delete_pre_build_request.go.tpl - fixed indentation and missing line at EOF.

Author: BriannaRoskind

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,14 +1,14 @@
 ack_generate_info:
-  build_date: "2021-07-16T17:33:59Z"
-  build_hash: 9c35918f8a98b41ada10cb3aa460dbf463428463
-  go_version: go1.16.4 darwin/amd64
+  build_date: "2021-07-22T20:36:39Z"
+  build_hash: 6911f924191f5315747c8e81fbcbf29b4edc8cb1
+  go_version: go1.16.4 linux/amd64
   version: v0.3.1
 api_directory_checksum: 4906740ff8ff6ebfffbaea971f7e8aa5a858edd8
 api_version: v1alpha1
-aws_sdk_go_version: ""
+aws_sdk_go_version: v1.38.11
 generator_config_info:
-  file_checksum: 00f02858bed96e1add2a40c7f6d165fdf94689cc
+  file_checksum: c05fa636159c1f79ab353fd790587a5b36f8b95b
   original_file_name: generator.yaml
 last_modification:
   reason: API generation
-  timestamp: 2021-07-16 17:34:06.202223 +0000 UTC
+  timestamp: 2021-07-22 20:36:41.771709478 +0000 UTC

generator.yaml

@@ -510,6 +510,10 @@ resources:
     hooks:
       delta_pre_compare:
         code: customSetDefaults(a, b)
+      sdk_delete_pre_build_request:
+        template_path: common/sdk_delete_pre_build_request.go.tpl
+      sdk_delete_post_request:
+        template_path: common/sdk_delete_post_request.go.tpl
     fields:
       FailureReason:
         is_read_only: true

pkg/common/custom_requeue.go

@@ -0,0 +1,39 @@
+// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"). You may
+// not use this file except in compliance with the License. A copy of the
+// License is located at
+//
+//     http://aws.amazon.com/apache2.0/
+//
+// or in the "license" file accompanying this file. This file is distributed
+// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package common
+
+import (
+	"errors"
+	ackrequeue "github.com/aws-controllers-k8s/runtime/pkg/requeue"
+)
+
+// RequeueIfModifying creates and returns an
+// ackrequeue if a resource's latest status matches
+// one of the provided modifying statuses.
+func RequeueIfModifying(
+	latestStatus *string,
+	resourceName *string,
+	modifyingStatuses *[]string,
+) error {
+	if latestStatus == nil || !IsModifyingStatus(latestStatus, modifyingStatuses) {
+		return nil
+	}
+
+	errMsg := *resourceName + " in " + *latestStatus + " state cannot be modified or deleted."
+	requeueWaitWhileModifying := ackrequeue.NeededAfter(
+		errors.New(errMsg),
+		ackrequeue.DefaultRequeueAfterDuration,
+	)
+	return requeueWaitWhileModifying
+}

pkg/common/custom_utilities.go

@@ -0,0 +1,29 @@
+// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"). You may
+// not use this file except in compliance with the License. A copy of the
+// License is located at
+//
+//     http://aws.amazon.com/apache2.0/
+//
+// or in the "license" file accompanying this file. This file is distributed
+// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package common
+
+// IsModifyingStatus returns true if a
+// resource's latest status matches one
+// of the provided modifying statuses.
+func IsModifyingStatus(
+	latestStatus *string,
+	modifyingStatuses *[]string,
+) bool {
+	for _, status := range *modifyingStatuses {
+		if *latestStatus == status {
+			return true
+		}
+	}
+	return false
+}

pkg/resource/feature_group/hooks.go

@@ -0,0 +1,45 @@
+// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"). You may
+// not use this file except in compliance with the License. A copy of the
+// License is located at
+//
+//     http://aws.amazon.com/apache2.0/
+//
+// or in the "license" file accompanying this file. This file is distributed
+// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package feature_group
+
+import (
+	"context"
+	"errors"
+	ackrequeue "github.com/aws-controllers-k8s/runtime/pkg/requeue"
+	svccommon "github.com/aws-controllers-k8s/sagemaker-controller/pkg/common"
+	svcsdk "github.com/aws/aws-sdk-go/service/sagemaker"
+)
+
+var (
+	modifyingStatuses = []string{svcsdk.FeatureGroupStatusCreating,
+		svcsdk.FeatureGroupStatusDeleting}
+
+	resourceName = resourceGK.Kind
+
+	requeueWaitWhileDeleting = ackrequeue.NeededAfter(
+		errors.New(resourceName+" is deleting."),
+		ackrequeue.DefaultRequeueAfterDuration,
+	)
+)
+
+// requeueUntilCanModify creates and returns an
+// ackrequeue error if a resource's latest status matches
+// any of the defined modifying statuses below.
+func (rm *resourceManager) requeueUntilCanModify(
+	ctx context.Context,
+	r *resource,
+) error {
+	latestStatus := r.ko.Status.FeatureGroupStatus
+	return svccommon.RequeueIfModifying(latestStatus, &resourceName, &modifyingStatuses)
+}

pkg/resource/feature_group/sdk.go

@@ -19,13 +19,20 @@ import (
 
 	ackcondition "github.com/aws-controllers-k8s/runtime/pkg/condition"
 	ackrequeue "github.com/aws-controllers-k8s/runtime/pkg/requeue"
+	svccommon "github.com/aws-controllers-k8s/sagemaker-controller/pkg/common"
 	svcsdk "github.com/aws/aws-sdk-go/service/sagemaker"
 	corev1 "k8s.io/api/core/v1"
 )
 
 var (
+	modifyingStatuses = []string{svcsdk.ModelPackageGroupStatusInProgress,
+		svcsdk.ModelPackageGroupStatusPending,
+		svcsdk.ModelPackageGroupStatusDeleting}
+
+	resourceName = resourceGK.Kind
+
 	requeueWaitWhileDeleting = ackrequeue.NeededAfter(
-		errors.New("ModelPackageGroup is deleting"),
+		errors.New(resourceName+" is deleting."),
 		ackrequeue.DefaultRequeueAfterDuration,
 	)
 )
@@ -36,39 +43,20 @@ func (rm *resourceManager) customSetOutput(r *resource) {
 	}
 	ModelPackageGroupStatus := *r.ko.Status.ModelPackageGroupStatus
 	msg := "ModelPackageGroup is in" + ModelPackageGroupStatus + "status"
-	if !isModifiying(r) {
+	if !svccommon.IsModifyingStatus(&ModelPackageGroupStatus, &modifyingStatuses) {
 		ackcondition.SetSynced(r, corev1.ConditionTrue, &msg, nil)
 	} else {
 		ackcondition.SetSynced(r, corev1.ConditionFalse, &msg, nil)
 	}
 }
 
-func (rm *resourceManager) requeueUntilCanModify(ctx context.Context,
-	latest *resource,
+// requeueUntilCanModify creates and returns an
+// ackrequeue error if a resource's latest status matches
+// any of the defined modifying statuses below.
+func (rm *resourceManager) requeueUntilCanModify(
+	ctx context.Context,
+	r *resource,
 ) error {
-	if latest.ko.Status.ModelPackageGroupStatus == nil {
-		return nil
-	}
-	ModelPackageGroupStatus := *latest.ko.Status.ModelPackageGroupStatus
-	if isModifiying(latest) {
-		errMsg := "ModelPackageGroup in" + ModelPackageGroupStatus + "state cannot be modified or deleted"
-		requeueWaitWhileModifying := ackrequeue.NeededAfter(
-			errors.New(errMsg),
-			ackrequeue.DefaultRequeueAfterDuration,
-		)
-		return requeueWaitWhileModifying
-	}
-	return nil
-}
-
-func isModifiying(r *resource) bool {
-	if r == nil || r.ko.Status.ModelPackageGroupStatus == nil {
-		return false
-	}
-	ModelPackageGroupStatus := *r.ko.Status.ModelPackageGroupStatus
-
-	if ModelPackageGroupStatus == string(svcsdk.ModelPackageGroupStatusInProgress) || ModelPackageGroupStatus == string(svcsdk.ModelPackageGroupStatusPending) || ModelPackageGroupStatus == string(svcsdk.ModelPackageGroupStatusDeleting) {
-		return true
-	}
-	return false
+	latestStatus := r.ko.Status.ModelPackageGroupStatus
+	return svccommon.RequeueIfModifying(latestStatus, &resourceName, &modifyingStatuses)
 }

pkg/resource/model_package_group/hooks.go

@@ -1,8 +1,9 @@
-    if err == nil {
-            if _, err := rm.sdkFind(ctx, r); err != ackerr.NotFound {
+
+if err == nil {
+        if _, err := rm.sdkFind(ctx, r); err != ackerr.NotFound {
                 if err != nil {
-                    return err
+                        return err
                 }
                 return requeueWaitWhileDeleting
-            }
-    }
+        }
+}

pkg/resource/model_package_group/sdk.go

@@ -1,3 +1,4 @@
-    if err = rm.requeueUntilCanModify(ctx, r); err != nil {
-            return err
-        }
+
+if err = rm.requeueUntilCanModify(ctx, r); err != nil {
+        return err
+}

templates/common/sdk_delete_post_request.go.tpl

@@ -117,8 +117,5 @@ def test_create_feature_group(self, feature_group):
         # Delete the k8s resource.
         _, deleted = k8s.delete_custom_resource(reference, WAIT_PERIOD_COUNT, WAIT_PERIOD_LENGTH)
         assert deleted
-        # TODO: Implement logic to requeueOnDelete.
-        # TODO: Once the delete requeue PR is merged,
-        # verify that it works for DeleteFailed state.
 
         assert get_sagemaker_feature_group(feature_group_name) is None