Skip to content

integration-cli: remove emptyfs #22

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.

Sign up for GitHub

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

Closed
vvoland wants to merge 1 commit into from
Closed

integration-cli: remove emptyfs #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions Dockerfile.e2e
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ ENTRYPOINT ["/scripts/run.sh"]
# Add an unprivileged user to be used for tests which need it
RUN addgroup docker && adduser -D -G docker unprivilegeduser -s /bin/ash

# GNU tar is used for generating the emptyfs image
RUN apk --no-cache add \
bash \
ca-certificates \
Expand All @@ -69,11 +68,9 @@ RUN apk --no-cache add \
iptables \
libcap2-bin \
pigz \
tar \
xz

COPY hack/test/e2e-run.sh /scripts/run.sh
COPY hack/make/.ensure-emptyfs /scripts/ensure-emptyfs.sh

COPY integration/testdata /tests/integration/testdata
COPY integration/build/testdata /tests/integration/build/testdata
Expand Down
23 changes: 0 additions & 23 deletions hack/make/.ensure-emptyfs
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions hack/make/.integration-daemon-setup
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,3 @@
set -e

source "$MAKEDIR/.detect-daemon-osarch"
if [ "$DOCKER_ENGINE_GOOS" != "windows" ]; then
bundle .ensure-emptyfs
fi
1 change: 0 additions & 1 deletion hack/test/e2e-run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,5 +81,4 @@ set_platform_timeout() {
fi
}

sh /scripts/ensure-emptyfs.sh
run_test_integration
18 changes: 2 additions & 16 deletions integration-cli/docker_cli_inspect_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,6 @@ func checkValidGraphDriver(c *testing.T, name string) {
}
}

func (s *DockerCLIInspectSuite) TestInspectImage(c *testing.T) {
testRequires(c, DaemonIsLinux)
imageTest := "emptyfs"
// It is important that this ID remain stable. If a code change causes
// it to be different, this is equivalent to a cache bust when pulling
// a legacy-format manifest. If the check at the end of this function
// fails, fix the difference in the image serialization instead of
// updating this hash.
imageTestID := "sha256:11f64303f0f7ffdc71f001788132bca5346831939a956e3e975c93267d89a16d"
id := inspectField(c, imageTest, "Id")
Comment on lines -39 to -45
Copy link
Copy Markdown
Collaborator

@thaJeztah thaJeztah Jul 20, 2022

Choose a reason for hiding this comment

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

Yeah, agreed, this is the only bit that somewhat concerns me. Not sure what the exact impact would be (and if it matters). Perhaps @tianon or @tonistiigi have a better estimation to see if it's important.

Copy link
Copy Markdown

@tianon tianon Jul 20, 2022

Choose a reason for hiding this comment

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

I believe the intent of this test was to validate that the full round-tripping from a pre-existing tarball (not created by the code-under-test) worked correctly and as expected, and that we didn't accidentally change any of the image configuration in the process, which I think is probably still valuable? Not entirely sure.

My "gut" reaction would be that we should have docker load still import the potentially non-platform-matching image (since that's what users expect from doing that on engines today), and that this test should instead be conditional on the containerd-snapshotter flag (switching from this older ID to whatever the new ID is) -- perhaps even changing the test to not only validate the manifest in that case, but to also then query containerd for the config blob checksum (which would still match this older sha256)?

Definitely on-board with changing all the other tests to no longer use emptyfs though. 👍


assert.Equal(c, id, imageTestID)
}

func (s *DockerCLIInspectSuite) TestInspectInt64(c *testing.T) {
dockerCmd(c, "run", "-d", "-m=300M", "--name", "inspectTest", "busybox", "true")
inspectOut := inspectField(c, "inspectTest", "HostConfig.Memory")
Expand Down Expand Up @@ -137,7 +123,7 @@ func (s *DockerCLIInspectSuite) TestInspectTypeFlagWithInvalidValue(c *testing.T

func (s *DockerCLIInspectSuite) TestInspectImageFilterInt(c *testing.T) {
testRequires(c, DaemonIsLinux)
imageTest := "emptyfs"
imageTest := "busybox"
out := inspectField(c, imageTest, "Size")

size, err := strconv.Atoi(out)
Expand Down Expand Up @@ -175,7 +161,7 @@ func (s *DockerCLIInspectSuite) TestInspectContainerFilterInt(c *testing.T) {

func (s *DockerCLIInspectSuite) TestInspectImageGraphDriver(c *testing.T) {
testRequires(c, DaemonIsLinux, Devicemapper)
imageTest := "emptyfs"
imageTest := "busybox"
name := inspectField(c, imageTest, "GraphDriver.Name")

checkValidGraphDriver(c, name)
Expand Down
6 changes: 1 addition & 5 deletions integration-cli/docker_cli_run_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1945,11 +1945,7 @@ func (s *DockerCLIRunSuite) TestRunCidFileCleanupIfEmpty(c *testing.T) {
defer os.RemoveAll(tmpDir)
tmpCidFile := path.Join(tmpDir, "cid")

image := "emptyfs"
if testEnv.OSType == "windows" {
// Windows can't support an emptyfs image. Just use the regular Windows image
image = testEnv.PlatformDefaults.BaseImage
}
image := testEnv.PlatformDefaults.BaseImage
out, _, err := dockerCmdWithError("run", "--cidfile", tmpCidFile, image)
if err == nil {
c.Fatalf("Run without command must fail. out=%s", out)
Expand Down
6 changes: 3 additions & 3 deletions integration-cli/docker_cli_save_load_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ func (s *DockerCLISaveLoadSuite) TestSaveCheckTimes(c *testing.T) {
func (s *DockerCLISaveLoadSuite) TestSaveImageId(c *testing.T) {
testRequires(c, DaemonIsLinux)
repoName := "foobar-save-image-id-test"
dockerCmd(c, "tag", "emptyfs:latest", fmt.Sprintf("%v:latest", repoName))
dockerCmd(c, "tag", "busybox:latest", fmt.Sprintf("%v:latest", repoName))

out, _ := dockerCmd(c, "images", "-q", "--no-trunc", repoName)
cleanedLongImageID := strings.TrimPrefix(strings.TrimSpace(out), "sha256:")
Expand Down Expand Up @@ -202,10 +202,10 @@ func (s *DockerCLISaveLoadSuite) TestSaveMultipleNames(c *testing.T) {
repoName := "foobar-save-multi-name-test"

// Make one image
dockerCmd(c, "tag", "emptyfs:latest", fmt.Sprintf("%v-one:latest", repoName))
dockerCmd(c, "tag", "busybox:latest", fmt.Sprintf("%v-one:latest", repoName))

// Make two images
dockerCmd(c, "tag", "emptyfs:latest", fmt.Sprintf("%v-two:latest", repoName))
dockerCmd(c, "tag", "busybox:latest", fmt.Sprintf("%v-two:latest", repoName))

out, err := RunCommandPipelineWithOutput(
exec.Command(dockerBinary, "save", fmt.Sprintf("%v-one", repoName), fmt.Sprintf("%v-two:latest", repoName)),
Expand Down