Adding bucketrequest controller (WIP) #7

brahmaroutu · 2020-12-04T19:21:58Z

This code adds basic bucket request Add functionality.
A controller, pkg util to support unit and functional testing.

brahmaroutu · 2020-12-04T20:25:50Z

@wlan0 @jeffvance @rrati Please review.
/test pull-container-object-storage-interface-controller-build

brahmaroutu · 2020-12-04T20:32:17Z

/test pull-container-object-storage-interface-controller-build

brahmaroutu · 2020-12-04T20:55:18Z

/test pull-container-object-storage-interface-controller-build

brahmaroutu · 2020-12-04T21:37:42Z

/test pull-container-object-storage-interface-controller-build

jeffvance · 2020-12-05T04:07:32Z

pkg/bucketrequest/bucketrequest.go

+	b.bucketClient = bc
+}
+
+// Add creates a bucket in response to a buckerrequest


jeffvance · 2020-12-05T04:18:31Z

pkg/bucketrequest/bucketrequest.go

+		return "InvalidBucketClass", util.ErrStopProvision
+	}
+
+	glog.Info(logOperation("created", "bucket %q provisioned", bucket))


why is success declared here? The provisioner hasn't even had a chance to see the new bucket instance yet.

Success is that the bucket is created but status is not available until provisioner acts on it.

Good Question. @brahmaroutu

Doesn't Created give the false impression that the bucket is created in the backend?

Success is that the bucket is created but status is not available until provisioner acts on it.

But the log msg states that the bucket was provisioned and this is not the case.

jeffvance · 2020-12-05T04:19:27Z

pkg/bucketrequest/bucketrequest.go

+// Returns nil error only when the bucket was provisioned (in which case it also returns "Finished"),
+// a normal error when the bucket was not provisioned and provisioning should be retried (requeue the bucketRequest),
+// or the special errStopProvision when provisioning was impossible and no further attempts to provision should be tried.
+func (b *bucketRequestListener) provisionBucketRequestOperation(ctx context.Context, bucketRequest *v1alpha1.BucketRequest) (string, error) {


did you consider the func returning (bool, error) like we see for many of the wait funcs? Where the bool is true if the controller should not retry (sort of like done)

brahmaroutu · 2020-12-05T19:13:31Z

unti test fix: kubernetes-retired/container-object-storage-interface-spec#10

brahmaroutu · 2020-12-07T16:49:35Z

/test pull-container-object-storage-interface-controller-build

brahmaroutu · 2020-12-07T16:49:49Z

/test pull-container-object-storage-interface-controller-unit

NicolasT

First batch of comments.

NicolasT · 2020-12-07T17:53:58Z

cmd/controller-manager/controller-manager.go

+	var cancel context.CancelFunc
+
+	ctx, cancel = context.WithCancel(context.Background())
+	sigs := make(chan os.Signal, 1)
+	signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM, syscall.SIGSEGV)
+
+	go func() {
+		s := <-sigs
+		cancel()
+		panic(fmt.Sprintf("%s %s", s.String(), "Signal received. Exiting"))
+	}()


Why do this in init instead of main?

Moved the code.

NicolasT · 2020-12-07T17:55:00Z

cmd/controller-manager/controller-manager.go

+	"github.com/golang/glog"
+)
+
+var ctx context.Context


No need for this global I believe, see below.

NicolasT · 2020-12-07T17:55:35Z

cmd/controller-manager/controller-manager.go

+	SilenceErrors: true,
+	SilenceUsage:  true,
+	RunE: func(c *cobra.Command, args []string) error {
+		return run(args)


You can use Commands Context() and pass it to run instead: https://godoc.org/github.com/spf13/cobra#Command.Context

Fixed. Using cmd context now.

NicolasT · 2020-12-07T17:56:27Z

cmd/controller-manager/controller-manager.go

+}
+
+func main() {
+	if err := cmd.Execute(); err != nil {


If you move the context setup out of init (where IMHO it doesn't belong) into main, you can use Commands ExecuteContext (https://godoc.org/github.com/spf13/cobra#Command.ExecuteContext)

NicolasT · 2020-12-07T17:57:07Z

cmd/controller-manager/controller-manager.go

+func run(args []string) error {
+	ctrl, err := bucketcontroller.NewDefaultObjectStorageController("controller-manager", "leader-lock", 40)
+	if err != nil {
+		glog.Error(err)


Both log and glog seem to be used throughout this file?

Fixed, using only blog

NicolasT · 2020-12-07T17:57:44Z

cmd/controller-manager/controller-manager.go

+func run(args []string) error {
+	ctrl, err := bucketcontroller.NewDefaultObjectStorageController("controller-manager", "leader-lock", 40)
+	if err != nil {
+		glog.Error(err)


Either log the error and handle it, or don't log and return it (then, expect the upper layer to log it if appropriate).

Fixed. Returning error

NicolasT · 2020-12-07T17:59:11Z

cmd/controller-manager/controller-manager.go

+
+	ctx, cancel = context.WithCancel(context.Background())
+	sigs := make(chan os.Signal, 1)
+	signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM, syscall.SIGSEGV)


Do we really need/want to override the default handler for SIGSEGV? 🤔

agree no need to catch SIGSEGV. @wlan WDYT?

NicolasT · 2020-12-07T17:59:43Z

cmd/controller-manager/controller-manager.go

+	go func() {
+		s := <-sigs
+		cancel()
+		panic(fmt.Sprintf("%s %s", s.String(), "Signal received. Exiting"))


Why should this panic?

Removed panic, I think we just fall off the go routine. Should be fine as context is cancelled.

NicolasT · 2020-12-07T18:04:52Z

pkg/bucketrequest/bucketrequest.go

+func (b *bucketRequestListener) InitializeKubeClient(k kubeclientset.Interface) {
+	b.kubeClient = k
+}
+
+func (b *bucketRequestListener) InitializeBucketClient(bc bucketclientset.Interface) {
+	b.bucketClient = bc
+}


It appears, on first sight, a bucketRequestListener can't reasonably be used unless its kubeClient and bucketClient are set up. As such, instead of having these Initialize* methods, how about requiring a kubeclientset.Interface and bucketclientset.Interface to be passed to the 'constructor' (NewListener)? Less API, less possibility of 'doing something wrong'.

Some of the object require the client and some may not need. Controller listener interface is built generically. https://github.com/kubernetes-sigs/container-object-storage-interface-api/blob/43539346a903848b3f17aa0f4d41cd827d0070e1/controller/interfaces.go#L16

Remains an odd API which can easily be mis-used. All of a sudden a consumer needs to know what any method call requires to be set in order to 'sufficiently properly' initialize an instance.

NicolasT · 2020-12-07T18:07:55Z

pkg/bucketrequest/bucketrequest.go

+		// - the bucketRequest is already there if previous status was ProvisioningInBackground.
+		// - the bucketRequest is not there if if previous status was "Finished".
+	}
+	return nil


What if err != nil and status != "Finished"?

Reworked this code to return and process just error.

NicolasT · 2020-12-07T18:08:07Z

pkg/bucketrequest/bucketrequest.go

+	glog.V(1).Infof("add called for bucket %s", obj.Name)
+	bucketRequest := obj
+	status, err := b.provisionBucketRequestOperation(ctx, bucketRequest)
+	if err == nil || status == "Finished" {


Finished should likely be a const somewhere.

Code reworked as explained above.

NicolasT · 2020-12-07T18:09:58Z

pkg/bucketrequest/bucketrequest.go

+		Name:      bucketRequest.Name,
+		Namespace: bucketRequest.Namespace,
+		UID:       bucketRequest.ObjectMeta.UID}
+	bucket.Spec.AllowedNamespaces = util.CopyStrings(bucketClass.AllowedNamespaces) //could use k8s util/slice


Instead of these could ... comments, either do so (in final version of the code), or remove the comment 😃

Removed the comments.

NicolasT · 2020-12-07T18:11:05Z

pkg/bucketrequest/bucketrequest.go

+	}
+	if err == nil || apierrs.IsAlreadyExists(err) {
+		glog.V(5).Infof("Bucket %s saved", bucket.Name)
+		return "exists", nil


All these should likely be consts, or even a custom type and an enumeration of all possible values (also consts). Not plain string.

Reworked the code.

NicolasT · 2020-12-07T18:18:57Z

pkg/util/util.go

+	return client, kubeClient, ctrl
+}
+
+func GetBuckets(ctx context.Context, client bucketclientset.Interface, numExpected int) *types.BucketList {


This is likely only used in tests? Could use a comment at least to explain what the function does, how it behaves.

NicolasT · 2020-12-07T18:19:17Z

pkg/util/util.go

+	return false
+}
+
+func GetBucketAccesses(ctx context.Context, client bucketclientset.Interface, numExpected int) *types.BucketAccessList {


Same as above.

NicolasT · 2020-12-07T18:20:48Z

pkg/util/util.go

+	return false
+}
+
+func DeleteObjects(ctx context.Context, client bucketclientset.Interface, objs ...interface{}) {


I'd keep some more specific defers in the test code rather than a catch-all like this. Seems a bit error-prone.

I believe this way code is less verbose in the test and test is more readable. I agree user need to remember all objects to delete whether he choses to call this multiple time with multiple defer calls or at once.

It's 'magic'. Go code should be explicit and simple, no 'magic'. But fair enough, matter of taste so no real objection.

NicolasT · 2020-12-07T18:22:11Z

pkg/util/util.go

+}
+
+func getCRDClient() (apiextensions.CustomResourceDefinitionInterface, error) {
+	config, err := func() (*rest.Config, error) {


See before.

Documented that this is used by unit testing only.

NicolasT · 2020-12-07T18:23:51Z

pkg/util/util.go

+	return crdClientset.CustomResourceDefinitions(), err
+}
+
+func RegisterCRDs(ctx context.Context, client apiextensions.CustomResourceDefinitionInterface) error {


I'd argue these should be set up/managed in a test environment/cluster by the test automation, not by the test(s) themselves.

I agree, will be reworked once we have a proper setup mechanism.

wlan0 · 2020-12-08T17:03:16Z

I have minor nits only. We can take this in and address minor nits in subsequent PRs

wlan0 · 2020-12-08T17:03:22Z

/lgtm

k8s-ci-robot · 2020-12-08T17:03:55Z

[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

NicolasT · 2020-12-08T17:08:47Z

cmd/controller-manager/controller-manager.go

+	var cancel context.CancelFunc
+
+	_, cancel = context.WithCancel(cmd.Context())
+	sigs := make(chan os.Signal, 1)
+	signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
+
+	go func() {
+		<-sigs
+		cancel()
+	}()


All of this comes after we ran cmd.Execute, so it seems unused whatsoever?

if you are saying that this code should go before cmd.Execute then we will have to create the context like we did before.

We need to create it, but it shouldn't be a global, can remain within main 😃 Something like the following (from memory):

func main() { ctx, cancel := context.WithCancel(context.Background()) defer cancel() // Just in case sigs := make(chan os.Signal, 1) signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM) go func() { <-sigs cancel() }() if err := cmd.ExecuteContext(ctx); err != nil { glog.Fatal(err.Error()) } }

NicolasT · 2020-12-08T17:09:21Z

cmd/controller-manager/controller-manager.go

+
+	var cancel context.CancelFunc
+
+	_, cancel = context.WithCancel(cmd.Context())


cmd would not 'have' a Context here since we never set it. beforehand.

By default cmd gets context.Background() right?

Yes, though that's a bit of a 'fallback' in cobra. I think being explicit here (also see snippet above) is preferable.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 4, 2020

k8s-ci-robot requested review from jsafrane and saad-ali December 4, 2020 19:22

k8s-ci-robot added 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 Dec 4, 2020

brahmaroutu force-pushed the controller_code branch 3 times, most recently from c5ed674 to e6dd28a Compare December 4, 2020 19:39

brahmaroutu force-pushed the controller_code branch 3 times, most recently from 4f8836e to 730de26 Compare December 4, 2020 22:57

brahmaroutu changed the title ~~Adding bucketrequest controller~~ Adding bucketrequest controller (WIP) Dec 4, 2020

brahmaroutu force-pushed the controller_code branch 3 times, most recently from c6079f5 to b9cf8f7 Compare December 5, 2020 03:31

jeffvance reviewed Dec 5, 2020

View reviewed changes

brahmaroutu force-pushed the controller_code branch from b9cf8f7 to d10e10c Compare December 5, 2020 05:40

brahmaroutu force-pushed the controller_code branch from d10e10c to 9bf6e28 Compare December 7, 2020 16:57

NicolasT reviewed Dec 7, 2020

View reviewed changes

brahmaroutu force-pushed the controller_code branch from 9bf6e28 to af06dd8 Compare December 8, 2020 06:21

Adding bucketrequest controller

0078523

brahmaroutu force-pushed the controller_code branch from af06dd8 to 0078523 Compare December 8, 2020 06:37

k8s-ci-robot assigned wlan0 Dec 8, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2020

wlan0 approved these changes Dec 8, 2020

View reviewed changes

k8s-ci-robot merged commit c330fbd into kubernetes-retired:master Dec 8, 2020

NicolasT reviewed Dec 8, 2020

View reviewed changes

wlan0 linked an issue Dec 8, 2020 that may be closed by this pull request

BucketRequest Add #9

Closed


		var cancel context.CancelFunc

		_, cancel = context.WithCancel(cmd.Context())

Adding bucketrequest controller (WIP) #7

Adding bucketrequest controller (WIP) #7

Conversation

brahmaroutu commented Dec 4, 2020

brahmaroutu commented Dec 4, 2020

brahmaroutu commented Dec 4, 2020

brahmaroutu commented Dec 4, 2020

brahmaroutu commented Dec 4, 2020

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 Dec 5, 2020 • edited Loading

Choose a reason for hiding this comment

brahmaroutu commented Dec 5, 2020

brahmaroutu commented Dec 7, 2020

brahmaroutu commented Dec 7, 2020

NicolasT 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

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

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

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 commented Dec 8, 2020

wlan0 commented Dec 8, 2020

k8s-ci-robot commented Dec 8, 2020

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 Dec 5, 2020 •

edited

Loading