-
Notifications
You must be signed in to change notification settings - Fork 530
Add --max-concurrent-downloads flag for parallel layer downloads #716
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
| @@ -0,0 +1,109 @@ | |||
| #!/usr/bin/env swift | |||
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.
Why is this and test_parameter_flow.swift in the top level directory?
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.
should they belong in Tests/CLITests/Subcommands/Images/?
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.
Yes, this might be a good place for the tests.
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.
This is still unresolved.
c843892 to
cc15745
Compare
cc15745 to
26ebe63
Compare
Implements concurrent layer downloads to improve image pull performance by fetching multiple layers simultaneously instead of sequentially.
26ebe63 to
53e6c45
Compare
|
@dcantah anything else needed to merge this? |
|
@sbhavani Yes. We need a tag of Containerization that has the changes. I'm making some changes over there but should have a tag out within the next day or so and then we can bump to this. We should not have our dependency on Containerization be to a branch, and especially not main 😄 |
jglogan
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.
@sbhavani Just a couple of nits if you still have time to move this forward. Thank you for contributing!
Sources/ContainerClient/Flags.swift
Outdated
|
|
||
| public init(disableProgressUpdates: Bool, maxConcurrentDownloads: Int) { | ||
| self.disableProgressUpdates = disableProgressUpdates | ||
| self.maxConcurrentDownloads = maxConcurrentDownloads |
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.
What's the reason behind putting this in the progress group?
The problem I see here is that any command that wants to present the --progress flag (--disable-progress-updates is gone, apologies for letting this PR go so long so as to force a rebase here) will also see --max-concurrent-downloads whether downloads are relevant to the other command or not. Is there a better home for this? Or do we need to create another reusable group especially for download-related options?
I don't see any harm in copy/pasting this option into each command that needs it either, unless there's simply too many of them.
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.
Good point! I think image pull definitely needs it as a user might change this for large images.
container run and container create could use it but its probably not needed as an option.
Is there anything similar to Docker daemon.json? It would be convenient to set a config so all images are pulled with the same maxConcurrentDownloads
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.
This is still unresolved. Consider:
- Removing
maxConcurrentDownloadsfromFlags.Progress - Creating new
Flags.ImageFetchgroup specifically for image fetch options - Adding
@OptionGroup var imageFetchFlags: Flags.ImageFetchto:container image pullcontainer runcontainer create
no problem! I'll take a look and make the changes |
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.
Thank you for the contribution 👍
The default value 3 seems reasonable.
| @@ -0,0 +1,109 @@ | |||
| #!/usr/bin/env swift | |||
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.
Yes, this might be a good place for the tests.
e02e2ff to
21ac59f
Compare
dkovba
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.
Please run make fmt to pass the PR checks.
| let gitCommit = ProcessInfo.processInfo.environment["GIT_COMMIT"] ?? "unspecified" | ||
| let builderShimVersion = "0.7.0" | ||
| let scVersion = "0.13.0" | ||
| let scVersion = "0.11.0" |
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.
Originally, we had to upgrade to 0.11.0. At this moment, we don't have to downgrade to it. Please revert this change.
| czConfig.process.workingDirectory = process.workingDirectory | ||
| czConfig.process.rlimits = process.rlimits.map { | ||
| .init(type: $0.limit, hard: $0.hard, soft: $0.soft) | ||
| POSIXRlimit(type: $0.limit, hard: $0.hard, soft: $0.soft) |
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.
This changes seems to be unnecessary (repeats 2 times).
| switch process.user { | ||
| case .raw(let name): | ||
| czConfig.process.user = .init( | ||
| czConfig.process.user = ContainerizationOCI.User( |
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.
This changes seems to be unnecessary (repeats 4 times).
| @@ -0,0 +1,109 @@ | |||
| #!/usr/bin/env swift | |||
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.
This is still unresolved.
| public static func pull(reference: String, platform: Platform? = nil, scheme: RequestScheme = .auto, progressUpdate: ProgressUpdateHandler? = nil) async throws -> ClientImage { | ||
| public static func pull(reference: String, platform: Platform? = nil, scheme: RequestScheme = .auto, progressUpdate: ProgressUpdateHandler? = nil, maxConcurrentDownloads: Int = 3) async throws -> ClientImage { | ||
| guard maxConcurrentDownloads > 0 else { | ||
| throw ContainerizationError(.invalidArgument, message: "maxConcurrentDownloads must be greater than 0, got \(maxConcurrentDownloads)") |
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.
The error message should be user-facing. Consider something like:
- --max-concurrent-downloads must be greater than 0, got
(maxConcurrentDownloads) - invalid value for --max-concurrent-downloads: must be greater
than 0, got (maxConcurrentDownloads)
| @Option(name: .long, help: ArgumentHelp("Progress type (format: none|ansi)", valueName: "type")) | ||
| public var progress: ProgressType = .ansi | ||
|
|
||
| @Option(name: .long, help: "Maximum number of concurrent layer downloads (default: 3)") |
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.
@adityaramani Should we use the word "layer" or "blob" here?
Adds
--max-concurrent-downloadsflag tocontainer image pullfor configurable concurrent layer downloads.Fixes #715
Depends on apple/containerization#311
Usage:
Changes:
Performance: ~1.2-1.3x faster pulls for multi-layer images with higher concurrency
Tests: Included standalone tests verify concurrency behavior and parameter flow