Fix handling of the volume directive #334

peter-evans · 2018-09-04T07:25:46Z

This is an attempt to fix #313 by not adding the created directories to the VOLUME command snapshot. Adding to the snapshot creates a layer in the container that doesn't exist when building with Docker.

container-tools-bot · 2018-09-04T07:26:00Z

Hi @peter-evans. Thanks for your PR.

I'm waiting for a GoogleContainerTools member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

peter-evans · 2018-09-04T08:01:09Z

The integration test for Dockerfile_test_volume might fail because of this offset. I'll try and run the integration tests locally to fix if necessary.

priyawadhwa

Thanks for contributing! It seems like there is a bug with the VOLUME command, but I'm not sure if completely removing it from the snapshot will fix the error.

The tests aren't failing right now because snapshotting is still happening. You'll need to return an empty string array here to indicate that the filesystem shouldn't be snapshotted.

With just that change the test should fail because of the offset, but let's verify that before moving forward.

peter-evans · 2018-09-07T00:23:09Z

If removing the snapshot completely causes issues for subsequent layers an alternative might be to set a property on the snapshot layer indicating that it shouldn't be written to the container.

priyawadhwa · 2018-09-07T17:32:09Z

Yup, as expected the layers test failed:

FAIL: TestLayers/test_layer_Dockerfile_test_volume (13.38s)
    	integration_test.go:244: Difference in number of layers in each image is 0 but should be 1. Image 1: Image: [gcr.io/kaniko-test/docker-dockerfile_test_volume] Digest: [032330a4ebbb08b66fcb1bc8dd5670309fcf99a8138b0a201f4e4a3644e23abb] Number of Layers: [4], Image 2: Image: [gcr.io/kaniko-test/kaniko-dockerfile_test_volume] Digest: [983740719a6d9a9a7e1afa618f390e7db2170a1188f730067d0ed77f55f61055] Number of Layers: [4]

But the filesystem test also failed:

FAIL: TestRun/test_Dockerfile_test_volume (27.64s)
    	integration_test.go:196: diff = [
    		  {
    		    "Image1": "gcr.io/kaniko-test/docker-dockerfile_test_volume",
    		    "Image2": "gcr.io/kaniko-test/kaniko-dockerfile_test_volume",
    		    "DiffType": "File",
    		    "Diff": {
    		      "Adds": null,
    		      "Dels": [
    		        {
    		          "Name": "/baz/bat",
    		          "Size": 0
    		        },
    		        {
    		          "Name": "/foo/bar",
    		          "Size": 0
    		        }
    		      ],
    		      "Mods": null
    		    }
    		  },
    		  {
    		    "Image1": "gcr.io/kaniko-test/docker-dockerfile_test_volume",
    		    "Image2": "gcr.io/kaniko-test/kaniko-dockerfile_test_volume",
    		    "DiffType": "Metadata",
    		    "Diff": {
    		      "Adds": [],
    		      "Dels": []
    		    }
    		  }
    		]

Meaning that /baz/bat and /foo/bar were in the docker built version of the image but not the kaniko version (since we removed snapshotting).

I think this will require further exploration into how the VOLUME command works, because it seems like the directories are created and snapshotted. However, this doesn't happen in a unique layer, which would explain why kaniko has an extra layers.

One theory could be that Docker snapshotting VOLUME happens in conjunction with another command. Would you be interested in exploring this more?

peter-evans · 2018-09-08T01:47:17Z

Sure. I'll do some further investigation.

peter-evans · 2018-09-12T04:35:55Z

I can't find any official documentation to back this up but this is my theory of how Docker treats the VOLUME directive based on some testing.

Dockerfile directives seem to be split into two categories:

Directives that trigger a layer to be written to the image if the file-system has changes.
e.g. FROM, COPY, ADD, RUN, etc.
Directives that do not trigger a layer to be written to the image regardless of whether the file-system has changes.
e.g. ARG, ENV, ENTRYPOINT, EXPOSE, etc.

The VOLUME directive is in the second category. It never triggers a layer to be written, even if it causes file-system changes. If directives in the second category make file-system changes since the last layer written to the image they are carried over until a directive in the first category is met. In actuality, VOLUME seems to be the only second category directive that can make file-system changes. So in that regard it could be considered an exception. If the end of the Dockerfile is reached and there are still file-system changes carried over from VOLUME directives they are discarded—no final layer is written.

Below is an example to demonstrate. The the VOLUME directive changes are combined with file-system changes at the next RUN directive. Notice how that even though RUN echo "foo2" makes no file-system changes it still triggers a layer. This is because the file-system changes of the previous VOLUME directive have been carried over. If we removed VOLUME /volume1 and rebuilt the image RUN echo "foo2" would not trigger a new layer to be written. Also notice that the final VOLUME /volume5 file-system changes are discarded and no layer is added.

Dockerfile:

FROM gcr.io/google-appengine/debian9@sha256:1d6a9a6d106bd795098f60f4abb7083626354fa6735e81743c7f8cfca11259f0
VOLUME /volume1
RUN mkdir /foo1
VOLUME /volume2
VOLUME /volume3
RUN echo "foo2"
VOLUME /volume4
RUN mkdir /foo3
VOLUME /volume5

$ container-diff analyze daemon://kanikotest --type layer
-----FileLayer-----

Analysis for kanikotest Layer 0:
<Truncated for brevity>

Analysis for kanikotest Layer 1:
FILE            SIZE
/foo1           0
/volume1        0

Analysis for kanikotest Layer 2:
FILE            SIZE
/volume2        0
/volume3        0

Analysis for kanikotest Layer 3:
FILE            SIZE
/foo3           0
/volume4        0

priyawadhwa · 2018-09-12T18:05:54Z

Hey @peter-evans, thanks for investigating this! Your findings match what I've found testing locally as well, so I think we can go ahead with these assumptions for now.

Right now, kaniko snapshots in two ways:

The entire filesystem (after RUN commands)
Specific files (ADD/COPY/WORKDIR/VOLUME)

The code for this is here

In the case where VOLUME is followed by one of the commands in (2), you'd need some way of remembering which files were created by VOLUME and appending them to the list of files to be snapshotted here.

Right now, we maintain a whitelist of directories created by volumes, which are added here. We could repurpose this to store volumes that haven't been snapshotted, add these files to snapshotFiles, and then clear out the list after snapshotting.

This, along with a few more integration tests (you'd just have to add a couple test Dockerfiles in the dockerfiles directory, should be good for now.

And again, thanks so much for looking into this!

priyawadhwa · 2018-09-12T18:15:13Z

Another (probably better) option is to remove the volumeWhitelist from the util package entirely, and instead store dirs created by volume in build.go:

// This should live outside the command execution loop
var volumes []string
...
// This check should happen after a command has been executed
if dockerCommand.Name() == "volume" {
   volumes = append(volumes, snapshotFiles...)
   continue
}
...
if snapshotFiles != nil {
        if len(snapshotFiles) > 0  {
            snapshotFiles = append(snapshotFiles, volumes...)
            volumes = []string{}
       }
      contents, err = snapshotter.TakeSnapshot(snapshotFiles)
} else {
        contents, err = snapshotter.TakeSnapshotFS()
        volumes = []string{}
}

Then, before snapshotting specific files you can append the contents of volumes, and then clear out volumes after snapshotting has happened for a different command.

peter-evans · 2018-09-14T01:44:55Z

@priyawadhwa Thanks for the pointers. I'll see what I can do.

priyawadhwa · 2018-09-25T20:35:31Z

pkg/executor/build.go

@@ -167,6 +168,10 @@ func (s *stageBuilder) build(opts *config.KanikoOptions) error {
 			return err
 		}
 		files := command.FilesToSnapshot()
+		if command.Name() == "volume" {


I think cmd.Name() should work here, that way you won't need to add the function to every command.

Let's also add volume to constants.go

Fixed. Thanks!

priyawadhwa · 2018-09-25T20:37:32Z

pkg/util/fs_util.go

+// AddVolumePathToWhitelist adds the given path to the whitelist with
+// PrefixMatchOnly set to true. Snapshotting will ignore paths prefixed
+// with the volume, but the volume itself will not be ignored.
+func AddVolumePathToWhitelist(path string) error {


In what case would we need to whitelist volumes? I'm not totally sure we need to whitelist them at all.

Yes, volumes do need to be whitelisted. This is because any file-system changes inside a volume should not be added to the snapshot. Consider this example:

FROM gcr.io/google-appengine/debian9@sha256:1d6a9a6d106bd795098f60f4abb7083626354fa6735e81743c7f8cfca11259f0 VOLUME /foo RUN echo "hello world" > /foo/hello

After the volume /foo is created any file-system changes within that volume should not be in the snapshot. The file hello will not be in the layer created at the RUN directive. However, the directory foo itself should be included in the snapshot. Due to this complication I felt that the best way to handle it was by being able to flag whitelist entries as PrefixMatchOnly=true. This only allows matches where the whitelisted directory is a prefix of the path being checked. If it's an exact match it will not be whitelisted. This allows the volume directories to be snapshot, but anything inside them to be whitelisted.

After I wrote the above comment I went back to double check. Turns out it's more complicated still. See the following example built with Docker and the container-diff analysis.

FROM gcr.io/google-appengine/debian9@sha256:1d6a9a6d106bd795098f60f4abb7083626354fa6735e81743c7f8cfca11259f0 VOLUME /foo RUN echo “hello world” > /foo/hello COPY run.sh /foo/run.sh VOLUME /bar COPY run.sh /bar/run.sh VOLUME /baz ADD run.sh /baz/run.sh WORKDIR /baz/bat RUN echo “hello world” > /bar/hello RUN echo “hello world” > /baz/hello RUN echo “hello world” > /tmp/hello

Analysis for kaniko-test Layer 1: FILE SIZE /foo 0 Analysis for kaniko-test Layer 2: FILE SIZE /foo 24B /foo/run.sh 24B Analysis for kaniko-test Layer 3: FILE SIZE /bar 24B /bar/run.sh 24B Analysis for kaniko-test Layer 4: FILE SIZE /baz 24B /baz/run.sh 24B Analysis for kaniko-test Layer 5: FILE SIZE /baz 0 /baz/bat 0 Analysis for kaniko-test Layer 6: FILE SIZE /tmp 12B /tmp/hello 12B

File-system changes within volumes are whitelisted during system-wide snapshots. None of the hello files created within volumes appear in the layers. The only hello file that does appear is contained in /tmp, which isn't a volume. However, directives that specify files (ADD/COPY/WORKDIR) override the whitelist.

At the moment, TakeSnapshot(files) is checking the whitelist. I will try removing it and improve the volume integration test to cover these cases.

I removed checking the whitelist for TakeSnapshot(files) and it now works as expected. I added to Dockerfile_test_volume_2 to provide coverage of these specific file directive cases.

peter-evans · 2018-09-26T01:46:24Z

pkg/commands/volume.go

 		if err != nil {
 			return err
 		}

 		// Only create and snapshot the dir if it didn't exist already
 		if _, err := os.Stat(volume); os.IsNotExist(err) {
 			logrus.Infof("Creating directory %s", volume)
-			v.snapshotFiles = []string{volume}
+			v.snapshotFiles = append(v.snapshotFiles, volume)


Just a comment about this change. This is a bug I found where it is not respecting multiple volumes created by the same VOLUME directive.

The Dockerfile_test_volume test did not catch it because tmp is already a directory and so v.snapshotFiles did not get overridden. I added an extra volume for the directive to create to provide test coverage.

VOLUME /foo/bar /tmp /qux/quux

peter-evans · 2018-09-27T02:37:25Z

@priyawadhwa I can see that the kokoro run failed. Any way I can see the detail for that?
I've run all the integration tests on a clean VM and they all passed so I'm not sure what could be wrong.

priyawadhwa · 2018-09-27T17:42:21Z

@peter-evans looks like the number of layers was incorrect:

    --- FAIL: TestLayers/test_layer_Dockerfile_test_volume (13.26s)
    	integration_test.go:233: Difference in number of layers in each image is 1 but should be 0. Image 1: Image: [gcr.io/kaniko-test/docker-dockerfile_test_volume] Digest: [e76e94ae181e524b80f45d8b43f6cf7eee04bb50b1bfd9bddd07c63449a73145] Number of Layers: [4], Image 2: Image: [gcr.io/kaniko-test/kaniko-dockerfile_test_volume] Digest: [0482d5821245a91e6cb5aca60a50bd5658e7f6644365583bd93031513138f3d9] Number of Layers: [5]

peter-evans · 2018-09-28T01:35:50Z

@priyawadhwa I just spun up a new VM in GCP and ran the integration tests again and they all pass.
Please see the log here: kaniko-integration-test.log

Ubuntu 16.04.5 LTS
Docker 18.06.1-ce, build e68fc7a
go version go1.11 linux/amd64

It would be great if you could let me know some detail about kokoro's build environment, such as distro, Docker version, etc.

peter-evans · 2018-09-28T02:12:53Z

I also checked the layers in kaniko-dockerfile_test_volume and it correctly returns 4, not 5. Is kokoro definitely building the correct branch? I wonder if it got confused because I squashed the commits.

$ container-diff analyze asia.gcr.io/cloud-shell-215704/kaniko-dockerfile_test_volume --type=layer

-----FileLayer-----

Analysis for asia.gcr.io/cloud-shell-215704/kaniko-dockerfile_test_volume Layer 0:
<Truncated for brevity>

Analysis for asia.gcr.io/cloud-shell-215704/kaniko-dockerfile_test_volume Layer 1:
FILE        SIZE
/foo        0

Analysis for asia.gcr.io/cloud-shell-215704/kaniko-dockerfile_test_volume Layer 2:
FILE            SIZE
/foo            6B
/foo/hey        6B

Analysis for asia.gcr.io/cloud-shell-215704/kaniko-dockerfile_test_volume Layer 3:
FILE             SIZE
/baz             0
/baz/bat         0
/foo             0
/foo/bar         0
/qux             0
/qux/quux        0
/tmp             0

peter-evans · 2018-09-28T02:43:14Z

I rebased onto master and ran the integration tests again. All pass.

priyawadhwa

Cool, likes like all tests are passing now, just had one question.

priyawadhwa · 2018-09-28T22:17:57Z

pkg/snapshot/snapshot.go

-			return false, fmt.Errorf("Error checking if parent dir %s can be snapshotted: %s", file, err)
-		}
-		if !shouldSnapshot {
+		if val, ok := snapshottedFiles[file]; ok && val {


Why did you remove shouldSnapshot() and isBuildFile()? I think those functions are necessary so that kaniko is able to build itself.

For the specific files cases (ADD, COPY, WORKDIR) that call snapshotFiles() it is correct that they are added to the snapshot even if one of their parent directories is a volume. That is the main reason I removed checking the whitelist, which is what shouldSnapshot() was doing. Now that we aren't checking the whitelist there is no need for isBuildFile() either. That function's job was to make sure that build files that were whitelisted were added to the snapshot anyway.

The key thing to consider about this change is, is it ok to not check the whitelist at all for specific files cases, directives ADD, COPY and WORKDIR? (VOLUME is included too, but only if it's snapshot with another specific file case and not a system-wide snapshot.) Up to now, Kaniko has prevented adding whitelisted files and directories with those directives. It doesn't seem to be necessary to me. The main purpose of the whitelist seems to be to ignore files and directories when there is no way to know if the file was mounted or came from the base image. If a Dockerfile is explicitly trying to add files and directories with ADD, COPY and WORKDIR, I don't think there is a reason to prevent that. Let me know if you think I've missed something!

@peter-evans that makes sense, thanks for explaining!

priyawadhwa

LGTM, thanks so much for looking into this and fixing this bug!

container-tools-bot added size/XS needs-ok-to-test labels Sep 4, 2018

priyawadhwa added the kokoro:run label Sep 4, 2018

kokoro-team removed the kokoro:run label Sep 4, 2018

priyawadhwa reviewed Sep 6, 2018

View reviewed changes

priyawadhwa added the kokoro:run label Sep 7, 2018

kokoro-team removed the kokoro:run label Sep 7, 2018

peter-evans force-pushed the fix-volume-cmd branch from 1c1d3c9 to 5cac31e Compare September 25, 2018 08:39

container-tools-bot added size/L and removed size/XS labels Sep 25, 2018

priyawadhwa reviewed Sep 25, 2018

View reviewed changes

peter-evans force-pushed the fix-volume-cmd branch from 5cac31e to ab4b65e Compare September 26, 2018 01:23

peter-evans changed the title ~~Stop adding volume directories to the snapshot~~ Fix handling of the volume directive Sep 26, 2018

peter-evans commented Sep 26, 2018

View reviewed changes

peter-evans force-pushed the fix-volume-cmd branch from ab4b65e to 046b3ac Compare September 26, 2018 05:00

priyawadhwa added the kokoro:run label Sep 27, 2018

kokoro-team removed the kokoro:run label Sep 27, 2018

Fix handling of volume directive

b1e28dd

peter-evans force-pushed the fix-volume-cmd branch from 046b3ac to b1e28dd Compare September 28, 2018 02:16

priyawadhwa added the kokoro:run label Sep 28, 2018

kokoro-team removed the kokoro:run label Sep 28, 2018

priyawadhwa reviewed Sep 28, 2018

View reviewed changes

priyawadhwa approved these changes Oct 1, 2018

View reviewed changes

priyawadhwa merged commit 8f0d257 into GoogleContainerTools:master Oct 1, 2018

peter-evans deleted the fix-volume-cmd branch October 9, 2018 00:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of the volume directive #334

Fix handling of the volume directive #334

peter-evans commented Sep 4, 2018

container-tools-bot commented Sep 4, 2018

peter-evans commented Sep 4, 2018

priyawadhwa left a comment

peter-evans commented Sep 7, 2018

priyawadhwa commented Sep 7, 2018

peter-evans commented Sep 8, 2018

peter-evans commented Sep 12, 2018

priyawadhwa commented Sep 12, 2018

priyawadhwa commented Sep 12, 2018

peter-evans commented Sep 14, 2018

priyawadhwa Sep 25, 2018

peter-evans Sep 26, 2018

priyawadhwa Sep 25, 2018

peter-evans Sep 26, 2018

peter-evans Sep 26, 2018

peter-evans Sep 26, 2018

peter-evans Sep 26, 2018

peter-evans commented Sep 27, 2018

priyawadhwa commented Sep 27, 2018

peter-evans commented Sep 28, 2018

peter-evans commented Sep 28, 2018

peter-evans commented Sep 28, 2018

priyawadhwa left a comment

priyawadhwa Sep 28, 2018

peter-evans Sep 29, 2018

priyawadhwa Oct 1, 2018

priyawadhwa left a comment

Fix handling of the volume directive #334

Fix handling of the volume directive #334

Conversation

peter-evans commented Sep 4, 2018

container-tools-bot commented Sep 4, 2018

peter-evans commented Sep 4, 2018

priyawadhwa left a comment

Choose a reason for hiding this comment

peter-evans commented Sep 7, 2018

priyawadhwa commented Sep 7, 2018

peter-evans commented Sep 8, 2018

peter-evans commented Sep 12, 2018

priyawadhwa commented Sep 12, 2018

priyawadhwa commented Sep 12, 2018

peter-evans commented Sep 14, 2018

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

peter-evans commented Sep 27, 2018

priyawadhwa commented Sep 27, 2018

peter-evans commented Sep 28, 2018

peter-evans commented Sep 28, 2018

peter-evans commented Sep 28, 2018

priyawadhwa left a 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

priyawadhwa left a comment

Choose a reason for hiding this comment