Skip to content
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

(re-)Implement parallelism in deploy #110

Merged
merged 4 commits into from
Feb 4, 2025

Conversation

tianon
Copy link
Member

@tianon tianon commented Jan 31, 2025

This requires the input to be in the correct order (children first), but that was already an implied constraint.

In my testing, this takes our .test/test.sh --deploy block testing deploy from ~5s down to ~2.5s.

When we originally introduced this, it collided with some other aging infrastructure in a bad way, kicking over everything, and had to be reverted. We've since changed that and this should be safe to re-introduce.

See also:

@tianon tianon requested a review from a team as a code owner January 31, 2025 00:15
@tianon
Copy link
Member Author

tianon commented Jan 31, 2025

As noted in #105 (comment), if this is something @yosifkit agrees seems relatively safe to re-introduce, we should wait at least until next Monday to do so. 🙇

@tianon
Copy link
Member Author

tianon commented Jan 31, 2025

We could also do what we did in cmd/lookup and have an explicit --parallel boolean flag for this: 🤔

if len(args) > 0 && args[0] == "--parallel" {
args = args[1:]
parallel = true
}

@yosifkit
Copy link
Member

I think an explicit --parallel flag would be good. Then we can turn it on or off as we test and verify our proxy without reverting it again.

@tianon
Copy link
Member Author

tianon commented Jan 31, 2025

  • Implement explicit --parallel flag

  • Add --dry-run flag

    This swaps the output to be input-compatible such that we can verify the results of a deploy, especially our new "filtered deploy" (Speed up deploy optimistically #106; we can verify that the filtering was warranted, but out-of-band instead of in-band).

    This also allows us to do more/better testing (see .test changes, especially test.sh).

@tianon tianon force-pushed the parallelism-redux branch from cec129f to ba525e6 Compare January 31, 2025 18:21
This requires the input to be in the correct order (children first), but that was already an implied constraint.

In my testing, this takes our `.test/test.sh --deploy` block testing deploy from ~5s down to ~2.5s.

When we originally introduced this, it collided with some other aging infrastructure in a bad way, kicking over everything, and had to be reverted.  We've since changed that and this should be safe to re-introduce.
I'm working on a `--dry-run` flag that will improve the coverage of this, but wanted separate commits.
This swaps the output to be input-compatible such that we can verify the results of a deploy, especially our new "filtered deploy" (we can verify that the filtering was warranted, but out-of-band instead of in-band).

This also allows us to do more/better testing (see `.test` changes, especially `test.sh`).
@tianon tianon force-pushed the parallelism-redux branch from ba525e6 to 10f3b08 Compare January 31, 2025 18:23
The new `deploy --dry-run` feature relies on the assumption that JSON-ified normalized input objects are valid input back, which wasn't an assumption we were validating.

This adjusts `TestNormalizeInput` to take the output of normalization and pass it back through for an added "roundtrip" test.  In doing so, I found a single edge case where our normalized objects were *not* valid/correct inputs (tag->tag copy of a manifest) and made a minor adjustment to the normalization to close that hole.

I also added a new test case for that edge case, and made sure *it* round-trips correctly too.
@tianon
Copy link
Member Author

tianon commented Feb 4, 2025

comparing https://github.com/docker-library/meta-scripts/actions/runs/13119586724/job/36602129641#logs to https://github.com/docker-library/meta-scripts/actions/runs/13125173171/job/36619923417?pr=110

--- before.txt	2025-02-03 16:08:00.384383528 -0800
+++ after.txt	2025-02-03 16:06:06.851327211 -0800
@@ -3,10 +3,13 @@
 cmd/builds/main.go:			loadCacheFromFile		76.0%
 cmd/builds/main.go:			saveCacheToFile			78.6%
 cmd/builds/main.go:			main				80.7%
+cmd/deploy/input.go:			clone				100.0%
 cmd/deploy/input.go:			normalizeInputRefs		100.0%
 cmd/deploy/input.go:			normalizeInputLookup		100.0%
-cmd/deploy/input.go:			NormalizeInput			81.3%
-cmd/deploy/main.go:			main				77.1%
+cmd/deploy/input.go:			NormalizeInput			80.3%
+cmd/deploy/input.go:			do				87.5%
+cmd/deploy/input.go:			dryRun				78.6%
+cmd/deploy/main.go:			main				91.0%
 cmd/lookup/main.go:			main				74.6%
 om/om.go:				Keys				100.0%
 om/om.go:				Get				100.0%
@@ -21,7 +24,7 @@
 registry/cache.go:			GetBlob				100.0%
 registry/cache.go:			GetManifest			100.0%
 registry/cache.go:			GetTag				91.2%
-registry/cache.go:			resolveBlob			83.3%
+registry/cache.go:			resolveBlob			100.0%
 registry/cache.go:			ResolveManifest			100.0%
 registry/cache.go:			ResolveBlob			100.0%
 registry/cache.go:			ResolveTag			84.6%
@@ -31,7 +34,8 @@
 registry/client.go:			Client				57.1%
 registry/client.go:			EntryForRegistry		86.7%
 registry/lookup.go:			Lookup				87.5%
-registry/push.go:			EnsureManifest			78.9%
+registry/manifest-children.go:	ParseManifestChildren		100.0%
+registry/push.go:			EnsureManifest			73.2%
 registry/push.go:			CopyManifest			71.4%
 registry/push.go:			EnsureBlob			77.8%
 registry/push.go:			CopyBlob			65.5%
@@ -47,4 +51,4 @@
 registry/synthesize-index.go:	setRefAnnotation		100.0%
 registry/synthesize-index.go:	normalizeManifestPlatform	83.9%
 registry/user-agent.go:		RoundTrip			83.3%
-total:					(statements)			80.1%
+total:					(statements)			81.5%

@yosifkit yosifkit merged commit da908ae into docker-library:main Feb 4, 2025
1 check passed
@yosifkit yosifkit deleted the parallelism-redux branch February 4, 2025 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants