Skip to content
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.

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

storage: add s3 backend support (without GC and dedupe) #216

Merged
merged 1 commit into from
Nov 15, 2021

Conversation

eusebiu-constantin-petu-dbk
Copy link
Collaborator

"version": "0.1.0-dev",
"storage": {
"rootDirectory": "/zot",
"objectStoreParams": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's review this part carefully.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "objectStorageParams", let's just call it "storageDriver"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@eusebiu-constantin-petu-dbk eusebiu-constantin-petu-dbk force-pushed the s3_docker branch 4 times, most recently from 052bd63 to e32460c Compare August 27, 2021 13:39
@eusebiu-constantin-petu-dbk eusebiu-constantin-petu-dbk changed the title POC S3 backend without GC, Dedupe, Tests POC S3 backend without GC and Dedupe Aug 27, 2021
@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #216 (5096ba5) into main (8e4d828) will decrease coverage by 1.28%.
The diff coverage is 75.21%.

❗ Current head 5096ba5 differs from pull request most recent head 4ecc97c. Consider uploading reports for the commit 4ecc97c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #216      +/-   ##
==========================================
- Coverage   79.25%   77.97%   -1.29%     
==========================================
  Files          43       30      -13     
  Lines        5360     5066     -294     
==========================================
- Hits         4248     3950     -298     
+ Misses        818      804      -14     
- Partials      294      312      +18     
Impacted Files Coverage Δ
pkg/api/config.go 78.57% <ø> (ø)
pkg/storage/s3/storage.go 74.68% <74.68%> (ø)
pkg/api/controller.go 88.67% <86.95%> (-0.69%) ⬇️
pkg/storage/storage_fs.go 76.09% <100.00%> (-0.67%) ⬇️
pkg/extensions/search/cve/cve.go 74.64% <0.00%> (-12.20%) ⬇️
pkg/api/errors.go 98.14% <0.00%> (-1.86%) ⬇️
pkg/api/routes.go 55.67% <0.00%> (-1.36%) ⬇️
pkg/storage/cache.go 57.89% <0.00%> (-1.29%) ⬇️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e4d828...4ecc97c. Read the comment docs.

@rchincha
Copy link
Contributor

@eusebiu-constantin-petu-dbk
Copy link
Collaborator Author

eusebiu-constantin-petu-dbk commented Oct 26, 2021

https://docs.aws.amazon.com/sdk-for-go/api/service/s3/s3iface/

But I'm not using the sdk directly, I'm using "github.com/docker/distribution/registry/storage/driver/factory"

Look here: https://github.com/anuvu/zot/pull/216/files#diff-4edf1087b440a5696d89e156f567d352995d832217650afee1c116c54fc032d1R103

@rchincha rchincha changed the title POC S3 backend without GC and Dedupe storage: add s3 backend support (without GC and dedupe) Oct 26, 2021
"version": "0.1.0-dev",
"storage": {
"rootDirectory": "/zot",
"objectStoreParams": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "objectStorageParams", let's just call it "storageDriver"

},
"subPaths": {
"/devops": {
"rootDirectory": "/zot-devops"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each of these "rootDirectory" could potentially have their own "storageDriver"s

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we mix filesystem and s3? In the current implementation we can.

"storage": {
"rootDirectory": "/zot",
"objectStoreParams": {
"name": "s3",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the name of the driver or the KeyID for aws storage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's both, I will add a document specifying all the parameters

storeName := fmt.Sprintf("%v", c.Config.Storage.ObjectStoreParams["name"])

// Init a Storager from connection string.
store, err := factory.Create(storeName, c.Config.Storage.ObjectStoreParams)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we create a factory.Create("s3mock"...)? so we control its behaviour?
And specify driver=s3mock in config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don;t understand the question.

You can do that already if you want with regionendpoint parameter.

                "storageDriver": {
                    "name": "s3",
                    "region": "us-east-2",
                    "regionendpoint": "localhost:4566",
                    "bucket": "zot-storage",
                    "secure": false,
                    "skipverify": false
                }
                

localhost:4566 is https://github.com/localstack/localstack

@eusebiu-constantin-petu-dbk eusebiu-constantin-petu-dbk force-pushed the s3_docker branch 4 times, most recently from 8d1d632 to 140c309 Compare November 9, 2021 12:56
pkg/api/controller.go Show resolved Hide resolved
pkg/api/controller.go Show resolved Hide resolved
@eusebiu-constantin-petu-dbk eusebiu-constantin-petu-dbk force-pushed the s3_docker branch 3 times, most recently from f563ac1 to 6053051 Compare November 10, 2021 14:13
pkg/api/controller.go Show resolved Hide resolved
pkg/api/controller.go Outdated Show resolved Hide resolved
@eusebiu-constantin-petu-dbk eusebiu-constantin-petu-dbk force-pushed the s3_docker branch 2 times, most recently from e5c9af7 to 54536c2 Compare November 12, 2021 15:51

storageDriverParams := map[string]interface{}{
"rootDir": "zot",
"name": "s3",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this S3StorageDriverName

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and other places too

conf.HTTP.Port = port
storageDriverParams := map[string]interface{}{
"rootDir": "zot",
"name": "s3",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this to the string constant.

Signed-off-by: Petu Eusebiu <peusebiu@cisco.com>
Copy link
Contributor

@rchincha rchincha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm


storageDriverParams := map[string]interface{}{
"rootDir": "zot",
"name": "s3",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants