-
Notifications
You must be signed in to change notification settings - Fork 522
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?
Changes from all commits
53e6c45
903beb8
2c97436
21ac59f
9659c53
b2e13d9
efa7b87
341c918
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -220,7 +220,11 @@ extension ClientImage { | |
| }) | ||
| } | ||
|
|
||
| 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)") | ||
| } | ||
|
|
||
| let client = newXPCClient() | ||
| let request = newRequest(.imagePull) | ||
|
|
||
|
|
@@ -234,6 +238,7 @@ extension ClientImage { | |
|
|
||
| let insecure = try scheme.schemeFor(host: host) == .http | ||
| request.set(key: .insecureFlag, value: insecure) | ||
| request.set(key: .maxConcurrentDownloads, value: Int64(maxConcurrentDownloads)) | ||
|
|
||
| var progressUpdateClient: ProgressUpdateClient? | ||
| if let progressUpdate { | ||
|
|
@@ -293,7 +298,7 @@ extension ClientImage { | |
| return (digests, size) | ||
| } | ||
|
|
||
| public static func fetch(reference: String, platform: Platform? = nil, scheme: RequestScheme = .auto, progressUpdate: ProgressUpdateHandler? = nil) async throws -> ClientImage | ||
| public static func fetch(reference: String, platform: Platform? = nil, scheme: RequestScheme = .auto, progressUpdate: ProgressUpdateHandler? = nil, maxConcurrentDownloads: Int = 3) async throws -> ClientImage | ||
| { | ||
| do { | ||
| let match = try await self.get(reference: reference) | ||
|
|
@@ -307,7 +312,7 @@ extension ClientImage { | |
| guard err.isCode(.notFound) else { | ||
| throw err | ||
| } | ||
| return try await Self.pull(reference: reference, platform: platform, scheme: scheme, progressUpdate: progressUpdate) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm somewhat confused by the commit description however. I thought we had found we do pull layers in parallel, but that it's just a hardcoded 8.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry I originally thought it wasn't pulling in parallel but I noticed it was hardcoded later
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated original issue to more accurately reflect this: #715 |
||
| return try await Self.pull(reference: reference, platform: platform, scheme: scheme, progressUpdate: progressUpdate, maxConcurrentDownloads: maxConcurrentDownloads) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| #!/usr/bin/env swift | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should they belong in Tests/CLITests/Subcommands/Images/?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this might be a good place for the tests.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The project is currently using camelCase for filenames.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated test cases |
||
|
|
||
| import Foundation | ||
|
|
||
| func testConcurrentDownloads() async throws { | ||
| print("Testing concurrent download behavior...\n") | ||
|
|
||
| // Track concurrent task count | ||
| actor ConcurrencyTracker { | ||
| var currentCount = 0 | ||
| var maxObservedCount = 0 | ||
| var completedTasks = 0 | ||
|
|
||
| func taskStarted() { | ||
| currentCount += 1 | ||
| maxObservedCount = max(maxObservedCount, currentCount) | ||
| } | ||
|
|
||
| func taskCompleted() { | ||
| currentCount -= 1 | ||
| completedTasks += 1 | ||
| } | ||
|
|
||
| func getStats() -> (max: Int, completed: Int) { | ||
| return (maxObservedCount, completedTasks) | ||
| } | ||
|
|
||
| func reset() { | ||
| currentCount = 0 | ||
| maxObservedCount = 0 | ||
| completedTasks = 0 | ||
| } | ||
| } | ||
|
|
||
| let tracker = ConcurrencyTracker() | ||
|
|
||
| // Test with different concurrency limits | ||
| for maxConcurrent in [1, 3, 6] { | ||
| await tracker.reset() | ||
|
|
||
| // Simulate downloading 20 layers | ||
| let layerCount = 20 | ||
| let layers = Array(0..<layerCount) | ||
|
|
||
| print("Testing maxConcurrent=\(maxConcurrent) with \(layerCount) layers...") | ||
|
|
||
| let startTime = Date() | ||
|
|
||
| try await withThrowingTaskGroup(of: Void.self) { group in | ||
| var iterator = layers.makeIterator() | ||
|
|
||
| // Start initial batch based on maxConcurrent | ||
| for _ in 0..<maxConcurrent { | ||
| if iterator.next() != nil { | ||
| group.addTask { | ||
| await tracker.taskStarted() | ||
| try await Task.sleep(nanoseconds: 10_000_000) | ||
| await tracker.taskCompleted() | ||
| } | ||
| } | ||
| } | ||
| for try await _ in group { | ||
| if iterator.next() != nil { | ||
| group.addTask { | ||
| await tracker.taskStarted() | ||
| try await Task.sleep(nanoseconds: 10_000_000) | ||
| await tracker.taskCompleted() | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let duration = Date().timeIntervalSince(startTime) | ||
| let stats = await tracker.getStats() | ||
|
|
||
| print(" ✓ Completed: \(stats.completed)/\(layerCount)") | ||
| print(" ✓ Max concurrent: \(stats.max)") | ||
| print(" ✓ Duration: \(String(format: "%.3f", duration))s") | ||
|
|
||
| guard stats.max <= maxConcurrent + 1 else { | ||
| throw TestError.concurrencyLimitExceeded | ||
| } | ||
|
|
||
| guard stats.completed == layerCount else { | ||
| throw TestError.incompleteTasks | ||
| } | ||
|
|
||
| print(" ✅ PASSED\n") | ||
| } | ||
|
|
||
| print("All tests passed!") | ||
| } | ||
|
|
||
| enum TestError: Error { | ||
| case concurrencyLimitExceeded | ||
| case incompleteTasks | ||
| } | ||
|
|
||
| Task { | ||
| do { | ||
| try await testConcurrentDownloads() | ||
| exit(0) | ||
| } catch { | ||
| print("Test failed: \(error)") | ||
| exit(1) | ||
| } | ||
| } | ||
|
|
||
| RunLoop.main.run() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| #!/usr/bin/env swift | ||
|
|
||
| import Foundation | ||
|
|
||
| print("Testing parameter flow...\n") | ||
|
|
||
| print("1. CLI flag parsing...") | ||
| struct ProgressFlags { | ||
| var disableProgressUpdates = false | ||
| var maxConcurrentDownloads: Int = 3 | ||
| } | ||
|
|
||
| let defaultFlags = ProgressFlags() | ||
| print(" ✓ Default: \(defaultFlags.maxConcurrentDownloads)") | ||
|
|
||
| let customFlags = ProgressFlags(disableProgressUpdates: false, maxConcurrentDownloads: 6) | ||
| print(" ✓ Custom: \(customFlags.maxConcurrentDownloads)") | ||
| print(" PASSED\n") | ||
|
|
||
| print("2. XPC key...") | ||
| enum ImageServiceXPCKeys: String { | ||
| case maxConcurrentDownloads | ||
| } | ||
|
|
||
| let key = ImageServiceXPCKeys.maxConcurrentDownloads | ||
| print(" ✓ Key exists: \(key.rawValue)") | ||
| print(" PASSED\n") | ||
|
|
||
| print("3. Function signatures...") | ||
| func mockClientImagePull( | ||
| reference: String, | ||
| maxConcurrentDownloads: Int = 3 | ||
| ) -> String { | ||
| return "pull(\(reference), maxConcurrent=\(maxConcurrentDownloads))" | ||
| } | ||
|
|
||
| _ = mockClientImagePull(reference: "nginx:latest") | ||
| _ = mockClientImagePull(reference: "nginx:latest", maxConcurrentDownloads: 6) | ||
| print(" ✓ Compiles") | ||
| print(" PASSED\n") | ||
|
|
||
| print("4. Parameter propagation...") | ||
|
|
||
| struct MockXPCMessage { | ||
| var values: [String: Any] = [:] | ||
|
|
||
| mutating func set(key: String, value: Int64) { | ||
| values[key] = value | ||
| } | ||
|
|
||
| func int64(key: String) -> Int64 { | ||
| return values[key] as? Int64 ?? 3 | ||
| } | ||
| } | ||
|
|
||
| func simulateFlow(maxConcurrent: Int) -> Int { | ||
| let flags = ProgressFlags(maxConcurrentDownloads: maxConcurrent) | ||
| var xpcMessage = MockXPCMessage() | ||
| xpcMessage.set(key: "maxConcurrentDownloads", value: Int64(flags.maxConcurrentDownloads)) | ||
| return Int(xpcMessage.int64(key: "maxConcurrentDownloads")) | ||
| } | ||
|
|
||
| for testValue in [1, 3, 6] { | ||
| guard simulateFlow(maxConcurrent: testValue) == testValue else { | ||
| print(" ✗ Failed") | ||
| exit(1) | ||
| } | ||
| } | ||
| print(" ✓ Values propagate correctly") | ||
| print(" PASSED\n") | ||
|
|
||
| print("5. Implementation verification...") | ||
|
|
||
| let filesToCheck = [ | ||
| "Sources/ContainerClient/Flags.swift", | ||
| "Sources/ContainerClient/Core/ClientImage.swift", | ||
| "Sources/Services/ContainerImagesService/Server/ImageService.swift", | ||
| ] | ||
|
|
||
| for file in filesToCheck { | ||
| if let content = try? String(contentsOf: URL(fileURLWithPath: file), encoding: .utf8), | ||
| content.contains("maxConcurrentDownloads") { | ||
| continue | ||
| } | ||
| print(" ✗ Missing in \(file)") | ||
| exit(1) | ||
| } | ||
| print(" ✓ Found in implementation") | ||
| print(" PASSED\n") | ||
|
|
||
| print("All tests passed!") |
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 we check that
maxConcurrentDownloads > 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.
Added validation check