Skip to content
This repository has been archived by the owner on Aug 14, 2020. It is now read-only.

Iaguis/squash #6

Merged
merged 40 commits into from
Feb 5, 2015
Merged

Iaguis/squash #6

merged 40 commits into from
Feb 5, 2015

Conversation

iaguis
Copy link
Member

@iaguis iaguis commented Jan 28, 2015

This PR implements squash functionality for ACI images. The last 3 commits are the interesting ones, the rest is covered by #5.

Fixes #2

@iaguis
Copy link
Member Author

iaguis commented Jan 29, 2015

Updated PR

endpoints,
// TODO(iaguis) discover if httpsOrHTTP
fmt.Sprintf("https://%s/v1/", strings.TrimSpace(endpointEl)))
layersOutputDir := "."
Copy link
Contributor

Choose a reason for hiding this comment

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

this should just be outputDir no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooops, good catch. Thanks!

The previous approach to squashing would simply go through the layers
from the app layer to the base layer and add the files to the final ACI
image file.

This produced incorrect ACIs because it doesn't preserve the extraction
order. When a file was overwriting a directory present in a previous
layer, the squashed ACI had the directory entry *after* the file.

We now create a map of files to ACI layers and populate it starting from
the base image. If files are repeated we take the last one. Then, we do
a second pass and add the file entry to the output tarball if the
current layer is the correct one.
Since we can have entries like

    rootfs/.wh.what/ever

without having

    rootfs/.wh.what/

the previous solution didn't work
The ACI format expects a rootfs entry in the tar file, we now add one
when generating the ACIs.

Also, aci.ArchiveWriter writes the manifest with current date, which
leads to non-reproducable ACIs. I don't think modifying the aci package
is the right thing to do so we write directly to the tar file.
@philips
Copy link
Contributor

philips commented Feb 3, 2015

I think we should just move this and keep moving forward. /cc @jonboulle

@sgotti
Copy link
Member

sgotti commented Feb 4, 2015

@iaguis I was looking at the patchset. Wouldn't be better to reuse some existing code? Right now I splitted the acirenderer patch in two parts (rkt/rkt#464 and rkt/rkt#465) and the first one should be used also by docker2aci and doesn't need any previous PR.

I was thinking of this code path:

@iaguis
Copy link
Member Author

iaguis commented Feb 4, 2015

I'd like to reuse your code. Right now I see two problems though:

  • acirenderer depends on rocket. It's only ptar so it should be easy to decouple
  • acirenderer extracts the ACIs to disk. This simplifies everything but it requires root which I was trying to avoid.

Any thoughts on that? Maybe requiring root is not a big deal?

@jonboulle
Copy link
Contributor

acirenderer depends on rocket. It's only ptar so it should be easy to decouple

Yep, no need for this to live in Rocket itself

acirenderer extracts the ACIs to disk. This simplifies everything but it requires root which I was trying to avoid.

Hmm, why does it need root (a priori of being applied to a filesystem that needs root)?

@jonboulle
Copy link
Contributor

@iaguis @sgotti I would love to see the acirenderer code reused if at all possible, but I also think we should probably just land this now and follow up with that.

Do you think it makes sense for that code to move "upstream" to appc/spec or so?

@iaguis
Copy link
Member Author

iaguis commented Feb 4, 2015

Hmm, why does it need root (a priori of being applied to a filesystem that needs root)?

Extracting files with the correct permissions/owners or with node files requires root since a regular user can't make the appropriate calls (chown, chmod...)

I also think we should probably just land this now and follow up with that.

I agree

@jonboulle
Copy link
Contributor

Hah, OK, I was thinking completely backwards
On Feb 4, 2015 4:18 PM, "Iago López Galeiras" notifications@github.com
wrote:

Hmm, why does it need root (a priori of being applied to a filesystem that
needs root)?

Extracting files with the correct permissions/owners or with node files
requires root since a regular user can't make the appropriate calls (chown,
chmod...)

I also think we should probably just land this now and follow up with that.

I agree


Reply to this email directly or view it on GitHub
#6 (comment).

@sgotti
Copy link
Member

sgotti commented Feb 4, 2015

@jonboulle @iaguis You're right!

In the meantime I just changed acirenderer with a better algo so now it will create a list of files to extract for every aci.
This list can be used by another function to directly create a tar (like needed by docker2aci, or to extract it to a filesystem, like the original acirenderer).

The extraction part is in its file (extract.go) so this can also be splitted and the acirenderer core (acirenderer.go and the tests) can be moved.
@jonboulle appc/spec can be ok as the place to move it.

PTAL (rkt/rkt#464)

@iaguis
Copy link
Member Author

iaguis commented Feb 4, 2015

That's awesome! I'll try to use it in docker2aci and let you know any issues I find.

@philips
Copy link
Contributor

philips commented Feb 4, 2015

I just found this library: https://github.com/winchman/libsquash

@jonboulle jonboulle mentioned this pull request Feb 5, 2015
@jonboulle
Copy link
Contributor

Landing this and we can roll forward with the follow ups: #9 #10

jonboulle added a commit that referenced this pull request Feb 5, 2015
@jonboulle jonboulle merged commit bb72ee5 into appc:master Feb 5, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker2aci: support squashing images
4 participants