-
Notifications
You must be signed in to change notification settings - Fork 92
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: replace tar extraction subprocess with in-process #7589
base: master
Are you sure you want to change the base?
Conversation
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.
I was holding off on this TODO because I wanted to make sure the performance was on-par. Any chance you could run some quick tests with larger images with several layers (on a fast network connection, clearing cache between runs) to make sure it's not significantly slower and CPU usage is not significantly higher?
I applied this patch on top of master
Then ran it with
Repeated the same test with this PR on top. I think the Go approach could be slightly slower and higher memory vs pipe to |
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.
LGTM assuming the TestFileOwnership
test passes after you rebase (we aren't passing --no-same-owner
anymore)
9a44e12
to
2528197
Compare
enterprise/server/remote_execution/containers/ociruntime/ociruntime.go
Outdated
Show resolved
Hide resolved
7166367
to
f56bf91
Compare
enterprise/server/remote_execution/containers/ociruntime/ociruntime.go
Outdated
Show resolved
Hide resolved
f56bf91
to
e9c897e
Compare
I learned about https://github.com/google/safeopen/blob/main/safeopen_linux.go over the weekend as well as golang/go#67002 (comment) (coming to Go 1.24). But I think these could be added later in a different PR? |
Address the TODO comment.