-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Artifact add optimization on macOS and Windows #27426
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
base: main
Are you sure you want to change the base?
Conversation
0b018d8 to
28a90c6
Compare
|
Quick breeze, it LGTM, and happy green test buttons to boot! |
|
really quick pass LGTM ... hopefully someone can walk through a little slower than today allowed. |
28a90c6 to
7a6d65e
Compare
|
Based on benchmark results, I disabled this improvement for WSL. I rebenchmarked this improvement on Windows 11 and it didn't get better, but overall CPU usage is lower. Let me know if you think disabling this feature for WSL makes sense. PTAL: @Luap99 @baude @TomSweeneyRedHat @mheon |
|
Unrelated CI failure; fixed in #27562 |
Fixes: https://issues.redhat.com/browse/RUN-3385 Fixes: containers#26321 Signed-off-by: Jan Rodák <[email protected]>
Signed-off-by: Jan Rodák <[email protected]>
The local API path optimization is ineffective on WSL because of NTFS mounting overhead. Signed-off-by: Jan Rodák <[email protected]>
7a6d65e to
d889aeb
Compare
|
LGTM |
|
PTAL @l0rd |
|
@Honny1 I have tested with hyper-v but I cannot see any difference in terms of perfs. I will look at it closer tomorrow. |
|
I reran that on Windows 11 and Hyper-V, and the results are the same; here is the report. I observed performance changes depending on the resources allocated to the Podman machine. I think I can set up an experiment to test different combinations, specifically comparing 2GB vs. 8GB of memory. Also, I am running these benchmarks on older hardware. WDYT? @l0rd |
|
The problem was on my side. I didn't replace the podman service in the machine. Now I have fixed that and I can see the improvemnts in perfs (5s vs 65s!) and I have also quickly verified that a file in a drive that is not mounted in the machine is correctly copied (and is slower as expected). LGTM on the Hyper-V side. I can have a look on WSL too if you want. |
|
That would be great to verify if WSL can benefit from newer hardware. Don't forget to build from source and drop the latest commit from this PR. (It disables this feature for WSL.) |
|
I confirm that on WSL, there is no performance difference before/after if I use the last commit of this PR (11s vs 5s with HyperV). But I get the same performances using commit 2f7094c too: > .\bin\windows\podman version
Client: Podman Engine
....
Git Commit: 2f7094c0dea104703c9adb9aedc505e503d34c42-dirty
....
Server: Podman Engine
...
Git Commit: 2f7094c0dea104703c9adb9aedc505e503d34c42
...
> Measure-Command { ./bin/windows/podman artifact add quay.io/myartifact/benchmark:latest ./artifacts/random-file }
Days : 0
Hours : 0
Minutes : 0
Seconds : 11
Milliseconds : 933
Ticks : 119333670
TotalDays : 0.000138117673611111
TotalHours : 0.00331482416666667
TotalMinutes : 0.19888945
TotalSeconds : 11.933367
TotalMilliseconds : 11933.367
> wsl --version
WSL version: 2.6.1.0
Kernel version: 6.6.87.2-1
WSLg version: 1.0.66
MSRDC version: 1.2.6353
Direct3D version: 1.611.1-81528511
DXCore version: 10.0.26100.1-240331-1435.ge-release
Windows version: 10.0.26200.7171 |
|
This indicates that on faster computers, this operation can perform similarly. I would still keep this feature disabled for WSL. |
l0rd
left a comment
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 have reviewed the code and, except a minor comment/question, LGTM.
Out of curiosity (and It's not something introduced by this PR anyway), but why do we require the artifact path to be absolute?
| // IsHyperVProvider checks if the current machine provider is Hyper-V. | ||
| // It returns true if the provider is Hyper-V, false otherwise, or an error if the check fails. | ||
| func IsHyperVProvider(ctx context.Context) (bool, error) { | ||
| func getVmProviderType(ctx context.Context) (define.VMType, error) { |
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.
Can't we use getProvider here?
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.
No, because this creates a small issue with providers and clients. When a user is using multiple providers, the client needs to determine which provider is used for the specific machine using that connection. I got caught by this. If we used getProvider, the user would need to specify CONTAINERS_MACHINE_PROVIDER before each command that utilizes the local API to determine which provider should be detected.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Honny1, l0rd 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 |
The local API facilitates path translation from client to server for convenience. Mounted host directories mimic the host's absolute paths, so a path on the host matches the path inside the VM (with a small exception for Windows, where |
This PR adds a new local artifact add API endpoint and enforces absolute path requirements for local file operations.
Fixes: https://issues.redhat.com/browse/RUN-3385
Fixes: #26321
Benchmark
podman artifact add quay.io/myartifact/benchmark:latest ./artifacts/random-fileBenchmark Results:
Mac OS
Providers
applehv
libkrun (krunkit 0.2.1)
Windows
Providers
WSL
Hyper-V
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?