Add missing params for podman-remote build #9275

rhatdan · 2021-02-08T20:33:06Z

Currently we still have hard coded --isolation=chroot for podman-remote build.

Implement missing arguments for podman build

Implements
--jobs, --disable-compression, --excludes

Fixes:
MaxPullPushRetries
RetryDuration

Signed-off-by: Daniel J Walsh [email protected]

openshift-ci-robot · 2021-02-08T20:33:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

edsantiago · 2021-02-08T20:36:54Z

test/system/070-build.bats

+FROM docker.io/library/busybox AS build
+RUN rm -f /bin/paste
+USER 1001
+COPY --from=docker.io/library/busybox /bin/paste /test/


Could I encourage you to s;docker.io/library/;quay.io/libpod/; ?

Yup cut and pasted from Buildah.

rhatdan · 2021-02-09T19:28:25Z

Fixes: #9290

TomSweeneyRedHat · 2021-02-09T21:57:00Z

The fedora-33 test failed during an update of ubutnu. I've hit re-run.

TomSweeneyRedHat · 2021-02-09T22:03:30Z

pkg/api/handlers/compat/images_build.go

+		CpuQuota               int64  `schema:"cpuquota"`    // nolint
+		CpuSetCpus             string `schema:"cpusetcpus"`  // nolint
+		CpuShares              uint64 `schema:"cpushares"`   // nolint
+		Compression            uint64 `schema:"compression"` // nolint


nit Compression before ConfigureNetwork

TomSweeneyRedHat · 2021-02-09T22:06:02Z

One nit of a nit, otherwise
LGTM

vrothberg · 2021-02-10T06:51:26Z

Restarted flakes

rhatdan · 2021-02-10T13:22:24Z

I think the compose ones are real. For some reason when the Docker API is used, the network is not being setup correctly.

Fixes: containers#9290 Currently we still have hard coded --isolation=chroot for podman-remote build. Implement missing arguments for podman build Implements --jobs, --disable-compression, --excludes Fixes: MaxPullPushRetries RetryDuration Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2021-02-21T15:50:51Z

@containers/podman-maintainers PTAL
@vrothberg @giuseppe @mheon PTAL

giuseppe · 2021-02-22T08:50:53Z

pkg/api/handlers/compat/images_build.go

 	if utils.IsLibpodRequest(r) {
+		//		isolation = buildah.Isolation(query.Isolation)


should we drop this commented code?

Lets merge this. I waiting to open a PR to fix this as soon as this PR merges.

vrothberg

/lgtm

edsantiago · 2021-02-24T19:45:22Z

test/system/070-build.bats

+@test "build with copy-from referencing the base image" {
+  skip_if_rootless "cannot mount as rootless"
+  target=busybox-derived
+  target_mt=busybox-mt-derived
+  tmpdir=$PODMAN_TMPDIR/build-test
+  mkdir -p $tmpdir
+  containerfile1=$tmpdir/Containerfile1
+    cat >$containerfile1 <<EOF
+FROM quay.io/libpod/busybox AS build
+RUN rm -f /bin/paste
+USER 1001
+COPY --from=quay.io/libpod/busybox /bin/paste /test/
+EOF
+  containerfile2=$tmpdir/Containerfile2
+    cat >$containerfile2 <<EOF
+FROM quay.io/libpod/busybox AS test
+RUN rm -f /bin/nl
+FROM quay.io/libpod/alpine AS final
+COPY --from=quay.io/libpod/busybox /bin/nl /test/
+EOF
+  run_podman build -t ${target} -f ${containerfile1} ${tmpdir}
+  run_podman build --jobs 4 -t ${target} -f ${containerfile1} ${tmpdir}
+
+  run_podman build -t ${target} -f ${containerfile2} ${tmpdir}
+  run_podman build --no-cache --jobs 4 -t ${target_mt} -f ${containerfile2} ${tmpdir}
+
+  # (can only test locally; podman-remote has no image mount command)
+  if ! is_remote; then
+    run_podman image mount ${target}
+    root_single_job=$output
+
+    run_podman image mount ${target_mt}
+    root_multi_job=$output
+
+    # Check that both the version with --jobs 1 and --jobs=N have the same number of files
+    test $(find $root_single_job -type f | wc -l) = $(find $root_multi_job -type f | wc -l)
+  fi
+}
+


@rhatdan @nalind I realize this is already merged, but I'm afraid this test is almost entirely a NOP. I've spent the last hour-plus trying to clean it up, and am forced to give up because I don't truly understand its purpose. Is it to test the --jobs option? To test COPY --from? I'm going to need to rewrite the whole thing, and I would be grateful for any hints on how to do so.

The basic idea, I believe since @nalind wrote it for Buildah, was to run the same build twice once with multiple --jobs and make sure that the output in the end was the same.

Thank you! In that case, are lines 476-477 (the first two builds) needed? Or for that matter containerfile1?

@nalind WDYT?

Rereading the tests, I stole this from, I am now not sure what it is testing.

The test was originally to ensure that when COPY --from was given an image name rather than a name we'd given to a build stage, that it would pull content from the unmodified image rather than a stage that was based on it, which was a bug that we'd fixed.
The parts that ran builds both with and without --jobs and counted the files in containers based on the resulting images, to check that the results from both were comparable, were mixed in later rather than added as a separate test.

- cp test: clean up stray image - build test: add workaround for containers#9567 (ultra-slow ubuntu). We're seeing CI flakes (timeouts) due to ubuntu 2004 being absurdly slow. Workaround: double our timeout on one specific test when ubuntu + remote. - build test: clean up new copy-from test (from containers#9275). The test was copy-pasted from buildah system tests, without really adapting for podman environment (e.g. it was using images that we don't use here, and would cause pulls, which will cause flakes). Rewrite test so it references only $IMAGE, remove some confusing/unnecessary stuff, selectively run parts of it even when rootless or remote, and add a test to confirm that copy-from succeeded. - load test: add error-message test to new load-invalid (containers#9672). Basically, make sure the command fails for the right reason. - play test (kube): use $IMAGE, not alpine; and add pause-image cleanup to teardown() - apiv2 mounts test: add a maintainability comment in a tricky section of code; and tighten up the mount point test. Signed-off-by: Ed Santiago <[email protected]>

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2021

edsantiago reviewed Feb 8, 2021

View reviewed changes

rhatdan force-pushed the build branch 2 times, most recently from 48f44c3 to a3a83ed Compare February 9, 2021 15:56

rhatdan added the Backport to v3.0 label Feb 9, 2021

rhatdan force-pushed the build branch 2 times, most recently from 3c24c52 to 5b7e061 Compare February 9, 2021 19:27

rhatdan force-pushed the build branch from 5b7e061 to be19f5b Compare February 9, 2021 21:22

TomSweeneyRedHat reviewed Feb 9, 2021

View reviewed changes

rhatdan force-pushed the build branch from be19f5b to 9930c6d Compare February 9, 2021 23:45

rhatdan force-pushed the build branch 4 times, most recently from dffd1e7 to 3b07b9f Compare February 13, 2021 11:52

rhatdan changed the title ~~Use confined OCI by default for builds~~ Add missing params for podman-remote build Feb 13, 2021

rhatdan force-pushed the build branch 3 times, most recently from 4aa9c61 to 50f415c Compare February 16, 2021 09:53

rhatdan force-pushed the build branch from 50f415c to 690c02f Compare February 16, 2021 11:48

giuseppe reviewed Feb 22, 2021

View reviewed changes

vrothberg reviewed Feb 22, 2021

View reviewed changes

openshift-ci-robot assigned vrothberg Feb 22, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2021

openshift-merge-robot merged commit 10d52c0 into containers:master Feb 22, 2021

edsantiago mentioned this pull request Feb 22, 2021

podman-remote network create: race condition creating conflist file #9472

Closed

edsantiago reviewed Feb 24, 2021

View reviewed changes

edsantiago mentioned this pull request Mar 15, 2021

System test cleanup #9719

Merged

edsantiago mentioned this pull request Mar 24, 2021

CI flake: Podman run/create two containers with the same IP #7096

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing params for podman-remote build #9275

Add missing params for podman-remote build #9275

rhatdan commented Feb 8, 2021 •

edited

Loading

openshift-ci-robot commented Feb 8, 2021

edsantiago Feb 8, 2021

rhatdan Feb 8, 2021

rhatdan commented Feb 9, 2021

TomSweeneyRedHat commented Feb 9, 2021

TomSweeneyRedHat Feb 9, 2021

TomSweeneyRedHat commented Feb 9, 2021 •

edited

Loading

vrothberg commented Feb 10, 2021

rhatdan commented Feb 10, 2021

rhatdan commented Feb 21, 2021

giuseppe Feb 22, 2021

rhatdan Feb 22, 2021

vrothberg left a comment

edsantiago Feb 24, 2021

rhatdan Feb 26, 2021

edsantiago Feb 26, 2021

rhatdan Mar 1, 2021

nalind Mar 1, 2021

		if utils.IsLibpodRequest(r) {
		// isolation = buildah.Isolation(query.Isolation)

Add missing params for podman-remote build #9275

Add missing params for podman-remote build #9275

Conversation

rhatdan commented Feb 8, 2021 • edited Loading

openshift-ci-robot commented Feb 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Feb 9, 2021

TomSweeneyRedHat commented Feb 9, 2021

Choose a reason for hiding this comment

TomSweeneyRedHat commented Feb 9, 2021 • edited Loading

vrothberg commented Feb 10, 2021

rhatdan commented Feb 10, 2021

rhatdan commented Feb 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Feb 8, 2021 •

edited

Loading

TomSweeneyRedHat commented Feb 9, 2021 •

edited

Loading