Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
Honny1 wants to merge3 commits intocontainers:main
base:main
Choose a base branch
Loading
fromHonny1:local-api-artifact-add

Conversation

@Honny1
Copy link
Member

@Honny1Honny1 commentedNov 3, 2025
edited
Loading

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

  • Command:podman artifact add quay.io/myartifact/benchmark:latest ./artifacts/random-file
  • Artifacts Dir content:
$ du -a -h | sort -hr5G.5.0G ./random-file

Benchmark Results:

Mac OS

  • Number of runs: 10
  • Machine:
    • Chip: Apple M3 Pro
    • RAM: 36 GB
    • SSD: 1 TB Apple SSD
  • Podman machine configuration:
    • CPUs: 8
    • Memory: 8.1 GB
  • report

Providers

applehv
  • Before: 32.82 s
  • After:5.77 s
libkrun (krunkit 0.2.1)
  • Before: 25.87 s
  • After:7.17 s

Windows

  • Windows 10
  • Number of runs: 10
  • Machine:
    • CPU: Intel Core i7-8665U
    • RAM: 32 GB
    • SSD: 500 GB Samsung 970 Evo Plus
  • Podman machine configuration:
    • CPUs: 8
    • Memory: 2 GB (WSL) | 8.2 GB (Hyper-v)
  • report

Providers

WSL
  • Memory: 2 GB
  • Before:35.25 s
  • After: 78.31 s
Hyper-V
  • Memory: 8.2 GB
  • Before: 362.70 s
  • After:51.66 s

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

  • Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
    commits. (git commit -s). (If needed, usegit commit -s --amend). The author email must match
    the sign-off email address. SeeCONTRIBUTING.md
    for more information.
  • Referenced issues usingFixes: #00000 in commit message (if applicable)
  • Tests have been added/updated (or no tests are needed)
  • Documentation has been updated (or no documentation changes are needed)
  • All commits passmake validatepr (format/lint checks)
  • Release note entered in the section below (orNone if no user-facing changes)

Does this PR introduce a user-facing change?

Added new local artifact add API endpoint (`libpod/local/artifacts/add`) for loading artifacts from local filesImage and artifact load endpoints now require absolute paths for local file operations to improve security and prevent path ambiguity

@openshift-ciopenshift-cibot added do-not-merge/work-in-progressIndicates that a PR should not merge because it is a work in progress. release-note approvedIndicates a PR has been approved by an approver from all required OWNERS files. labelsNov 3, 2025
@github-actionsgithub-actionsbot added the kind/api-changeChange to remote API; merits scrutiny labelNov 3, 2025
@Honny1Honny1force-pushed thelocal-api-artifact-add branch 6 times, most recently from0b018d8 to28a90c6CompareNovember 4, 2025 14:00
@TomSweeneyRedHat
Copy link
Member

Quick breeze, it LGTM, and happy green test buttons to boot!

@Honny1Honny1 marked this pull request as ready for reviewNovember 6, 2025 10:27
@openshift-ciopenshift-cibot removed the do-not-merge/work-in-progressIndicates that a PR should not merge because it is a work in progress. labelNov 6, 2025
@baude
Copy link
Member

really quick pass LGTM ... hopefully someone can walk through a little slower than today allowed.

@Honny1
Copy link
MemberAuthor

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.

Benchmark report

PTAL:@Luap99@baude@TomSweeneyRedHat@mheon

@Honny1
Copy link
MemberAuthor

Unrelated CI failure; fixed in#27562

Signed-off-by: Jan Rodák <hony.com@seznam.cz>
The local API path optimization is ineffective on WSL because of NTFS mounting overhead.Signed-off-by: Jan Rodák <hony.com@seznam.cz>
@TomSweeneyRedHat
Copy link
Member

LGTM
but I'd like a head nod or two from folks more familiar with macOS and Windows than I.

@Honny1
Copy link
MemberAuthor

PTAL@l0rd

@l0rd
Copy link
Member

@Honny1 I have tested with hyper-v but I cannot see any difference in terms of perfs. I will look at it closer tomorrow.

@Honny1
Copy link
MemberAuthor

Honny1 commentedNov 21, 2025
edited
Loading

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

Benchmark Report (hyperv)

@l0rd
Copy link
Member

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.

@Honny1
Copy link
MemberAuthor

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.)

@l0rd
Copy link
Member

l0rd commentedNov 24, 2025
edited
Loading

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 commit2f7094c too:

> .\bin\windows\podman versionClient:       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              :0Hours             :0Minutes           :0Seconds           :11Milliseconds      :933Ticks             :119333670TotalDays         :0.000138117673611111TotalHours        :0.00331482416666667TotalMinutes      :0.19888945TotalSeconds      :11.933367TotalMilliseconds :11933.367> wsl--versionWSL version:2.6.1.0Kernel version:6.6.87.2-1WSLg version:1.0.66MSRDC version:1.2.6353Direct3D version:1.611.1-81528511DXCore version:10.0.26100.1-240331-1435.ge-releaseWindows version:10.0.26200.7171

@Honny1
Copy link
MemberAuthor

This indicates that on faster computers, this operation can perform similarly. I would still keep this feature disabled for WSL.

Copy link
Member

@l0rdl0rd left a 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.
funcIsHyperVProvider(ctx context.Context) (bool,error) {
funcgetVmProviderType(ctx context.Context) (define.VMType,error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can't we usegetProvider here?

Copy link
MemberAuthor

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 usedgetProvider, the user would need to specifyCONTAINERS_MACHINE_PROVIDER before each command that utilizes the local API to determine which provider should be detected.

@openshift-ci
Copy link
Contributor

[APPROVALNOTIFIER] This PR isAPPROVED

This pull-request has been approved by:Honny1,l0rd

The full list of commands accepted by this bot can be foundhere.

The pull request process is describedhere

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

@Honny1
Copy link
MemberAuthor

Out of curiosity (and It's not something introduced by this PR anyway), but why do we require the artifact path to be absolute?

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, whereC:\\ translates to/mnt/c/). Consequently, users utilizing the Podman client do not need to strictly use absolute paths. This is standard behavior across all local APIs.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@l0rdl0rdl0rd approved these changes

Assignees

No one assigned

Labels

approvedIndicates a PR has been approved by an approver from all required OWNERS files.kind/api-changeChange to remote API; merits scrutinyrelease-note

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Podman-Remote needs api to dolocal artifact creation or builds.

4 participants

@Honny1@TomSweeneyRedHat@baude@l0rd

[8]ページ先頭

©2009-2025 Movatter.jp