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

oci/cas/dir: Load blob URI from oci-layout #214

Closed
wants to merge 4 commits into from

Conversation

wking
Copy link
Contributor

@wking wking commented Nov 3, 2017

With this change, users can configure their blob storage once at init time with an optional --blob-uri. Most other commands (which do not need path → blob conversion) can load the blob location from the oci-layout layout file (the 1.1.0 format is in flight). The only other user-facing change is to umoci gc, which gains a --digest-regexp. Folks who customized their blob URI will need to supply --digest-regexp to reverse whichever blob URI they're using.

This seems like a more convenient interface to me than requiring all callers to provide the custom blob location (#190). And it is more powerful as well, allowing users to shard their blob storage, etc. if they feel moved to do so.

I've also swapped in my external CAS implementation, since that provides the interfaces we could use for read/write CAS distinctions (#185). Obviously you could do all these same things with a local-to-umoci CAS implementation as well. I'd rather put the time into a stand-alone implementaion that can also be used in the oci-discovery context. But it's all Apache 2.0, so we can copy/paste between repos if you see anything you like and aren't interested in an external dependency.

@cyphar
Copy link
Member

cyphar commented Nov 3, 2017

Absolutely not, Trevor.

This sort of mis-use of "oci-discovery" is precisely why I didn't like it -- you've blended together the concepts of distribution and internal structure so much to the point where a PR like this turns up. What on earth does local (internal) structure of an OCI layout have to do with distribution? Is the plan to allow you to specify alternative places to fetch blobs inside an OCI image? But then how do we deal with mirroring and so on? Is mirroring going to require modifying an image's internal structure, or will there be multiple places you can get this information from (which now just increases the number of systems which have to be in place to do mirroring).

To be clear, I think that allowing for custom internal structures is probably a good thing.

However, I don't like that you're using "oci-discovery" for this purpose. I also don't understand why you've reimplemented oci/cas in the form of wking/casengine and now have made oci/cas just use that -- I don't understand the benefits and it just feels like reinventing the wheel for no real gain.

@wking
Copy link
Contributor Author

wking commented Nov 3, 2017 via email

wking added 2 commits November 3, 2017 22:17
This commit hard-codes the blobs/{algorithm}/{encoded} template [1],
but sets the stage for future work to relax that positioning [2].

I'm adding a PutIndex call in the tests, becase the CAS implementation
now has its own temp directory which is not known to the dirEngine.
Casengine's dir implementation does not use .umoci-* temporary
directories (it uses .casengine-* temporary directories), so it's
protected from Clean.  And the .casengine-* implementation does not
currently provide it's own Clean() implementation, although I may add
that in the future.

The "Deprecated:" syntax is discussed in [3,4,5].

Also adjust Close() to return the first error it encounters, but to
continue to optimistically attempt the remaining cleanup, logging any
subsequent errors.

Bumping go-mtree pulls in [6] and gives us a lowercase sirupsen import
that is compatible with oci-discovery and casengine.

[1]: https://github.com/opencontainers/image-spec/blob/v1.0.0/image-layout.md#blobs
[2]: https://github.com/openSUSE/umoci/pull/190
[3]: https://blog.golang.org/godoc-documenting-go-code
[4]: golang/blog@257114a
[5]: golang/go#10909
[6]: vbatts/go-mtree#144

Signed-off-by: W. Trevor King <wking@tremily.us>
Generated with:

  $ git rm -rf vendor
  $ make update-deps
  $ git add vendor

I needed the initial remove to get rid of Sirupsen/logrus/LICENSE, and
thought a blanket remove would be the safest way to ensure we left no
stale files.

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking force-pushed the external-casengine branch 4 times, most recently from 955bc5e to 067fec0 Compare November 4, 2017 17:28
wking added 2 commits November 4, 2017 10:30
With this change, users can configure their blob storage once at init
time with an optional --blob-uri.  Most other commands (which do not
need path -> blob conversion) can load the blob location from the
oci-layout layout file (the 1.1.0 format is in flight with [1,2]).
The only other user-facing change is to 'umoci gc', which gains a
--digest-regexp.  Folks who customized their blob URI will need to
supply --digest-regexp to reverse whichever blob URI they're using.

This seems like a more convenient interface to me than requiring all
callers to provide the custom blob location [3].  And it is more
powerful as well, allowing users to shard their blob storage [4],
etc. if they feel moved to do so.

[1]: xiekeyang/oci-discovery#20
[2]: https://github.com/wking/image-spec/blob/ref-engine-discovery-layout/image-layout.md
[3]: https://github.com/openSUSE/umoci/pull/190
[4]: opencontainers/image-spec#449

Signed-off-by: W. Trevor King <wking@tremily.us>
Generated with:

  $ make update-deps
  $ git add vendor

Signed-off-by: W. Trevor King <wking@tremily.us>
@cyphar
Copy link
Member

cyphar commented Jun 7, 2020

This is fairly stale by now, closing. Also now that umoci is being considered to be an OCI Reference Implementation, this sort of feature would be very strongly frowned against.

@cyphar cyphar closed this Jun 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants