ostree: new option for the overlay driver #137

giuseppe · 2017-11-14T10:41:33Z

if ostree_repo is specified, then any layer is first committed to the specified ostree repository and then checkout out to the same location so that files are deduplicated using OSTree hardlinks.

The exclude_ostree BUILDTAG can be used to disable ostree support.

rhatdan · 2017-11-15T14:43:21Z

drivers/ostree/driver.go

+	}
+
+	_, err = os.Stat(repoLocation)
+	if err != nil {


Should you check if this err is enoexist otherwise return the error.

rhatdan · 2017-11-15T14:43:51Z

drivers/ostree/driver.go

+		if err := idtools.MkdirAllAndChown(repoLocation, 0700, rootIDs); err != nil {
+			return nil, err
+		}
+		err := exec.Command("ostree", fmt.Sprintf("--repo=%s", repoLocation), "init").Run()


Nit do this on same line
if err := exec.Command("ostree", fmt.Sprintf("--repo=%s", repoLocation), "init").Run(); err != nil {
}

rhatdan · 2017-11-15T14:44:34Z

drivers/ostree/driver.go

+		idMappings := idtools.NewIDMappingsFromMaps(uidMaps, gidMaps)
+		rootIDs := idMappings.RootPair()
+		if err := idtools.MkdirAllAndChown(repoLocation, 0700, rootIDs); err != nil {
+			return nil, err


Can you wrap this error with something helpful to the user.

rhatdan · 2017-11-15T14:45:00Z

drivers/ostree/driver.go

+
+	repo, err := otbuiltin.OpenRepo(repoLocation)
+	if err != nil {
+		return nil, err


Does this give a use fule error message?

rhatdan · 2017-11-15T14:45:58Z

drivers/ostree/driver.go

+	commitOpts.Parent = "0000000000000000000000000000000000000000000000000000000000000000"
+	branch := fmt.Sprintf("ocilayer/%s", id)
+
+	_, err = d.repo.Commit(root, branch, commitOpts)


Nit; Put this in one line.

rhatdan · 2017-11-15T14:46:29Z

drivers/ostree/driver.go

+	}
+
+	err = system.EnsureRemoveAll(root)
+	if err != nil {


Nit; Put this in one line.

rhatdan · 2017-11-15T14:46:37Z

drivers/ostree/driver.go

+	}
+
+	checkoutOpts := otbuiltin.NewCheckoutOptions()
+	err = otbuiltin.Checkout(d.repoLocation, root, branch, checkoutOpts)


Nit; Put this in one line.

rhatdan · 2017-11-15T14:47:07Z

drivers/ostree/driver.go

+	checkoutOpts := otbuiltin.NewCheckoutOptions()
+	err = otbuiltin.Checkout(d.repoLocation, root, branch, checkoutOpts)
+	if err != nil {
+		return err


Useful error?

rhatdan · 2017-11-15T14:47:27Z

drivers/ostree/driver.go

+
+	_, err = d.repo.CommitTransaction()
+	if err != nil {
+		return err


Usefull error?

rhatdan · 2017-11-15T14:48:24Z

drivers/ostree/driver.go

+	if convert {
+		err := d.convertToOSTree(root, id)
+		if err != nil {
+			return err


useful error?

@giuseppe Does convertToOSTRee give a useful error?

yes, it is defined in this module and it already gives useful errors

Thanks for checking.

giuseppe · 2017-11-15T19:04:28Z

@rhatdan, I've pushed another version with all the comments addressed ⬆️

rhatdan · 2017-11-15T20:26:55Z

drivers/ostree/driver.go

+}
+
+func getDir(home, id string) string {
+	return fmt.Sprintf("%s/%s/diff", home, id)


We tend to use filepath.Join() for this type of thing.

rhatdan · 2017-11-15T20:27:24Z

drivers/ostree/driver.go

+// Create prepares the filesystem for the OSTREE driver and copies the directory for the given id under the parent.
+func (d *Driver) convertToOSTree(root, id string) error {
+	if _, err := d.repo.PrepareTransaction(); err != nil {
+		return fmt.Errorf("Could not prepare the OSTree transaction: %v", err)


Use
errors.Wrapf(err, "could not prepare the OSTree transaction")

Standard is to have errors start with lowercase. I believe.

rhatdan · 2017-11-15T20:27:42Z

drivers/ostree/driver.go

+	branch := fmt.Sprintf("ocilayer/%s", id)
+
+	if _, err := d.repo.Commit(root, branch, commitOpts); err != nil {
+		return fmt.Errorf("Could not commit the layer: %v", err)


errors.Wrapf.

rhatdan · 2017-11-15T20:27:53Z

drivers/ostree/driver.go

+	}
+
+	if _, err := d.repo.CommitTransaction(); err != nil {
+		return fmt.Errorf("Could not complete the OSTree transaction: %v", err)


rhatdan · 2017-11-15T20:28:31Z

drivers/ostree/driver.go

+	}
+
+	if err := system.EnsureRemoveAll(root); err != nil {
+		return err


errors.Wrapf(err, "failed to remove %q", root)

@giuseppe What about this?

I looked at the code and it already gives a similar error message, so I think it would be duplicated return errors.Wrapf(e, "error while removing %s", dir). In some other cases it returns directly the error from os.RemoveAll()

rhatdan · 2017-11-15T20:28:49Z

drivers/ostree/driver.go

+
+	checkoutOpts := otbuiltin.NewCheckoutOptions()
+	if err := otbuiltin.Checkout(d.repoLocation, root, branch, checkoutOpts); err != nil {
+		return fmt.Errorf("Could not checkout from OSTree: %v", err)


errors.Wrapf

rhatdan · 2017-11-15T20:29:06Z

drivers/ostree/driver.go

+func (d *Driver) Remove(id string) error {
+	branch := fmt.Sprintf("ocilayer/%s", id)
+	if err := exec.Command("ostree", fmt.Sprintf("--repo=%s", d.repoLocation), "refs", "--delete", branch).Run(); err != nil {
+		return fmt.Errorf("Could not delete OSTree branch: %v", err)


errors.Wrapf("Could not delete OSTree branch")

rhatdan · 2017-11-15T20:29:32Z

drivers/ostree/driver.go

+
+// Get returns the directory for the given id.
+func (d *Driver) Get(id, mountLabel string) (string, error) {
+	path, err := d.overlay.Get(id, mountLabel)


return d.overlay.Get(id, mountLabel)

rhatdan · 2017-11-15T20:30:27Z

drivers/ostree/driver_stub.go

+}
+
+func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (graphdriver.Driver, error) {
+	return nil, fmt.Errorf("ostree driver not supported")


Use errors.Errorf()

giuseppe · 2017-11-16T07:42:56Z

thanks for the quick review. I've addressed the new comments

rhatdan · 2017-11-16T18:01:59Z

@nalind Should we add a new driver or enhance the Overlay driver to use OSTREE via an option?
overlay.

rhatdan · 2018-06-22T14:11:41Z

@giuseppe Tests are failing.

giuseppe · 2018-06-22T17:20:08Z

@rhatdan fixed. I needed to set the mtime to the EPOCH so it doesn't trigger a difference when OSTree is used.

giuseppe · 2018-06-22T17:55:07Z

~~the only missing part that I would like to work on as a follow-up is dropping the ostree CLI usage and add the missing features to the go bindings (delete branch and init repository).~~

Fixed in the last version I've pushed

rhatdan · 2018-06-25T15:35:44Z

LGTM
@nalind PTAL

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2018-06-29T16:00:11Z

rebased

usage example: skopeo copy docker-daemon:busybox:latest containers-storage:\[overlay2@/var/lib/containers/storage+/var/run/containers/storage:overlay2.ostree_repo=/var/lib/containers/storage/overlay2/ostree/.repo,overlay2.override_kernel_check=1\]\busybox Signed-off-by: Giuseppe Scrivano <[email protected]>

Signed-off-by: Giuseppe Scrivano <[email protected]>

It is needed to use an OSTree repository (either directly or as a parent repository) that is not under the storage home directory. Signed-off-by: Giuseppe Scrivano <[email protected]>

Signed-off-by: Giuseppe Scrivano <[email protected]>

so it won't be different once added to OSTree. Signed-off-by: Giuseppe Scrivano <[email protected]>

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2018-07-04T14:03:33Z

@rhatdan fine to merge this?

rhatdan · 2018-07-05T10:23:03Z

Going to finally merge this one.

giuseppe force-pushed the ostree-storage branch 15 times, most recently from 6362511 to 9f8471c Compare November 14, 2017 14:42

rhatdan self-requested a review November 15, 2017 14:40

rhatdan reviewed Nov 15, 2017

View reviewed changes

giuseppe force-pushed the ostree-storage branch 3 times, most recently from 52c18cf to daaf5f9 Compare November 15, 2017 17:45

rhatdan reviewed Nov 15, 2017

View reviewed changes

giuseppe force-pushed the ostree-storage branch 5 times, most recently from 6527f1b to ba78a99 Compare November 16, 2017 07:29

giuseppe force-pushed the ostree-storage branch from ba78a99 to 62c45db Compare December 1, 2017 17:39

giuseppe force-pushed the ostree-storage branch 5 times, most recently from e5fdfba to 40f7a37 Compare June 22, 2018 16:12

giuseppe force-pushed the ostree-storage branch 2 times, most recently from 05bfb70 to 82a6eb2 Compare June 23, 2018 10:50

giuseppe added 2 commits June 29, 2018 17:58

storage: add ostree files

eca8a17

Signed-off-by: Giuseppe Scrivano <[email protected]>

ostree: new package

ffd808a

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the ostree-storage branch from 82a6eb2 to d0e283c Compare June 29, 2018 15:59

giuseppe added 8 commits June 29, 2018 18:47

vfs: add support for ostree deduplication

52a3781

Signed-off-by: Giuseppe Scrivano <[email protected]>

configuration: new option skip_mount_home

f3e7ee3

It is needed to use an OSTree repository (either directly or as a parent repository) that is not under the storage home directory. Signed-off-by: Giuseppe Scrivano <[email protected]>

vagrant: install ostree

6c122a2

Signed-off-by: Giuseppe Scrivano <[email protected]>

Vagrantfile: update to Fedora 28

85bf5a4

Signed-off-by: Giuseppe Scrivano <[email protected]>

ci: install ostree

c7fdad4

Signed-off-by: Giuseppe Scrivano <[email protected]>

tests: set mtime to the epoch

d33931a

so it won't be different once added to OSTree. Signed-off-by: Giuseppe Scrivano <[email protected]>

tests: add test for ostree deduplication

13af8ce

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the ostree-storage branch from d0e283c to 13af8ce Compare June 29, 2018 16:47

giuseppe mentioned this pull request Jul 4, 2018

vendor: update containers/storage containers/buildah#846

Closed

rhatdan merged commit 9cbb6cb into containers:master Jul 5, 2018

giuseppe mentioned this pull request Oct 26, 2018

checkout: honor opaque checkouts ostreedev/ostree#1486

Closed

runcom mentioned this pull request Nov 9, 2018

devmapper regression #233

Closed

ostree: new option for the overlay driver #137

ostree: new option for the overlay driver #137

Conversation

giuseppe commented Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe commented Nov 16, 2017

rhatdan commented Nov 16, 2017

rhatdan commented Jun 22, 2018

giuseppe commented Jun 22, 2018

giuseppe commented Jun 22, 2018 • edited Loading

rhatdan commented Jun 25, 2018

giuseppe commented Jun 29, 2018

giuseppe commented Jul 4, 2018

rhatdan commented Jul 5, 2018

giuseppe commented Nov 14, 2017 •

edited

Loading

rhatdan Nov 15, 2017 •

edited

Loading

giuseppe commented Jun 22, 2018 •

edited

Loading