kube-object-storage / lib-bucket-provisioner

Library for the dynamic provisioning of object store buckets to be used by object store providers.
Apache License 2.0
20 stars 22 forks source link

Do not block OBC request handling on OBC deletion #189

Closed thotz closed 4 years ago

thotz commented 4 years ago

Please refer following rook issue

thotz commented 4 years ago

@jeffvance @copejon I know this library is planning to depreciated. Do I need on cosi and work on it later instead of fixing it now

copejon commented 4 years ago

COSI is still a long way off in terms of its first alpha release. For now, I would suggest running concurrent threads of workers. This change would mean that the Start() function in the library could be altered to be optionally run in >1 go routines as it's done in the sample controller.

So the line in the library

go wait.Until(c.runWorker, time.Second, stopCh)

would be wrapped with a loop and a parameter added to set threadiness, as it is in the sample controller:

func Run(threadiness int, stopCh <-chan struct{}) {
...
    for i := 0; i < threadiness; i++ {
            go wait.Until(c.runWorker, time.Second, stopCh)
    }
...
}

The default should be 1. When rook starts the library controller, it can pass in a higher number of threads.

thotz commented 4 years ago

COSI is still a long way off in terms of its first alpha release. For now, I would suggest running concurrent threads of workers. This change would mean that the Start() function in the library could be altered to be optionally run in >1 go routines as it's done in the sample controller.

So the line in the library

go wait.Until(c.runWorker, time.Second, stopCh)

would be wrapped with a loop and a parameter added to set threadiness, as it is in the sample controller:

func Run(threadiness int, stopCh <-chan struct{}) {
...
    for i := 0; i < threadiness; i++ {
              go wait.Until(c.runWorker, time.Second, stopCh)
    }
...
}

The default should be 1. When rook starts the library controller, it can pass in a higher number of threads.

Sure will work on this

thotz commented 4 years ago

@copejon , I was made suggested above and rook was not picking on local package. I followed instructions in libary contributors. And if I modify Run(), then change will break other provisioners right?

copejon commented 4 years ago

I was made suggested above and rook was not picking on local package.

It looks like Rook has switched over to go mods. To get Go to compile a mod from an alternate location, you need to add a replace directive to the go.mod file. You can do this from the CLI:

go mod edit -replace github.com/import/path=/absolute/path/to/repo-root/

This will alter the go.mod file in rook, so be sure to remove the directive before the PR is merged. Not sure if this is also possible through the CLI

And if I modify Run(), then change will break other provisioners right?

Right... though I'm not sure we've seen much adoption anyway.

Off the top of my head, I see 2 options: 1) Optional env var. E.g. LIB_BUCKET_PROVISIONER_THREADS=n, unset defaults to 1. This doesn't affect the API and allows for project to pull it in and test it without having to rebuild the binary whenever it's switched.

2) Refactor the interface: Add a func RunThreads(thread int) func. Then alter Run() to just call RunThreads(1). This preservers the behavior of Run() and separates out the new behavior.

I think I'm more partial to 1. It's less invasive and doesn't enhance the API.

jeffvance commented 4 years ago

I think I'm more partial to 1. It's less invasive and doesn't enhance the API.

Agree

thotz commented 4 years ago

go mod edit -replace github.com/import/path=/absolute/path/to/repo-root/

Yeah I did the same, it was mentioned in README.md

thotz commented 4 years ago

I think I'm more partial to 1. It's less invasive and doesn't enhance the API. Yeah I agree with u and Jeff

thotz commented 4 years ago

Closing this issue since PR got merged