feat(sync): allow remote storage #2007

elee1766 · 2023-11-03T23:29:51Z

hi, i am trying my hand at this issue. I am not sure if this is a valid solution, but it seems to be working for me. some confirmation from someone maybe more familiar with the logic would be appreciated.

What type of PR is this?
feature

Which issue does this PR fix:

#1297

What does this PR do / Why do we need it:

it uses a configurable temporary local directory to store the local oci blobs always for sync. I also rename "local" to "destination", since that is more semantically accurate now.

so apparently, s3 + sync is currently not supported because we are syncing images in a temporary local oci layout before committing them in the storage using storage APIs.

but i think the true reason that it not supported is because they are syncing images to a temporary local oci in a path based off off the storage drivers' root directory.

I think downloading to the local filesystem always is fine, so I just initialize a new local image store at the tmpdir configured, or os.TempDir() if not. the rest of the logic all remains the same.

since skipImage, err := service.destination.CanSkipImage(destinationRepo, tag, manifestDigest) actually works properly with remote s3 storage

so i think this should work, without the need to implement our own copy.Image()

Testing done on this change:

so far i've just tested this manually with one image, and it seems to be working with simple docker pulls. more needs to be done.

the config i used for testing

# vi: set ft=yaml:
distSpecVersion: 1.0.1
storage:
  rootDirectory: /tmp/zot
  gc: false
  dedupe: false
  storageDriver:
    name: s3
    region: nyc3
    regionendpoint: "nyc3.digitaloceanspaces.com"
    bucket: <bucket>
    accesskey: <access>
    secretkey: <secret>
http:
  address: 0.0.0.0
  port: "8080"
log:
  level: trace
extensions:
  sync:
    enable: true
    tmpDir: /tmp/zotsync
    registries:
      - urls:
          - https://docker.io/library
        content:
          - prefix: '**'
            destination: /docker
        onDemand: true
        tlsVerify: true

i probably need to write some tests?

Automation added to e2e:

not any yet

Will this break upgrades or downgrades?

it shouldn't

Does this PR introduce any user-facing change?:

yes, to be written

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

rchincha · 2023-11-04T18:26:58Z

@elee1766 can you also outline how one would setup a test harness for this?
Pls take a look at: https://github.com/project-zot/zot/tree/main/test

elee1766 · 2023-11-04T22:28:05Z

hmm, i will play around with it. @rchincha are there any rules/guidelines around your guys testing process i should be aware of ?

i think a proper test harness should importantly

make sure that the temp download is properly removed after copied to remote
make sure that the image once pulled, exists in the remote repository
make sure that after the image exists in the remote repository, the image, if it matches, does not repull.
make sure that the copied image is in the correct path, has the correct manifest, signature information, etc
see if dedup with remote storage works - if not, can just disable it until its figured out how to make it work

I think the easiest would be to use three instances of zot, one setup to be the destination, one setup to be the origin, and one setup to be the syncer. then everything can be tested from there? I saw you guys use localstack for s3, so i can use that for the storage.

rchincha · 2023-11-05T04:24:10Z

@elee1766 we have a similar harness already. Pls take a look at that also.
https://github.com/project-zot/zot/blob/main/.github/workflows/cluster.yaml#L129

eusebiu-constantin-petu-dbk · 2023-11-06T12:13:09Z

code looks good to me, add the bats tests and it should be enough.

Also sign your commit with gpg: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits
and also use git commit -s.

commit name should look like PR name for the commit style check to pass.

run make check to check for linter issues.

Brice187 · 2023-11-10T17:30:46Z

I am very interested in this feature. Did you already build a publicly available container image I can test in dev ?

eusebiu-constantin-petu-dbk · 2023-11-13T18:14:02Z

you need to update makeDownstreamServer() in sync_test.go to use sync config's TmpDir

add this line: destConfig.Extensions.Sync.TmpDir = destDir, so that tests, where we use chmod to trigger errors, passes.

rchincha · 2023-11-15T03:09:03Z

@Brice187 @elee1766

Just curious about this requirement. The most common use case for sync is to do a local mirror so that you are decoupled from the cloud wrt costs and connectivity. So could you enlighten us on this use case more where you want S3 as the backend. Why not just use the registries offered by the cloud providers directly?

elee1766 · 2023-11-15T04:22:52Z

@Brice187 @elee1766

Just curious about this requirement. The most common use case for sync is to do a local mirror so that you are decoupled from the cloud wrt costs and connectivity. So could you enlighten us on this use case more where you want S3 as the backend. Why not just use the registries offered by the cloud providers directly?

yeah sure, let me explain.

so we use the gitlab ci + the container registry to produce tagged images there, ala v0.1.2, to create releases. we then use harbor to synchronize from the self hosted gitlab -> the registries offered by the cloud providers directly. we don't see the gitlab container registry as viable for production deployments.

zot can replace both harbor and the registry offered by the cloud provider for this use case. at digitalocean, it's the same price. (a sub 20 dollar app engine app is enough to run it, and they use the same storage backend anyways)

rchincha · 2023-11-15T06:19:06Z

@elee1766

zot can replace both harbor and the registry offered by the cloud provider for this use case. at digitalocean, it's the same price. (a sub 20 dollar app engine app is enough to run it, and they use the same storage backend anyways)

I see, and you will deploy apps on DO via images hosted on your private zot (synced with images from gitlab CI)

Brice187 · 2023-11-15T17:00:49Z

@Brice187 @elee1766

Just curious about this requirement. The most common use case for sync is to do a local mirror so that you are decoupled from the cloud wrt costs and connectivity. So could you enlighten us on this use case more where you want S3 as the backend. Why not just use the registries offered by the cloud providers directly?

This is a pretty edge case scenario:
We want to have every software artifact cached locally in our system and the network segment, which can talk to the internet (dmz), does not have highly available storage. The container registry is located in this segment. The packetfilter to the internal network (where our HA storage is located) only allows https traffic from the dmz.

rchincha · 2023-11-15T18:02:59Z

@peusebiu Pls add comments for next steps.

@elee1766 do you want to make this PR ready for review (currently in draft state)?

elee1766 · 2023-11-15T22:27:27Z

not yet, I still need to work on tests. have been travelling recently

…

On Wed, Nov 15, 2023, 12:03 PM Ramkumar Chinchani ***@***.***> wrote: @peusebiu <https://github.com/peusebiu> Pls add comments for next steps. @elee1766 <https://github.com/elee1766> do you want to make this PR ready for review (currently in draft state)? — Reply to this email directly, view it on GitHub <#2007 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARH66INA6ZDA2VVXYQQHWLYET7V7AVCNFSM6AAAAAA65DG7ASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJTGAYTAMBRGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

eusebiu-constantin-petu-dbk · 2023-11-16T11:32:24Z

pkg/extensions/extension_sync.go

-					sch.SubmitGenerator(gen, registryConfig.PollInterval, scheduler.MediumPriority)
-				}
+			tmpDir := config.Extensions.Sync.TmpDir
+			if tmpDir == "" {


I would say, that by default, it should still sync temporarily under each repo, like today(main). if a TmpDir is given in config, then use that, this way all the tests should pass.

right @rchincha ?

Yes, we do not want to step outside of our "storageRootDir" path. That's all the space we will own and manage, and nothing outside of it.

@elee1766 My suggestion is, if there is a cloud backend defined in storage, then instead, do a second step of writing to that backend after the local sync is done

eusebiu-constantin-petu-dbk · 2023-11-16T11:34:13Z

you need to update makeDownstreamServer() in sync_test.go to use sync config's TmpDir

add this line: destConfig.Extensions.Sync.TmpDir = destDir, so that tests, where we use chmod to trigger errors, passes.

reiterating this :D, that's why sync_test.go fails. but I would say that by default sync should temp copy under each "repo/.sync", and not /tmp, and this way tests will pass.

elee1766 · 2023-11-16T23:00:30Z

ok, changed the sync_test.go as u said, and it seems to be happier now.

I able to make the bats test pass locally, although it seems it failing on ci. the joys of integration testing.

ill probably have to do some witchcraft later and force push to fix the commit requirements, for now i'll mark it as not a draft though, i am feeling better about it.

rchincha · 2023-11-17T23:31:26Z

@elee1766 fyi, https://github.com/project-zot/zot/releases/tag/v2.0.0-rc7
^ released this.

Would definitely like to put this PR in the next/final release.

rchincha · 2023-11-17T23:32:56Z

go.mod

@@ -1,6 +1,8 @@
 module zotregistry.io/zot

-go 1.20
+go 1.21


@elee1766 did you need go 1.21? We are still migrating to that.
#2049

This new "toolchain" directive is breaking all CI tests.

i don't think i need it. i'll push with 1.20

Signed-off-by: a <[email protected]>

elee1766 · 2023-11-18T17:20:26Z

seems some ci tests are failing, but i can't seem to replicate this locally.

what is the correct way to replicate the ci test failures locally without waiting for actual github ci ?

rchincha · 2023-11-21T00:26:35Z

pkg/extensions/config/sync/config.go

@@ -15,6 +15,7 @@ type Credentials struct {
 type Config struct {
 	Enable          *bool
 	CredentialsFile string
+	TmpDir          string


Would remove this.

rchincha · 2023-11-21T00:27:34Z

pkg/extensions/extension_sync.go

-					sch.SubmitGenerator(gen, registryConfig.PollInterval, scheduler.MediumPriority)
-				}
+			tmpDir := config.Extensions.Sync.TmpDir
+			if tmpDir == "" {


@elee1766 My suggestion is, if there is a cloud backend defined in storage, then instead, do a second step of writing to that backend after the local sync is done

eusebiu-constantin-petu-dbk · 2023-11-21T14:38:00Z

seems some ci tests are failing, but i can't seem to replicate this locally.

what is the correct way to replicate the ci test failures locally without waiting for actual github ci ?

You can locally run make run-blackbox-ci, usually I would manually run each bats file, looking on bats logs and also on zot's logs.

@rchincha

I would say the default for this feature would be to save under zot's rootDir like today, and if an imageStore is remote and we have sync config TmpDir then we will temp sync there, then move everything on remote(s3).

Otherwise zot will just throw an error, that storage is remote and for sync to work it needs a TmpDir.

I would say to not start zot with remote storage+sync without TmpDir set by the user.

@elee1766 @Brice187 Would this ^ meet your needs?

I will take this PR locally and will try to figure out what is missing for this.

eusebiu-constantin-petu-dbk · 2023-11-22T15:45:29Z

#2073

elee1766 · 2023-11-22T17:21:58Z

yeah it's more than enough.

rchincha · 2023-11-28T20:09:43Z

#2073

@elee1766 @Brice187 ^ this has now been merged.
Pls try from top of main. An example has been added to help with the config.

rchincha · 2023-11-28T20:11:17Z

Closing this PR. @elee1766 thanks for getting this started.

elee1766 requested review from eusebiu-constantin-petu-dbk, rchincha and andaaron as code owners November 3, 2023 23:29

elee1766 marked this pull request as draft November 3, 2023 23:29

elee1766 changed the title ~~feat: sync remote storage~~ feat(sync): allow remote storage Nov 3, 2023

elee1766 force-pushed the sync-remote-storage branch from ce8cd09 to 88839b0 Compare November 6, 2023 21:23

elee1766 force-pushed the sync-remote-storage branch 4 times, most recently from eaecae6 to d700667 Compare November 11, 2023 06:48

eusebiu-constantin-petu-dbk reviewed Nov 16, 2023

View reviewed changes

elee1766 marked this pull request as ready for review November 16, 2023 23:00

rchincha reviewed Nov 17, 2023

View reviewed changes

elee1766 force-pushed the sync-remote-storage branch 2 times, most recently from f3cded9 to 069fd22 Compare November 18, 2023 02:31

feat(sync/remote): allow remote storage

f91c8af

Signed-off-by: a <[email protected]>

elee1766 force-pushed the sync-remote-storage branch from 069fd22 to f91c8af Compare November 18, 2023 02:37

fix(sync/remote): make linter happier

e30edbd

Signed-off-by: a <[email protected]>

elee1766 force-pushed the sync-remote-storage branch from ac499c8 to e30edbd Compare November 18, 2023 02:42

fix(sync): go version

07fda0e

Signed-off-by: a <[email protected]>

rchincha requested changes Nov 21, 2023

View reviewed changes

rchincha closed this Nov 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sync): allow remote storage #2007

feat(sync): allow remote storage #2007

elee1766 commented Nov 3, 2023 •

edited

Loading

rchincha commented Nov 4, 2023

elee1766 commented Nov 4, 2023 •

edited

Loading

rchincha commented Nov 5, 2023

eusebiu-constantin-petu-dbk commented Nov 6, 2023 •

edited

Loading

Brice187 commented Nov 10, 2023

eusebiu-constantin-petu-dbk commented Nov 13, 2023 •

edited

Loading

rchincha commented Nov 15, 2023 •

edited

Loading

elee1766 commented Nov 15, 2023

rchincha commented Nov 15, 2023

Brice187 commented Nov 15, 2023 •

edited

Loading

rchincha commented Nov 15, 2023

elee1766 commented Nov 15, 2023 via email

eusebiu-constantin-petu-dbk Nov 16, 2023 •

edited

Loading

rchincha Nov 16, 2023

rchincha Nov 21, 2023

eusebiu-constantin-petu-dbk commented Nov 16, 2023 •

edited

Loading

elee1766 commented Nov 16, 2023

rchincha commented Nov 17, 2023

rchincha Nov 17, 2023

elee1766 Nov 18, 2023

elee1766 commented Nov 18, 2023

rchincha Nov 21, 2023

rchincha Nov 21, 2023

eusebiu-constantin-petu-dbk commented Nov 21, 2023 •

edited

Loading

eusebiu-constantin-petu-dbk commented Nov 22, 2023

elee1766 commented Nov 22, 2023

rchincha commented Nov 28, 2023

rchincha commented Nov 28, 2023

feat(sync): allow remote storage #2007

feat(sync): allow remote storage #2007

Conversation

elee1766 commented Nov 3, 2023 • edited Loading

rchincha commented Nov 4, 2023

elee1766 commented Nov 4, 2023 • edited Loading

rchincha commented Nov 5, 2023

eusebiu-constantin-petu-dbk commented Nov 6, 2023 • edited Loading

Brice187 commented Nov 10, 2023

eusebiu-constantin-petu-dbk commented Nov 13, 2023 • edited Loading

rchincha commented Nov 15, 2023 • edited Loading

elee1766 commented Nov 15, 2023

rchincha commented Nov 15, 2023

Brice187 commented Nov 15, 2023 • edited Loading

rchincha commented Nov 15, 2023

elee1766 commented Nov 15, 2023 via email

eusebiu-constantin-petu-dbk Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

rchincha Nov 16, 2023

Choose a reason for hiding this comment

rchincha Nov 21, 2023

Choose a reason for hiding this comment

eusebiu-constantin-petu-dbk commented Nov 16, 2023 • edited Loading

elee1766 commented Nov 16, 2023

rchincha commented Nov 17, 2023

rchincha Nov 17, 2023

Choose a reason for hiding this comment

elee1766 Nov 18, 2023

Choose a reason for hiding this comment

elee1766 commented Nov 18, 2023

rchincha Nov 21, 2023

Choose a reason for hiding this comment

rchincha Nov 21, 2023

Choose a reason for hiding this comment

eusebiu-constantin-petu-dbk commented Nov 21, 2023 • edited Loading

eusebiu-constantin-petu-dbk commented Nov 22, 2023

elee1766 commented Nov 22, 2023

rchincha commented Nov 28, 2023

rchincha commented Nov 28, 2023

elee1766 commented Nov 3, 2023 •

edited

Loading

elee1766 commented Nov 4, 2023 •

edited

Loading

eusebiu-constantin-petu-dbk commented Nov 6, 2023 •

edited

Loading

eusebiu-constantin-petu-dbk commented Nov 13, 2023 •

edited

Loading

rchincha commented Nov 15, 2023 •

edited

Loading

Brice187 commented Nov 15, 2023 •

edited

Loading

eusebiu-constantin-petu-dbk Nov 16, 2023 •

edited

Loading

eusebiu-constantin-petu-dbk commented Nov 16, 2023 •

edited

Loading

eusebiu-constantin-petu-dbk commented Nov 21, 2023 •

edited

Loading