New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding BucketAccess Add Functionality, Fix cmd controller and pass co… #15

Merged

k8s-ci-robot merged 1 commit into kubernetes-sigs:master from brahmaroutu:controller_code_ba

Dec 14, 2020

Contributor

brahmaroutu commented Dec 9, 2020 •

edited

Loading

…ntext to command

Fixes #10
Fixes #8

/assign @wlan0
/ping @rrati @jeffvance

k8s-ci-robot assigned wlan0

k8s-ci-robot requested review from rrati and saad-ali

December 9, 2020 03:37

k8s-ci-robot added approved cncf-cla: yes size/L labels

brahmaroutu added this to the Demo 1 milestone

jeffvance reviewed

View reviewed changes

pkg/util/util.go Outdated

@@ @@ -65,6 +71,25 @@ func GetUUID() string { @@
               	return string(uuid.NewUUID())
               }
+              func ReadConfigData(kubeClient kubeclientset.Interface, configMapRef *v1.ObjectReference) string {

jeffvance Dec 9, 2020

should there be an err as part of the func return? Otherwise how does caller know the difference between an empty CM and a GET or Marshal error?

Contributor Author

brahmaroutu Dec 9, 2020

Fixed. Thanks for the review.

jeffvance reviewed

View reviewed changes

pkg/bucketaccessrequest/bucketaccessrequest.go Outdated Show resolved Hide resolved

brahmaroutu force-pushed the controller_code_ba branch 2 times, most recently from 978d658 to 8f3f158 Compare

December 9, 2020 05:24

jeffvance reviewed

View reviewed changes

pkg/bucketaccessrequest/bucketaccessrequest.go Outdated

+              		// Provisioning is 100% finished / not in progress.
+              		switch err {
+              		case util.ErrInvalidBucketAccessClass:
+              			glog.V(5).Infof("Bucket access class specified does not exist. Stop provisioning, removing bucket access request %s from bucket access requests in progress", bucketAccessRequest.UID)

jeffvance Dec 9, 2020

is this interpretation of err always correct? could this err also be that a field is missing in the BAC?
"Stop provisioning, removing bucket access request %s from bucket access requests in progress". This msg makes me think that the controller will next "stop provisioning", which is not correct.

What if each msg just interprets the returned err and at the end append the msg about provisioning stopped? Eg.

switch err {
		case util.ErrInvalidBucketAccessClass:
			errmsg = sprintf("Bucket access class error: %v", err)
                case util.ErrBucketAccessAlreadyExists:
                        errmsg = sprintf("Bucket access already exist: %v", err)
                 ...
}
err = nil
errmsg = sprintf("%s\nProvisioning stopped.", errmsg)

Contributor Author

brahmaroutu Dec 9, 2020

There are some errors that are not recoverable. There is no point in keeping the BAR in the queue.

pkg/bucketaccessrequest/bucketaccessrequest.go Outdated

+              			glog.V(5).Infof("Bucket access class specified does not exist. Stop provisioning, removing bucket access request %s from bucket access requests in progress", bucketAccessRequest.UID)
+              			err = nil
+              		case util.ErrBucketAccessAlreadyExists:
+              			glog.V(5).Infof("Bucket access already exist for this bucket request. Stop provisioning, removing bucket access request %s from bucket access requests in progress", bucketAccessRequest.UID)

jeffvance Dec 9, 2020

"removing bucket access request %s from bucket access requests in progress" is not understandable to me.

Contributor Author

brahmaroutu Dec 9, 2020

lack of better wording... I am trying to say
"Bucket access already exist for this bucket request. Stop provisioning, removing bucket access request br1 from bucket access requests in progress"
can also be changed to
"Bucket access already exist for this bucket request br1."

Contributor Author

brahmaroutu Dec 10, 2020

Fixed the wording

jeffvance reviewed

View reviewed changes

pkg/bucketaccessrequest/bucketaccessrequest.go

+              		switch err {
+              		case util.ErrInvalidBucketAccessClass:
+              			glog.V(5).Infof("Bucket access class specified does not exist. Stop provisioning, removing bucket access request %s from bucket access requests in progress", bucketAccessRequest.UID)
+              			err = nil

jeffvance Dec 9, 2020 •

edited

Loading

how is this case not an error? Don't we want to keep the BAR in the work-queue and retry?

Contributor Author

brahmaroutu Dec 9, 2020

It is an error that is not recoverable and we will not retry. We just print the error and ignore this from the queue and stop retrying.

jeffvance reviewed

View reviewed changes

pkg/bucketaccessrequest/bucketaccessrequest.go Outdated

+              	return nil
+              }
+              func (b *bucketAccessRequestListener) FindBucketAccess(ctx context.Context, br *v1alpha1.BucketAccessRequest) *v1alpha1.BucketAccess {

jeffvance Dec 9, 2020

s/br/bar/ in arg list

Contributor Author

brahmaroutu Dec 9, 2020

fixed

jeffvance reviewed

View reviewed changes

pkg/bucketaccessrequest/bucketaccessrequest.go Outdated

+              	if err != nil {
+              		return nil
+              	}
+              	if len(bucketAccessList.Items) > 0 {

jeffvance Dec 9, 2020

nit:

if err != nil || len(bucketAccessList.Items) <=0 {
	return nil
}

Contributor Author

brahmaroutu Dec 9, 2020

Fixed

jeffvance reviewed

View reviewed changes

pkg/bucketaccessrequest/bucketaccessrequest.go

+              			}
+              		}
+              	}
+              	return nil

jeffvance Dec 9, 2020

if do above then this return should be deleted

Contributor Author

brahmaroutu Dec 9, 2020

you still need the return here

jeffvance reviewed

View reviewed changes

pkg/bucketaccessrequest/bucketaccessrequest.go Outdated

+              	}
+              	if len(bucketAccessList.Items) > 0 {
+              		for _, bucketaccess := range bucketAccessList.Items {
+              			if bucketaccess.Spec.BucketAccessRequest == br.Name {

jeffvance Dec 9, 2020 •

edited

Loading

according to the KEP, BA.Spec.BucketAccessRequest is an ObjectReference containing name, namespace and UID. Did that change?

Comparing names is insufficient since the same BAR name can appear in different namespaces.

Contributor Author

brahmaroutu Dec 10, 2020

Fixed

jeffvance reviewed

View reviewed changes

pkg/bucketaccessrequest/bucketaccessrequest.go Outdated

+              	bucketaccess := b.FindBucketAccess(ctx, bucketAccessRequest)
+              	if bucketaccess != nil {
+              		// bucketaccess has been already provisioned, nothing to do.

jeffvance Dec 9, 2020

Is there really nothing left to do? What about setting BA.status? What about verifying various fields (like bindings) to make sure things look coherent?

Contributor Author

brahmaroutu Dec 9, 2020

The BA found based on a match criteria. I am not sure what other fields you would like to verify. Can you be more specific.

jeffvance reviewed

View reviewed changes

pkg/bucketaccessrequest/bucketaccessrequest.go Outdated

+              	bucketAccessClass, err := b.bucketClient.ObjectstorageV1alpha1().BucketAccessClasses().Get(ctx, bucketAccessClassName, metav1.GetOptions{})
+              	if bucketAccessClass == nil {
+              		// bucket has been already provisioned, nothing to do.

jeffvance Dec 9, 2020 •

edited

Loading

If BAC is nil then the BAR did not specify the BAC or the BAC has been deleted. Why is the conclusion that the bucket has already been provisioned? Note: returning this err is treated like nil, implying success.

Contributor Author

brahmaroutu Dec 9, 2020

Fixed the comment.

jeffvance reviewed

View reviewed changes

pkg/bucketaccessrequest/bucketaccessrequest.go

+              		return err
+              	}
+              	if bucketRequest.Spec.BucketInstanceName == "" {

jeffvance Dec 9, 2020 •

edited

Loading

what code updates the BR.bucketInstanceName? It should not be the sidecar. Assuming the central controller does this, then that implies that BR binding has to occur before BA binding. Is this correct?

Contributor Author

brahmaroutu Dec 9, 2020 •

edited

Loading

That is correct.

jeffvance reviewed

View reviewed changes

pkg/bucketaccessrequest/bucketaccessrequest.go Outdated

+              	bucketaccess.Spec.Parameters = util.CopySS(bucketAccessClass.Parameters)
+              	bucketaccess, err = b.bucketClient.ObjectstorageV1alpha1().BucketAccesses().Create(context.Background(), bucketaccess, metav1.CreateOptions{})
+              	if err == nil || apierrs.IsAlreadyExists(err) {

jeffvance Dec 9, 2020

If the err is "already exists" then there is probably more work to do for idempotency. We need to do some checks to make sure the existing BA is what we expect.

Contributor Author

brahmaroutu Dec 9, 2020

Fixed.

brahmaroutu force-pushed the controller_code_ba branch from 8f3f158 to e27b7f3 Compare

December 9, 2020 06:59


          Adding BucketAccess Add Functionality, Fix cmd controller and pass co…

84ffaba

…ntext to command

Fixes kubernetes-sigs#10
Fixes kubernetes-sigs#8

brahmaroutu force-pushed the controller_code_ba branch from e27b7f3 to 84ffaba Compare

December 10, 2020 20:20

k8s-ci-robot added size/XL and removed size/L labels

Contributor

wlan0 commented Dec 14, 2020

LGTM

Contributor

wlan0 commented Dec 14, 2020

/lgtm
/approve

k8s-ci-robot added the lgtm label

wlan0 approved these changes

View reviewed changes

Contributor

k8s-ci-robot commented Dec 14, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brahmaroutu, 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~~ [brahmaroutu,wlan0]

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

k8s-ci-robot merged commit 09366ec into kubernetes-sigs:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved cncf-cla: yes lgtm size/XL