-
Notifications
You must be signed in to change notification settings - Fork 95
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
ociruntime: reapply tar unpack #8023
Conversation
e8794bf
to
e8e3285
Compare
Also tested some public images that our users used and did not find any issues. |
@@ -102,6 +102,72 @@ go_test( | |||
], | |||
) | |||
|
|||
# keep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we have a few of these types of targets elsewhere but they are a bit of a hassle to maintain because gazelle doesn't handle them properly. I think it would be fine to use a flag (instead of env var) to guard the test, and then people could run it by passing the flag to the "normal" test target, like bazel test enterprise/server/remote_execution/containers/ociruntime:ociruntime_test --config=remote --test_arg=-test_pull=true
(optionally adding --test_filter=TestPullImage
to run just that test case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix
return status.UnavailableErrorf("create directory: %s", err) | ||
} | ||
case tar.TypeReg: | ||
if err := os.MkdirAll(dir, os.ModePerm); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to make the parent dir for TypeSymlink and TypeLink as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I can add the create parent dir to all known cases
enterprise/server/remote_execution/containers/ociruntime/ociruntime.go
Outdated
Show resolved
Hide resolved
if err := os.Link(source, target); err != nil { | ||
return status.UnavailableErrorf("create hard link: %s", err) | ||
} | ||
if err := os.Chown(target, header.Uid, header.Gid); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand hardlinks, this would change the metadata of the target, which is shared between all hardlinks. Since we ensure that the target is created by this tar and thus could have had a mode set, should we skip it here to prevent flip-flopping? I don't know how tar handles this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will remove this
enterprise/server/remote_execution/containers/ociruntime/ociruntime.go
Outdated
Show resolved
Hide resolved
e8e3285
to
bfe7051
Compare
bfe7051
to
24962b0
Compare
This change include a few small changes inside:
Reapply "ociruntime: replace tar extraction subprocess with in-process (ociruntime: replace tar extraction subprocess with in-process #7589)" (Revert "ociruntime: replace tar extraction subprocess with in-process (#7589)" #8019)
Add a basic image pull test and guard it with a separate, manual Bazel test target to help verifying common images.
Fix issues with the previous implementation.
Ensure that parent directory is created for file tar entries.
Ignore unknown tar entry types (i.e. devices).