Skip to content
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.

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

WIP: commit: optionally create layers for ADD/COPY/RUN/WORKDIR #2547

Closed
wants to merge 1 commit into from

Conversation

nalind
Copy link
Member

@nalind nalind commented Aug 13, 2020

What type of PR is this?

/kind bug

What this PR does / why we need it:

In multi-layer mode, only add layers when ADD, COPY, RUN, or WORKDIR make changes to the container's rootfs.

How to verify it

Building an image using --layers=true and a Dockerfile containing

FROM fedora
WORKDIR /testdir

should produce an image with two layers, rather than just the base image's single layer.

Conformance tests have been extended to compare how many layers we generate in comparison to docker build, though it makes exceptions for items we create as targets for bind mounts when handling RUN instructions.

Which issue(s) this PR fixes:

Fixes #2402

Special notes for your reviewer:

The workdir-in-image-owner and workdir-not-in-image conformance tests should exercise this.

Does this PR introduce a user-facing change?

Instead of always creating new layers for ADD, COPY, and RUN instructions, `buildah build-using-dockerfile --layers` will now avoid adding new layers when those instructions make no changes to the filesystem.

@nalind nalind mentioned this pull request Aug 13, 2020
@rhatdan
Copy link
Member

rhatdan commented Aug 13, 2020

What happens if I have

FROM fedora
WORKDIR /home/dwalsh
MAINTAINER dan walsh

@nalind
Copy link
Member Author

nalind commented Aug 13, 2020

That doesn't work right. Moving this to a draft PR.

@nalind nalind marked this pull request as draft August 13, 2020 18:11
@openshift-ci-robot
Copy link
Collaborator

@nalind: PR needs rebase.

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.

@openshift-ci-robot
Copy link
Collaborator

@nalind: PR needs rebase.

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.

@rhatdan
Copy link
Member

rhatdan commented Sep 16, 2020

@nalind Any movement on this one? Needs a rebase.

@rhatdan
Copy link
Member

rhatdan commented Oct 7, 2020

@nalind Maybe we should just get this merged since it closes one of the cases. Then we can continue to work on the corner cases.

@nalind nalind force-pushed the conformance-workdir branch from 49a35fb to 5ec97be Compare November 6, 2020 21:42
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nalind

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:

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

@nalind
Copy link
Member Author

nalind commented Nov 6, 2020

/retitle commit: optionally create layers for ADD/COPY/RUN/WORKDIR

@openshift-ci-robot openshift-ci-robot changed the title conformance: make WORKDIR create a layer if it's last commit: optionally create layers for ADD/COPY/RUN/WORKDIR Nov 6, 2020
@nalind
Copy link
Member Author

nalind commented Nov 6, 2020

I expect conformance tests to fail on overlay because resetting the contents of a directory causes us to mark it as opaque until #2756 changes how we do things for those cases.

@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Feb 18, 2021

@nalind Is this still being worked on or can we close it?

@nalind
Copy link
Member Author

nalind commented Feb 22, 2021

We still need this.

@rhatdan
Copy link
Member

rhatdan commented Feb 22, 2021

@nalind can you rebase and repush?

@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Mar 25, 2021

@nalind rebase and repush.

@nalind nalind force-pushed the conformance-workdir branch from 5ec97be to ea6310c Compare March 25, 2021 14:16
Change our strategy for creating layers from:
 * Always create layers for ADD, COPY, and RUN
to:
 * Create layers for ADD, COPY, RUN, and WORKDIR if
   they made changes to the filesystem.  This includes
   creating the WORKDIR directory if it doesn't already
   exist.

It's often a subtle difference, but it fixes cases where a WORKDIR
instruction wasn't followed by RUN, so we didn't end up accidentally
creating it ourselves.

We're not perfect here, because our RUN creates targets for bind mounts
that `docker build` avoids, but we're closer than we were before.

Signed-off-by: Nalin Dahyabhai <[email protected]>
@nalind nalind force-pushed the conformance-workdir branch from ea6310c to fc0670d Compare March 25, 2021 16:57
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 18, 2021

@nalind: PR needs rebase.

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.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2021
@rhatdan rhatdan removed the stale-pr label Aug 19, 2021
@rhatdan
Copy link
Member

rhatdan commented Aug 19, 2021

@nalind this oldy-but-goody PR needs a rebase.

@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Sep 20, 2021

@nalind what is going on with this PR?

@rhatdan rhatdan removed the stale-pr label Sep 20, 2021
@nalind
Copy link
Member Author

nalind commented Sep 20, 2021

A simple rebase isn't going to be enough here - it didn't pass all of the tests before it needed that. Re-adding the WIP label.

@nalind nalind changed the title commit: optionally create layers for ADD/COPY/RUN/WORKDIR WIP: commit: optionally create layers for ADD/COPY/RUN/WORKDIR Sep 20, 2021
@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@github-actions github-actions bot closed this Oct 21, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

buildah bud --layers=false / does not handle WORKDIR correctly.
4 participants