From a853d9bbda3e8b856898102752b8f64c74713e75 Mon Sep 17 00:00:00 2001 From: Akash Mukhopadhyay Date: Mon, 19 Sep 2022 01:09:15 -0700 Subject: [PATCH 1/3] Adding a few logs and some fixes --- pkg/bucket/bucket_controller.go | 24 +++++++++++++++------ pkg/bucketaccess/bucketaccess_controller.go | 3 ++- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/pkg/bucket/bucket_controller.go b/pkg/bucket/bucket_controller.go index b5040ba..e7476f0 100644 --- a/pkg/bucket/bucket_controller.go +++ b/pkg/bucket/bucket_controller.go @@ -87,10 +87,12 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket) return nil } - if bucket.Spec.ExistingBucketID != "" { - bucket.Status.BucketReady = true - bucket.Status.BucketID = bucket.Spec.ExistingBucketID + bucketReady := false + var bucketID string + if bucket.Spec.ExistingBucketID != "" { + bucketReady = true + bucketID = bucket.Spec.ExistingBucketID } else { req := &cosi.DriverCreateBucketRequest{ Parameters: bucket.Spec.Parameters, @@ -114,8 +116,8 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket) } if rsp.BucketId != "" { - bucket.Status.BucketID = rsp.BucketId - bucket.Status.BucketReady = true + bucketID = rsp.BucketId + bucketReady = true } else { err = errors.New("DriverCreateBucket returned an empty bucketID") return err @@ -126,22 +128,32 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket) ref := bucket.Spec.BucketClaim bucketClaim, err := b.bucketClaims(ref.Namespace).Get(ctx, ref.Name, metav1.GetOptions{}) if err != nil { + klog.ErrorS(err, "Failed to get bucketClaim", + "bucketClaim", ref.Name, + "bucket", bucket.ObjectMeta.Name) return err } bucketClaim.Status.BucketReady = true if _, err = b.bucketClaims(bucketClaim.Namespace).Update(ctx, bucketClaim, metav1.UpdateOptions{}); err != nil { + klog.ErrorS(err, "Failed to update bucketClaim", + "bucketClaim", ref.Name, + "bucket", bucket.ObjectMeta.Name) return err } } } controllerutil.AddFinalizer(bucket, consts.BucketFinalizer) - if _, err = b.buckets().Update(ctx, bucket, metav1.UpdateOptions{}); err != nil { + if bucket, err = b.buckets().Update(ctx, bucket, metav1.UpdateOptions{}); err != nil { klog.ErrorS(err, "Failed to update bucket finalizers", "bucket", bucket.ObjectMeta.Name) return errors.Wrap(err, "Failed to update bucket finalizers") } + // Setting the status here so that the updated object is used + bucket.Status.BucketReady = bucketReady + bucket.Status.BucketID = bucketID + // if this step fails, then controller will retry with backoff if _, err = b.buckets().UpdateStatus(ctx, bucket, metav1.UpdateOptions{}); err != nil { klog.ErrorS(err, "Failed to update bucket status", diff --git a/pkg/bucketaccess/bucketaccess_controller.go b/pkg/bucketaccess/bucketaccess_controller.go index 50e5409..1db40ab 100644 --- a/pkg/bucketaccess/bucketaccess_controller.go +++ b/pkg/bucketaccess/bucketaccess_controller.go @@ -266,7 +266,8 @@ func (bal *BucketAccessListener) Add(ctx context.Context, inputBucketAccess *v1a } if controllerutil.AddFinalizer(bucketAccess, consts.BAFinalizer) { - if _, err = bal.bucketAccesses(bucketAccess.ObjectMeta.Namespace).Update(ctx, bucketAccess, metav1.UpdateOptions{}); err != nil { + bucketAccess, err = bal.bucketAccesses(bucketAccess.ObjectMeta.Namespace).Update(ctx, bucketAccess, metav1.UpdateOptions{}) + if err != nil { klog.ErrorS(err, "Failed to update BucketAccess finalizer", "bucketAccess", bucketAccess.ObjectMeta.Name, "bucket", bucket.ObjectMeta.Name) From 8cb88abdb42cfbf4e895adc5fa22460d3505533f Mon Sep 17 00:00:00 2001 From: Akash Mukhopadhyay Date: Mon, 19 Sep 2022 02:18:06 -0700 Subject: [PATCH 2/3] Fixing the update of bucketclaim to updatestatus --- pkg/bucket/bucket_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/bucket/bucket_controller.go b/pkg/bucket/bucket_controller.go index e7476f0..22e0907 100644 --- a/pkg/bucket/bucket_controller.go +++ b/pkg/bucket/bucket_controller.go @@ -135,7 +135,7 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket) } bucketClaim.Status.BucketReady = true - if _, err = b.bucketClaims(bucketClaim.Namespace).Update(ctx, bucketClaim, metav1.UpdateOptions{}); err != nil { + if _, err = b.bucketClaims(bucketClaim.Namespace).UpdateStatus(ctx, bucketClaim, metav1.UpdateOptions{}); err != nil { klog.ErrorS(err, "Failed to update bucketClaim", "bucketClaim", ref.Name, "bucket", bucket.ObjectMeta.Name) From 9a139ce09ea90d808789186d7d553926303169c7 Mon Sep 17 00:00:00 2001 From: Akash Mukhopadhyay Date: Mon, 19 Sep 2022 11:21:55 -0700 Subject: [PATCH 3/3] Improving logs and fixing a delete scenario --- pkg/bucket/bucket_controller.go | 49 +++++++++++++++++---- pkg/bucketaccess/bucketaccess_controller.go | 41 +++++++++++------ 2 files changed, 67 insertions(+), 23 deletions(-) diff --git a/pkg/bucket/bucket_controller.go b/pkg/bucket/bucket_controller.go index 22e0907..63a40ee 100644 --- a/pkg/bucket/bucket_controller.go +++ b/pkg/bucket/bucket_controller.go @@ -102,7 +102,7 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket) rsp, err := b.provisionerClient.DriverCreateBucket(ctx, req) if err != nil { if status.Code(err) != codes.AlreadyExists { - klog.ErrorS(err, "Failed to create bucket", + klog.V(3).ErrorS(err, "Driver failed to create bucket", "bucket", bucket.ObjectMeta.Name) return errors.Wrap(err, "Failed to create bucket") } @@ -111,7 +111,8 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket) if rsp == nil { err = errors.New("DriverCreateBucket returned a nil response") - klog.ErrorS(err, "Internal Error") + klog.V(3).ErrorS(err, "Internal Error from driver", + "bucket", bucket.ObjectMeta.Name) return err } @@ -119,6 +120,8 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket) bucketID = rsp.BucketId bucketReady = true } else { + klog.V(3).ErrorS(err, "DriverCreateBucket returned an empty bucketID", + "bucket", bucket.ObjectMeta.Name) err = errors.New("DriverCreateBucket returned an empty bucketID") return err } @@ -128,7 +131,7 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket) ref := bucket.Spec.BucketClaim bucketClaim, err := b.bucketClaims(ref.Namespace).Get(ctx, ref.Name, metav1.GetOptions{}) if err != nil { - klog.ErrorS(err, "Failed to get bucketClaim", + klog.V(3).ErrorS(err, "Failed to get bucketClaim", "bucketClaim", ref.Name, "bucket", bucket.ObjectMeta.Name) return err @@ -136,31 +139,40 @@ func (b *BucketListener) Add(ctx context.Context, inputBucket *v1alpha1.Bucket) bucketClaim.Status.BucketReady = true if _, err = b.bucketClaims(bucketClaim.Namespace).UpdateStatus(ctx, bucketClaim, metav1.UpdateOptions{}); err != nil { - klog.ErrorS(err, "Failed to update bucketClaim", + klog.V(3).ErrorS(err, "Failed to update bucketClaim", "bucketClaim", ref.Name, "bucket", bucket.ObjectMeta.Name) return err } + + klog.V(5).Infof("Successfully updated status of bucketClaim: %s, bucket: %s", bucketClaim.ObjectMeta.Name, bucket.ObjectMeta.Name) } } controllerutil.AddFinalizer(bucket, consts.BucketFinalizer) if bucket, err = b.buckets().Update(ctx, bucket, metav1.UpdateOptions{}); err != nil { - klog.ErrorS(err, "Failed to update bucket finalizers", "bucket", bucket.ObjectMeta.Name) + klog.V(3).ErrorS(err, "Failed to update bucket finalizers", "bucket", bucket.ObjectMeta.Name) return errors.Wrap(err, "Failed to update bucket finalizers") } + klog.V(5).Infof("Successfully added finalizer to bucket: %s", bucket.ObjectMeta.Name) + // Setting the status here so that the updated object is used bucket.Status.BucketReady = bucketReady bucket.Status.BucketID = bucketID // if this step fails, then controller will retry with backoff if _, err = b.buckets().UpdateStatus(ctx, bucket, metav1.UpdateOptions{}); err != nil { - klog.ErrorS(err, "Failed to update bucket status", + klog.V(3).ErrorS(err, "Failed to update bucket status", "bucket", bucket.ObjectMeta.Name) return errors.Wrap(err, "Failed to update bucket status") } + klog.V(3).InfoS("Add Bucket success", + "bucket", bucket.ObjectMeta.Name, + "bucketID", bucketID, + "ns", bucket.ObjectMeta.Namespace) + return nil } @@ -186,12 +198,18 @@ func (b *BucketListener) Update(ctx context.Context, old, new *v1alpha1.Bucket) if strings.EqualFold(bucketAccess.Spec.BucketClaimName, bucketClaimName) { err = b.bucketAccesses(bucketClaimNs).Delete(ctx, bucketAccess.Name, metav1.DeleteOptions{}) if err != nil { + klog.V(3).ErrorS(err, "Error deleting BucketAccess", + "name", bucketAccess.Name, + "bucket", bucket.ObjectMeta.Name) return err } } } + klog.V(5).Infof("Successfully deleted dependent bucketAccess of bucket:%s", bucket.ObjectMeta.Name) + controllerutil.RemoveFinalizer(bucket, consts.BABucketFinalizer) + klog.V(5).Infof("Successfully removed finalizer: %s of bucket: %s", consts.BABucketFinalizer, bucket.ObjectMeta.Name) } if controllerutil.ContainsFinalizer(bucket, consts.BucketFinalizer) { @@ -202,6 +220,9 @@ func (b *BucketListener) Update(ctx context.Context, old, new *v1alpha1.Bucket) } } + klog.V(3).InfoS("Update Bucket success", + "name", bucket.ObjectMeta.Name, + "ns", bucket.ObjectMeta.Namespace) return nil } @@ -225,7 +246,7 @@ func (b *BucketListener) InitializeKubeClient(k kube.Interface) { serverVersion, err := k.Discovery().ServerVersion() if err != nil { - klog.ErrorS(err, "Cannot determine server version") + klog.V(3).ErrorS(err, "Cannot determine server version") } else { b.kubeVersion = utilversion.MustParseSemantic(serverVersion.GitVersion) } @@ -238,7 +259,7 @@ func (b *BucketListener) InitializeBucketClient(bc buckets.Interface) { func (b *BucketListener) deleteBucketOp(ctx context.Context, bucket *v1alpha1.Bucket) error { if !strings.EqualFold(bucket.Spec.DriverName, b.driverName) { - klog.V(5).InfoS("Skipping bucket for provisiner", + klog.V(5).InfoS("Skipping bucket for provisioner", "bucket", bucket.ObjectMeta.Name, "driver", bucket.Spec.DriverName, ) @@ -254,26 +275,36 @@ func (b *BucketListener) deleteBucketOp(ctx context.Context, bucket *v1alpha1.Bu if _, err := b.provisionerClient.DriverDeleteBucket(ctx, req); err != nil { if status.Code(err) != codes.NotFound { - klog.ErrorS(err, "Failed to delete bucket", + klog.V(3).ErrorS(err, "Failed to delete bucket", "bucket", bucket.ObjectMeta.Name, ) return err } } + + klog.V(5).Infof("Successfully deleted bucketID: %s from the object storage for bucket: %s", bucket.Status.BucketID, bucket.ObjectMeta.Name) } if bucket.Spec.BucketClaim != nil { ref := bucket.Spec.BucketClaim bucketClaim, err := b.bucketClaims(ref.Namespace).Get(ctx, ref.Name, metav1.GetOptions{}) if err != nil { + klog.V(3).ErrorS(err, "Error fetching bucketClaim", + "bucketClaim", ref.Name, + "bucket", bucket.ObjectMeta.Name) return err } if controllerutil.RemoveFinalizer(bucketClaim, consts.BCFinalizer) { if _, err := b.bucketClaims(bucketClaim.ObjectMeta.Namespace).Update(ctx, bucketClaim, metav1.UpdateOptions{}); err != nil { + klog.V(3).ErrorS(err, "Error removing finalizer from bucketClaim", + "bucketClaim", bucketClaim.ObjectMeta.Name, + "bucket", bucket.ObjectMeta.Name) return err } } + + klog.V(5).Infof("Successfully removed finalizer: %s from bucketClaim: %s for bucket: %s", consts.BCFinalizer, bucketClaim.ObjectMeta.Name, bucket.ObjectMeta.Name) } return nil diff --git a/pkg/bucketaccess/bucketaccess_controller.go b/pkg/bucketaccess/bucketaccess_controller.go index 1db40ab..8dde3c5 100644 --- a/pkg/bucketaccess/bucketaccess_controller.go +++ b/pkg/bucketaccess/bucketaccess_controller.go @@ -107,13 +107,13 @@ func (bal *BucketAccessListener) Add(ctx context.Context, inputBucketAccess *v1a namespace := bucketAccess.ObjectMeta.Namespace bucketClaim, err := bal.bucketClaims(namespace).Get(ctx, bucketClaimName, metav1.GetOptions{}) if err != nil { - klog.ErrorS(err, "Failed to fetch bucketClaim", "bucketClaim", bucketClaimName) + klog.V(3).ErrorS(err, "Failed to fetch bucketClaim", "bucketClaim", bucketClaimName) return errors.Wrap(err, "Failed to fetch bucketClaim") } if bucketClaim.Status.BucketName == "" || bucketClaim.Status.BucketReady != true { err := errors.New("BucketName cannot be empty or BucketNotReady in bucketClaim") - klog.ErrorS(err, + klog.V(3).ErrorS(err, "Invalid arguments", "bucketClaim", bucketClaim.Name, "bucketAccess", bucketAccess.ObjectMeta.Name, @@ -142,7 +142,7 @@ func (bal *BucketAccessListener) Add(ctx context.Context, inputBucketAccess *v1a bucket, err := bal.buckets().Get(ctx, bucketClaim.Status.BucketName, metav1.GetOptions{}) if err != nil { - klog.ErrorS(err, "Failed to fetch bucket", "bucket", bucketClaim.Status.BucketName) + klog.V(3).ErrorS(err, "Failed to fetch bucket", "bucket", bucketClaim.Status.BucketName) return errors.Wrap(err, "Failed to fetch bucket") } @@ -163,7 +163,7 @@ func (bal *BucketAccessListener) Add(ctx context.Context, inputBucketAccess *v1a rsp, err := bal.provisionerClient.DriverGrantBucketAccess(ctx, req) if err != nil { if status.Code(err) != codes.AlreadyExists { - klog.ErrorS(err, + klog.V(3).ErrorS(err, "Failed to grant access", "bucketAccess", bucketAccess.ObjectMeta.Name, "bucketClaim", bucketClaimName, @@ -175,14 +175,14 @@ func (bal *BucketAccessListener) Add(ctx context.Context, inputBucketAccess *v1a if rsp.AccountId == "" { err = errors.New("AccountId not defined in DriverGrantBucketAccess") - klog.ErrorS(err, "BucketAccess", bucketAccess.ObjectMeta.Name) + klog.V(3).ErrorS(err, "BucketAccess", bucketAccess.ObjectMeta.Name) return errors.Wrap(err, fmt.Sprintf("BucketAccess %s", bucketAccess.ObjectMeta.Name)) } credentials := rsp.Credentials if len(credentials) != 1 { err = errors.New("Credentials returned in DriverGrantBucketAccessResponse should be of length 1") - klog.ErrorS(err, "BucketAccess", bucketAccess.ObjectMeta.Name) + klog.V(3).ErrorS(err, "BucketAccess", bucketAccess.ObjectMeta.Name) return errors.Wrap(err, fmt.Sprintf("BucketAccess %s", bucketAccess.ObjectMeta.Name)) } @@ -230,7 +230,7 @@ func (bal *BucketAccessListener) Add(ctx context.Context, inputBucketAccess *v1a if _, err := bal.secrets(namespace).Get(ctx, secretCredName, metav1.GetOptions{}); err != nil { if !kubeerrors.IsNotFound(err) { - klog.ErrorS(err, + klog.V(3).ErrorS(err, "Failed to create secrets", "bucketAccess", bucketAccess.ObjectMeta.Name, "bucket", bucket.ObjectMeta.Name) @@ -249,7 +249,7 @@ func (bal *BucketAccessListener) Add(ctx context.Context, inputBucketAccess *v1a Type: corev1.SecretTypeOpaque, }, metav1.CreateOptions{}); err != nil { if !kubeerrors.IsAlreadyExists(err) { - klog.ErrorS(err, + klog.V(3).ErrorS(err, "Failed to create minted secret", "bucketAccess", bucketAccess.ObjectMeta.Name, "bucket", bucket.ObjectMeta.Name) @@ -268,7 +268,7 @@ func (bal *BucketAccessListener) Add(ctx context.Context, inputBucketAccess *v1a if controllerutil.AddFinalizer(bucketAccess, consts.BAFinalizer) { bucketAccess, err = bal.bucketAccesses(bucketAccess.ObjectMeta.Namespace).Update(ctx, bucketAccess, metav1.UpdateOptions{}) if err != nil { - klog.ErrorS(err, "Failed to update BucketAccess finalizer", + klog.V(3).ErrorS(err, "Failed to update BucketAccess finalizer", "bucketAccess", bucketAccess.ObjectMeta.Name, "bucket", bucket.ObjectMeta.Name) return errors.Wrap(err, fmt.Sprintf("Failed to update BucketAccess finalizer. BucketAccess: %s", bucketAccess.ObjectMeta.Name)) @@ -280,7 +280,7 @@ func (bal *BucketAccessListener) Add(ctx context.Context, inputBucketAccess *v1a // if this step fails, then controller will retry with backoff if _, err := bal.bucketAccesses(bucketAccess.ObjectMeta.Namespace).UpdateStatus(ctx, bucketAccess, metav1.UpdateOptions{}); err != nil { - klog.ErrorS(err, "Failed to update BucketAccess Status", + klog.V(3).ErrorS(err, "Failed to update BucketAccess Status", "bucketAccess", bucketAccess.ObjectMeta.Name, "bucket", bucket.ObjectMeta.Name) return errors.Wrap(err, fmt.Sprintf("Failed to update BucketAccess Status. BucketAccess: %s", bucketAccess.ObjectMeta.Name)) @@ -298,11 +298,15 @@ func (bal *BucketAccessListener) Update(ctx context.Context, old, new *v1alpha1. "name", old.ObjectMeta.Name) bucketAccess := new.DeepCopy() - err := bal.deleteBucketAccessOp(ctx, bucketAccess) - if err != nil { - return err + if !bucketAccess.GetDeletionTimestamp().IsZero() { + err := bal.deleteBucketAccessOp(ctx, bucketAccess) + if err != nil { + return err + } } + klog.V(3).InfoS("Update BucketAccess success", + "name", old.ObjectMeta.Name) return nil } @@ -329,15 +333,24 @@ func (bal *BucketAccessListener) deleteBucketAccessOp(ctx context.Context, bucke if controllerutil.RemoveFinalizer(secret, consts.SecretFinalizer) { _, err = bal.secrets(bucketAccess.ObjectMeta.Namespace).Update(ctx, secret, metav1.UpdateOptions{}) if err != nil { + klog.V(3).ErrorS(err, "Error removing finalizer from secret", + "secret", secret.ObjectMeta.Name, + "bucketAccess", bucketAccess.ObjectMeta.Name) return err } + + klog.V(5).Infof("Successfully removed finalizer from secret: %s, bucketAccess: %s", secret.ObjectMeta.Name, bucketAccess.ObjectMeta.Name) } if controllerutil.RemoveFinalizer(bucketAccess, consts.BAFinalizer) { _, err = bal.bucketAccesses(bucketAccess.ObjectMeta.Namespace).Update(ctx, bucketAccess, metav1.UpdateOptions{}) if err != nil { + klog.V(3).ErrorS(err, "Error removing finalizer from bucketAccess", + "bucketAccess", bucketAccess.ObjectMeta.Name) return err } + + klog.V(5).Infof("Successfully removed finalizer from bucketAccess: %s", bucketAccess.ObjectMeta.Name) } return nil @@ -384,7 +397,7 @@ func (bal *BucketAccessListener) InitializeKubeClient(k kube.Interface) { serverVersion, err := k.Discovery().ServerVersion() if err != nil { - klog.ErrorS(err, "Cannot determine server version") + klog.V(3).ErrorS(err, "Cannot determine server version") } else { bal.kubeVersion = utilversion.MustParseSemantic(serverVersion.GitVersion) }