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

Add FileOp to LLB #396

Closed
tonistiigi opened this issue May 21, 2018 · 10 comments · Fixed by #809
Closed

Add FileOp to LLB #396

tonistiigi opened this issue May 21, 2018 · 10 comments · Fixed by #809

Comments

@tonistiigi
Copy link
Member

tonistiigi commented May 21, 2018

LLB doesn't currently support copying files directly and Dockerfile ADD/COPY uses a copy binary executed through exec. While this works fine for caching, it isn't very optimal and could be better with a specific operation implementation. It has also appeared that some of the edge cases are not very easy to configure using the special binary.

Most of the implementation for efficient copy procedure is already in tonistiigi/fsutil .

Proposal for the definition:

message FileOp {
	repeated FileTarget = 1;
	repeated FileAction = 2;
}

message FileTarget {
	int64 input = 1;
	int64 output = 2;
}

message FileAction {
	int64 input = 1 [(gogoproto.customtype) = "InputIndex", (gogoproto.nullable) = false]; // could be real input or target (target index + max input index)
	int64 target = 2; // index from target array
	oneof action {
		FileActionCopy copy = 3;
		FileActionMkFile mkfile = 4;
		FileActionMkDir mkdir = 5;
		FileActionRm rm = 6;
	}
}

message FileActionCopy {
	string src = 1;
	string dest = 2;
	ChownOpt owner = 4;
        int mode = 5 [(gogoproto.nullable) = true];
	bool followSymlink = 6;
	bool dirCopyContents = 7;
        bool attemptUnpack = 8;
        bool createDestPath = 9;
}

message FileMkFile {
	string path = 1;
	int mode = 2;
	bytes data = 3;
	ChownOpt owner = 4;
}

message FileMkDir {
	string path = 1;
	int mode = 2;
	ChownOpt owner = 4;
}

message FileActionRm {
	string path = 1;
}

message ChownOpt {
	UserOpt user = 1;
	UserOpt group = 2;
}

message UserOpt {
	string name = 1;
	int64 input = 2; // input that contains /etc/passwd if using a name
	int id = 3; 
}

@AkihiroSuda

@AkihiroSuda
Copy link
Member

SGTM

Maybe we should add xattr stuff later.

@tonistiigi
Copy link
Member Author

@AkihiroSuda As a way to override like chown? Maybe optional modtime override as well. Could be useful for reproducible timestamps.

@ijc
Copy link
Collaborator

ijc commented May 21, 2018

As it happens I was just looking at the use of various copy helpers in my projects and this looks like a good/useful replacement AFAICT.

Is the intention to completely replace the existing (unimplemented?) CopyOp?

@tonistiigi
Copy link
Member Author

Yes, this is same as previously discussed CopyOp, just made it more generic to handle more possible cases.

@ijc
Copy link
Collaborator

ijc commented Jul 11, 2018

Couple of random things which would be useful in this:

I have a usecase for FileActionCopy where it is useful to know specifically that a failure was due to the source not being present (essentially looking for optional files). I'm not sure if ENOENT in a specific node in the graph can reliably be detected/reported at the Solve layer (if there were several similar operations in an llb graph it might not be easy to distinguish without tricks). I currently handle this with a Run of if [ -e $src ] ; then cp $src $dst ; fi, then with a local export I infer the presence/absence of $src from $dst's presence. Having a ignoreMissingSource on FileActionCopy would be the obvious extension of that mechanism, but maybe there is a different/better way to achieve my goal overall?

Does FileMkDir create all parent dirs too, should that be a flag?

@tonistiigi
Copy link
Member Author

Is your case more about getting the error or just ignoring the error if the source file does not exist? Eg. would bool skipIfNotFound help in that case? Also, when we add

// StatFile(ctx context.Context, req StatRequest) (*StatResponse, error)
// ReadDir(ctx context.Context, req ReadDirRequest) ([]*StatResponse, error)
, would that be a better approach for your problem.

Does FileMkDir create all parent dirs too, should that be a flag?

It should either create parents automatically or be a flag.

@ijc
Copy link
Collaborator

ijc commented Jul 12, 2018

Is your case more about getting the error or just ignoring the error if the source file does not exist?

I need to probe for whether a specific file exists or not, so I need to distinguish (somehow) between that file not existing and some other error which might happen while solving.

However:

// StatFile(ctx context.Context, req StatRequest) (*StatResponse, error)
// ReadDir(ctx context.Context, req ReadDirRequest) ([]*StatResponse, error)

Seem like a much (much!) better way to address my use case! (once I address what is being discussed in #472). Thanks!

It should either create parents automatically or be a flag.

I agree (personally I'd go for a flag, because I've been trained by mkdir -p, but have no strong feelings or rationale).

@rpatrick00
Copy link

Personally, I think the default behavior when running a Docker build on Windows (for a Linux container) should be that directories are always copied with execute permission. Running "find . -type d -exec chmod +x {} ;" is unnecessary when running the same build from a Linux box but is required when running the build on Windows.

@tonistiigi
Copy link
Member Author

#809

@tonistiigi tonistiigi added this to the v0.5.0 milestone Feb 1, 2019
@thaJeztah
Copy link
Member

@ijc @tonistiigi regarding COPY -p / COPY --parents - There's an earlier discussion about that in moby/moby#35639 (and recently: moby/moby#38710 (moby/moby#38710 (comment)))

Looks like there's some subtle things in -p / --parents w.r.t. keeping / overwriting ownership; if we implement that option we should look carefully to implementing it the same (perhaps there's a design/spec somewhere, but haven't searched for that yet)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants