rkt: Implement distribution points by s-urbaniak · Pull Request #3369 · rkt/rkt

s-urbaniak · 2016-11-14T11:29:42Z

This is the implementation of the distribution concept proposed in #2953.

Supersedes #3101

This is work in progress to check the output of CI.

s-urbaniak

these are notes to myself

s-urbaniak · 2016-11-14T11:35:07Z

pkg/distribution/aciarchive.go

+}
+
+// NewACIArchive creates a new aci-archive distribution from the provided distribution uri
+// string


s-urbaniak · 2016-11-14T11:35:29Z

pkg/distribution/aciarchive.go

+		return nil, fmt.Errorf("cannot parse URI: %q: %v", u.String(), err)
+	}
+	if c.Type != TypeACIArchive {
+		return nil, fmt.Errorf("wrong distribution type: %q", c.Type)


s/wrong/illegal

s-urbaniak · 2016-11-14T11:35:43Z

pkg/distribution/aciarchive.go

+	// This should be a valid URL
+	data, err := url.QueryUnescape(c.Data)
+	if err != nil {
+		return nil, fmt.Errorf("wrong archive transport url %q: %v", c.Data, err)


s/wrong/illegal

s-urbaniak · 2016-11-14T11:35:59Z

pkg/distribution/aciarchive.go

+	}
+	aciu, err := url.Parse(data)
+	if err != nil {
+		return nil, fmt.Errorf("wrong archive transport url %q: %v", c.Data, err)


s/wrong/illegal

s-urbaniak · 2016-11-14T11:36:11Z

pkg/distribution/aciarchive.go

+
+	str := u.String()
+
+	path := aciu.String()


superfluous newlines

s-urbaniak · 2016-11-14T11:45:55Z

pkg/distribution/docker.go

+
+// SimpleDockerString returns a simplyfied docker cimd. This means removing
+// the index url if it's the default docker registry (registry-1.docker.io),
+// removing the default repo (library) when using the default docker registry


correct this doc, we are not returning a cimd

s-urbaniak · 2016-11-14T11:46:08Z

pkg/distribution/docker.go

+
+// FullDockerCIMD return the docker cimd populated with all the default values
+// docker strings like "busybox" or
+// "registry-1.docker.io/library/busybox:latest" will become the same docker


correct this doc, we are not returning a cimd

s-urbaniak · 2016-11-14T11:47:04Z

rkt/image/common.go

+func DistFromImageString(is string) (dist.Distribution, error) {
+	u, err := url.Parse(is)
+	if err != nil {
+		errwrap.Wrap(fmt.Errorf("failed to parse image string %q", is), err)


failed to parse image string %q as url

s-urbaniak · 2016-11-14T11:48:44Z

rkt/image/fetcher.go

 // file for verification, unless verification is disabled. If
 // f.WithDeps is true also image dependencies are fetched.
-func (f *Fetcher) FetchImage(img string, ascPath string, imgType apps.AppImageType) (string, error) {
+func (f *Fetcher) FetchImage(d dist.Distribution, str, ascPath string) (*types.Hash, error) {


the name str is too ambiguous

s-urbaniak · 2016-11-14T11:49:08Z

rkt/image/fetcher.go

+	h, err := types.NewHash(hash)
+	if err != nil {
+		// should never happen
+		log.PanicE("got an invalid hash, looks like it is corrupted", err)


"invalid hash"

s-urbaniak · 2016-11-14T12:44:48Z

pkg/distribution/cimd.go

+	}, nil
+}
+
+// NewCIMDString creates a new cimd URL


NewCIMDString creates a new cimd URL string.

s-urbaniak · 2016-11-14T12:51:39Z

pkg/distribution/distribution.go

+// Distribution is the interface that represents a distribution point.
+type Distribution interface {
+	// URI returns a copy of the Distribution CIMD URI.
+	URI() *url.URL


simply call this CIMD()?

s-urbaniak · 2016-11-15T12:02:44Z

rkt/image/common.go

 		return dist, nil
-	default:
-		// Try to create a dist directly from the provided URI
+	case "cimd":


reference the cimd schme constant directly

s-urbaniak · 2016-11-16T10:55:50Z

rkt/image/fetcher.go

 func (f *Fetcher) fetchImageDeps(hash string) error {
 	imgsl := list.New()
-	seen := map[string]struct{}{}
+	seen := map[dist.Distribution]struct{}{}


this seems very dangerous to me as it is very easy to miss the equality invariant of the underlying dynamic type. I'd rather make this map[string]dist.Distrbution, where the key is the string representation of the cimd URL.

s-urbaniak · 2016-11-16T14:53:12Z

@lucab PTAL

lucab · 2016-11-17T11:04:45Z

rkt/image/fetcher.go

+func (db *distBundle) setAppDefaults() error {
+	app := db.dist.(*dist.Appc).App()
 	if _, ok := app.Labels["arch"]; !ok {
 		app.Labels["arch"] = runtime.GOARCH


Note to self: this will need to be adjusted following appc/spec#668.

lucab

Some comments inline.

lucab · 2016-11-18T11:18:58Z

common/labelsort/labelsort.go

+import (
+	"sort"
+
+	"github.com/appc/spec/schema/types"


If this is something appc-specific, should we somehow label this to make it more explicit (and perhaps move to appc/spec at some point in the future)?

As discussed OOB: This is indeed appc-specific, and makes sense to move to appc/spec itself.

Ack. I'm fine to have it here for the moment to avoid a tag-and-sync delay, just leave a TODO note at the top for the moment.

lucab · 2016-11-18T11:40:18Z

pkg/distribution/distribution.go

+	return Get(u)
+}
+
+// from github.com/PuerkitoBio/purell


Please add the relevant license (BSD-3) to the header here, as well as the author. And this comment would better reference the exact file and version we took this from, for future-proof inspection.

hmm, I'll simply import the library, it is small, and we will have a direct dependency. Since sortQuery modifies the given url in-place using purell.NormalizeURL will work.

lucab · 2016-11-18T12:04:11Z

rkt/image/common.go

+func DistFromImageString(is string) (dist.Distribution, error) {
+	u, err := url.Parse(is)
+	if err != nil {
+		errwrap.Wrap(fmt.Errorf("failed to parse image url %q", is), err)


Missing return.

lucab · 2016-11-18T13:26:51Z

store/imagestore/store.go

+	for _, l := range ls {
+		out = append(out, fmt.Sprintf("%q:%q", l.Name, l.Value))
+	}
+	return "[" + strings.Join(out, ", ") + "]"


This doesn't look too nice, and seems to used exactly once in the code for error-printing. Should we get rid of it?

ack, the error format can refer directly to the %q fmt qualifier which will print i.e. [{"version" "1.0"} {"os" "linux"}] ..., so there is no need for this function.

lucab · 2016-11-21T17:03:44Z

This LGTM, but CI looks very confused here. Perhaps @s-urbaniak could re-trigger it later.

Currently glide errors out because some dependencies could not be found: [ERROR] Error scanning google.golang.org/grpc/stats: open /home/sur/.glide/cache/src/https-google.golang.org-grpc/stats: no such file or directory [ERROR] This error means the referenced package was not found. [ERROR] Missing file or directory errors usually occur when multiple packages [ERROR] share a common dependency and the first reference encountered by the scanner [ERROR] sets the version to one that does not contain a subpackage needed required [ERROR] by another package that uses the shared dependency. Try setting a [ERROR] version in your glide.yaml that works for all packages that share this [ERROR] dependency. [ERROR] Error scanning google.golang.org/grpc/tap: open /home/sur/.glide/cache/src/https-google.golang.org-grpc/tap: no such file or directory ... This fixes it by introducing them manually.

This removes the `-u` parameter suggestion for `glide get` since it is the default now.

This patch implements the distribution point concept with the three initial kind of distributions already used by rkt (appc, aci-archive, docker). It just makes the smallest required changes to the fetchers logic to make them work and pass the functional tests. This is the base for future fetchers (a future patch will refactor the fetcher logic to better fit the distribution concept) and store refactors.

Currently if the user specifies an app name like "app-name:v1.0" it won't be recognized because "app-name" is treated like an unknown URL scheme. This fixes it by assuming that unknown URL schemes imply appc images. Fixes rkt#3326

This was referenced Nov 14, 2016

rkt: Implement distribution points #3101

Closed

oci: move distribution proposal to proposals #3365

Closed

s-urbaniak added area/oci image do not merge labels Nov 14, 2016

s-urbaniak commented Nov 14, 2016

View reviewed changes

s-urbaniak force-pushed the pr-3101 branch 4 times, most recently from 33685a8 to d4542e5 Compare November 14, 2016 12:43

s-urbaniak commented Nov 14, 2016

View reviewed changes

s-urbaniak force-pushed the pr-3101 branch from d4542e5 to 33cc366 Compare November 14, 2016 12:46

s-urbaniak commented Nov 14, 2016

View reviewed changes

s-urbaniak force-pushed the pr-3101 branch 2 times, most recently from 175f3b6 to 97fa428 Compare November 15, 2016 11:22

s-urbaniak added this to the v1.20.0 milestone Nov 15, 2016

s-urbaniak mentioned this pull request Nov 15, 2016

Cannot correctly guess image type from name of format: image-name-with-hyphens:some_hash #3326

Closed

s-urbaniak force-pushed the pr-3101 branch from 97fa428 to fa29fd1 Compare November 15, 2016 11:57

s-urbaniak commented Nov 15, 2016

View reviewed changes

s-urbaniak force-pushed the pr-3101 branch from fa29fd1 to 724dfe7 Compare November 16, 2016 09:20

s-urbaniak changed the title ~~[WIP] rkt: Implement distribution points~~ rkt: Implement distribution points Nov 16, 2016

s-urbaniak removed the do not merge label Nov 16, 2016

s-urbaniak force-pushed the pr-3101 branch 2 times, most recently from 519b311 to 3bd8afa Compare November 16, 2016 09:57

s-urbaniak commented Nov 16, 2016

View reviewed changes

s-urbaniak force-pushed the pr-3101 branch from 3bd8afa to 9e2ffb3 Compare November 16, 2016 12:16

s-urbaniak added the needs/review label Nov 17, 2016

lucab reviewed Nov 17, 2016

View reviewed changes

lucab suggested changes Nov 18, 2016

View reviewed changes

s-urbaniak force-pushed the pr-3101 branch from 9e2ffb3 to c9badc4 Compare November 21, 2016 09:10

s-urbaniak force-pushed the pr-3101 branch 2 times, most recently from c70c6e7 to 0382834 Compare November 21, 2016 09:35

lucab approved these changes Nov 21, 2016

View reviewed changes

s-urbaniak and others added 8 commits November 21, 2016 19:48

Documentation/hacking: update description for adding dependencies

e04455c

This removes the `-u` parameter suggestion for `glide get` since it is the default now.

glide: add github.com/PuerkitoBio/purell

f84d620

glide: vendor github.com/PuerkitoBio/purell

e4c110a

glide: add github.com/PuerkitoBio/urlesc

49c0a79

glide: vendor dependencies github.com/PuerkitoBio/urlesc

02f77eb

s-urbaniak force-pushed the pr-3101 branch from 0382834 to e339ef3 Compare November 21, 2016 18:49

lucab added reviewed/lgtm and removed needs/review labels Nov 22, 2016

lucab merged commit 4c0a92f into rkt:master Nov 22, 2016

Conversation

s-urbaniak commented Nov 14, 2016

Uh oh!

s-urbaniak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-urbaniak commented Nov 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucab commented Nov 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants