Major refactor and bugfixes #49

wlan0 · 2021-03-26T21:38:03Z

Requires changes to API from this PR use bucketName instead of bucketName and bucketInstanceName container-object-storage-interface-api#35
Requires changes to SPEC from this PR remove opaque parameters from delete request and return bucket id o… container-object-storage-interface-spec#25

@krishchow @brahmaroutu @jeffvance @YiannisGkoufas

wlan0 · 2021-03-26T21:38:10Z

/hold

k8s-ci-robot · 2021-03-26T21:38:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wlan0

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [wlan0]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

YiannisGkoufas · 2021-04-01T11:16:45Z

resources/deployment.yaml

-          - "--listen-address=$(LISTEN_ADDRESS)"
-          - "--s3-endpoint=$(S3_ENDPOINT)"
+          - "--driver-addr=$(DRIVER_ADDRESS)"
+          - "--endpoint=$(ENDPOINT)"


The variables endpoint, access-key, secret-key are not used in the driver and the pod fails to start since it doesn't expect that flag

yeah driver update is still WIP

YiannisGkoufas · 2021-04-01T11:18:00Z

resources/deployment.yaml

          valueFrom:
            secretKeyRef:
              name: objectstorage-provisioner
-              key: LISTEN_ADDRESS
+              key: DRIVER_ADDRESS


Fails to create the deployment because the correct format for this is:

env: - name: DRIVER_ADDRESS valueFrom: secretKeyRef: name: objectstorage-provisioner key: DRIVER_ADDRESS

driver changes are WIP

YiannisGkoufas · 2021-04-01T11:18:17Z

resources/deployment.yaml

-        env:
-        - name: CONNECT_ADDRESS
+          - "--driver-addr=$(DRIVER_ADDRESS)"
+        envFrom:


fix to env:

driver changes are WIP

YiannisGkoufas · 2021-04-01T11:24:46Z

pkg/sampledriver/provisioner.go

+
+func (s *ProvisionerServer) ProvisionerCreateBucket(ctx context.Context,
+	req *cosi.ProvisionerCreateBucketRequest) (*cosi.ProvisionerCreateBucketResponse, error) {
+


Is the intention this to be empty? Since the current sample minio provisioner has the functionality of creating buckets, keys etc, maybe as part of this PR we can keep the same functionality
I do like the idea though of having a "bare" sample provisioner so that providers can use as base

Yeah, there are other changes coming into this part. Its a WIP still

YiannisGkoufas · 2021-04-01T14:23:10Z

pkg/provisioner/provisioner.go

+	}, nil
+}
+
+func NewDefaultCOSIProvisionerServer(address string,


Maybe we need to pass a dict of values here instead? Because in the case of COSI we need the endpoint, hmac keys etc and other providers might need other options as well

This is the server - this is the core framework. We shouldn't send the driver specific config (like keys) here. This is purely for starting the grpc serer listener.

If you want to use driver specific input, pass it in via CLI flags or other mechanisms (like env/metadata server/etc..) and wire it into Provisioner/Identity calls directly. (See how I've passed in provisioner name into sampledriver/identityServer)

jeffvance · 2021-04-01T18:54:57Z

cmd/minio-cosi-driver/main.go

+		sig := <-sigs
+		klog.InfoS("Signal received", "type", sig)
+		cancel()
+		<-time.After(30 * time.Second)


why is this long of a pause needed?

This is not a long pause. This is needed in case graceful shutdown doesn;t complete in 30 seconds.

the main thread is expected to exit in these 30 seconds

jeffvance · 2021-04-01T18:59:27Z

cmd/objectstorage-sidecar/main.go

+		sig := <-sigs
+		klog.InfoS("Signal received", "type", sig)
+		cancel()
+		<-time.After(30 * time.Second)


ditto on 30s wait

same answer as above

jeffvance · 2021-04-01T19:28:24Z

pkg/bucket/bucket_controller.go

+		"bucketclass", bucket.Spec.BucketClassName,
+	)
+
+	if !strings.EqualFold(bucket.Spec.Provisioner, b.provisionerName) {


maybe this check could be moved before the klog call above?

It cannot be done because provisionerName can only be transitively obtained by getting the corresponding bucket.

jeffvance · 2021-04-01T19:29:48Z

pkg/bucket/bucket_controller.go

+		return nil
+	}
+
+	if bucket.Status.BucketAvailable {


what about also checking for bucket.deletionTimestamp != nil ?

I haven't gone through all the usecases yet. It's better to do a full sweep across both controllers for such state transitions.

jeffvance · 2021-04-01T23:25:25Z

pkg/bucket/bucket_controller.go

+
+	"github.com/pkg/errors"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"


Nit: no pkg alias may be typical, but it could be less confusing to alias this pkg to something like "rpcstatus". Thinking code below if status.Code ...

yeah I agree. :)

jeffvance · 2021-04-01T23:43:25Z

pkg/bucket/bucket_controller.go

+		}
+	}
+
+	bucket.Status.BucketAvailable = false


Is this the trigger to the central controller to complete the delete operation?

jeffvance · 2021-04-01T23:45:03Z

pkg/bucket/bucket_controller.go

+		"bucketclass", bucket.Spec.BucketClassName,
+	)
+
+	if !strings.EqualFold(bucket.Spec.Provisioner, b.provisionerName) {


Is there a common place in the code path where this test can be done before any CRUD methods are called?

Its not that simple - underlying framework code cannot do that because in case of things like BAs, we have to trasitively find the bucket before inferring the provisioner

jeffvance · 2021-04-02T01:50:44Z

pkg/bucketaccess/bucketaccess_controller.go

+		return errors.Wrap(err, "Failed to fetch bucket")
+	}
+
+	if !strings.EqualFold(bucket.Spec.Provisioner, bal.provisionerName) {


can this check be moved to the beginning of the func?

It cannot. Because provisioner is inferred from the bucket

jeffvance · 2021-04-02T01:51:53Z

pkg/bucketaccess/bucketaccess_controller.go

+		"bucket", bucketName,
+	)
+
+	if bucketAccess.Spec.MintedSecretName != "" {


Should BA.deletionTimestamp be checked to make sure there's not a pending delete on the BA?

Same as above - I want to handle all cases that are not basic straightforward ones as a second sweep over the entire codebase.

jeffvance · 2021-04-02T01:52:56Z

pkg/bucketaccess/bucketaccess_controller.go

+	}, nil
+}
+
+// Add attempts to create a bucket for a given bucket. This function must be idempotent


Fix comment: this func grants access to an existing bucket

jeffvance · 2021-04-02T02:59:04Z

pkg/bucketaccess/bucketaccess_controller.go

+	}
+	ns := bal.namespace
+	mintedSecretName := "ba-" + string(bucketAccess.UID)
+	if _, err := bal.Secrets(ns).Get(ctx, mintedSecretName, metav1.GetOptions{}); err != nil {


Why do Get then Create? I think it would be cleaner to call only Create and test for already-exists.

That would work too

jeffvance · 2021-04-02T02:59:38Z

pkg/bucketaccess/bucketaccess_controller.go

+	if _, err := bal.Secrets(ns).Get(ctx, mintedSecretName, metav1.GetOptions{}); err != nil {
+		if !kubeerrors.IsNotFound(err) {
+			klog.ErrorS(err,
+				"Failed to create secrets",


~~Err msg could be improved by mentioning the secret already exists.~~

The condition is true if the error is anything but "Secret is not found". i.e. we are ignoring AlreadyExists.

jeffvance · 2021-04-02T03:00:39Z

pkg/bucketaccess/bucketaccess_controller.go

+				Namespace: ns,
+			},
+			StringData: map[string]string{
+				"CredentialsFilePath":     credentialsFilePath,


Would there be a benefit to these key names defined as constants somewhere?

Yes, I have thought about it - but left it as is for now. It doesn't make a big difference right now

haslersn · 2021-04-05T18:33:03Z

pkg/bucket/bucket_controller.go

+// Return values
+//    nil - Bucket successfully provisioned
+//    non-nil err - Internal error                                [requeue'd with exponential backoff]
+func (b *BucketListener) Add(ctx context.Context, bucket *v1alpha1.Bucket) error {


You should create a copy of bucket, since you modify below.

yeah already done

haslersn · 2021-04-06T03:32:10Z

pkg/sampledriver/provisioner.go

+	provisioner string
+}
+
+func (s *ProvisionerServer) ProvisionerCreateBucket(ctx context.Context,


When the provisioner creates the backend bucket but for some reason (provisioner crash, sidecar crash, etc.) the ProvisionerCreateBucketResponse is not handled, then the backend bucket is leaked. In order to solve this, I propose a two-step creation process:

ProvisionerCreateBucketID (gets called if bucket.Spec.BucketID == ""): The provisioner chooses a bucket ID and returns it in the response, but it doesn't create the backend bucket, yet. The sidecar persists the bucket ID to the bucket.Spec.BucketID.

ProvisionerCreateBucket (gets called if bucket.Spec.BucketID != "" && !bucket.Status.BucketAvailable): The provisioner now creates the backend bucket. The sidecar persists bucket.Status.BucketAvailable = true.

This way ProvisionerCreateBucket is retried with the same BucketID in case of abovementioned exceptional situation.

When the provisioner creates the backend bucket but for some reason (provisioner crash, sidecar crash, etc.) the ProvisionerCreateBucketResponse is not handled, then the backend bucket is leaked. In order to solve this, I propose a two-step creation process

This is not true. ProvisionerCreateBucket is idempotent. Bucket name + protocol is enough to ensure that.

That is only the case if the bucket ID is deterministic. Are you saying that, for all provisioners, the bucket ID must be deterministically determined by the bucket name and protocol?

- Requires changes to API from this PR github.com/kubernetes-retired/container-object-storage-interface-api/pull/35 - Requires changes to API from this PR github.com/kubernetes-retired/container-object-storage-interface-spec/pull/25

krishchow

one minor comment, but otherwise it LGTM

krishchow · 2021-04-06T21:01:37Z

cmd/objectstorage-sidecar/cmd.go

+		provisionerName = DefaultProvisionerName
+	}
+
+	ctrl, err := controller.NewDefaultObjectStorageController("cosi", provisionerName, 40)


Should we need to move identity and threads into command line flags?

Identity is inferred from ProvisionerGetInfo, and thread count is not something that NEEDS to be configurable yet - it'll cause more confusion than any potential benefits

sounds good!

krishchow · 2021-04-06T22:04:03Z

/lgtm

wlan0 self-assigned this Mar 26, 2021

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 26, 2021

k8s-ci-robot requested review from brahmaroutu and msau42 March 26, 2021 21:38

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 26, 2021

wlan0 force-pushed the master branch 7 times, most recently from 1e86a65 to 8fe5dd5 Compare March 31, 2021 06:13

YiannisGkoufas reviewed Apr 1, 2021

View reviewed changes

jeffvance reviewed Apr 1, 2021

View reviewed changes

jeffvance reviewed Apr 2, 2021

View reviewed changes

haslersn reviewed Apr 5, 2021

View reviewed changes

wlan0 force-pushed the master branch 2 times, most recently from 166514d to 1b92925 Compare April 5, 2021 22:14

haslersn reviewed Apr 6, 2021

View reviewed changes

wlan0 force-pushed the master branch from 1b92925 to 41b2b0a Compare April 6, 2021 20:45

Major refactor and bugfixes

067e55f

- Requires changes to API from this PR github.com/kubernetes-retired/container-object-storage-interface-api/pull/35 - Requires changes to API from this PR github.com/kubernetes-retired/container-object-storage-interface-spec/pull/25

wlan0 force-pushed the master branch from 41b2b0a to 067e55f Compare April 6, 2021 20:50

wlan0 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2021

krishchow reviewed Apr 6, 2021

View reviewed changes

k8s-ci-robot assigned krishchow Apr 6, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2021

k8s-ci-robot merged commit 42975ae into kubernetes-retired:master Apr 6, 2021

asundriya mentioned this pull request Oct 25, 2024

[sidecar] Failure Events are still present after status of Bucket Ready: true during Bucket creation - Title #156

Open


		func (s *ProvisionerServer) ProvisionerCreateBucket(ctx context.Context,
		req cosi.ProvisionerCreateBucketRequest) (cosi.ProvisionerCreateBucketResponse, error) {

Major refactor and bugfixes #49

Major refactor and bugfixes #49

Conversation

wlan0 commented Mar 26, 2021

wlan0 commented Mar 26, 2021

k8s-ci-robot commented Mar 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlan0 Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffvance Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haslersn Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishchow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishchow commented Apr 6, 2021

wlan0 Apr 2, 2021 •

edited

Loading

jeffvance Apr 1, 2021 •

edited

Loading

jeffvance Apr 1, 2021 •

edited

Loading

jeffvance Apr 2, 2021 •

edited

Loading

jeffvance Apr 2, 2021 •

edited

Loading

jeffvance Apr 2, 2021 •

edited

Loading

haslersn Apr 6, 2021 •

edited

Loading