perf: optimise docker authentication config lookup #2646
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Improve the performance of retrieving docker authentication configuration by performing requests in parallel and only performing lookups once. For a simple double layer image with standard Google Cloud Registry configuration this reduced the time spent doing authentication config lookups from ~4.6s to ~400ms.
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Thanks for adding this contribution! Could you share how you measured the lookups? I'm interested in doing some internal tests before merging this. (at first sight, it LGTM) |
To get accurate results I temporarily instrumented func getAuthConfigsFromDockerfile(c *ContainerRequest) map[string]registry.AuthConfig {
start := time.Now()
defer func() { fmt.Printf("getAuthConfigsFromDockerfile took: %s\n", time.Since(start)) }()I was seeing 5 seconds to build an image which was fully cached, so I got curious and found that 90% of that time was spent in In my case I had Google GCR configured so my {
"_credsStore": "desktop.exe",
"auths": {
"https://index.docker.io/v1/": {}
},
"credHelpers": {
"asia.gcr.io": "gcloud",
"eu.gcr.io": "gcloud",
"gcr.io": "gcloud",
"marketplace.gcr.io": "gcloud",
"staging-k8s.gcr.io": "gcloud",
"us-west1-docker.pkg.dev": "gcloud",
"us.gcr.io": "gcloud"
},
"credsStore": "desktop.exe",
"experimental": "enabled"
}Because my Dockerfile had two layers it was doing the auth lookups twice, hence the once. |
mdelapenya
approved these changes
Jul 16, 2024
Cool, I'll keep an eye out 😃 |
stevenh
added a commit
to stevenh/testcontainers-go
that referenced
this pull request
Jul 20, 2024
Check and return errors in the process of determining authentication configs so that unexpected failures don't occur latter in the process of build an image or creating a container. BuildOptions will now return an empty result on error instead of an incomplete one, to ensure that consumers don't use partial data. Fix builds with different config or override environments failing when the authentication configuration changes, which was introduced by testcontainers#2646. Report errors from GetRegistryCredentials calls to avoid unexpected failures latter on in the authentication process. Split out the functionality to read a Dockerfile from an io.Reader into ExtractImagesFromReader, as required when processing from a tar archive. Deprecated function ContainerRequest.GetAuthConfigs will now panic if an error occurs, so that callers understand that an failure occured. BREAKING CHANGE Add support for determining the required authentication in when building an image from a ContextArchive, this requires ContextArchive support io.Seeker.
stevenh
added a commit
to stevenh/testcontainers-go
that referenced
this pull request
Jul 20, 2024
Check and return errors in the process of determining authentication configs so that unexpected failures don't occur latter in the process of build an image or creating a container. BuildOptions will now return an empty result on error instead of an incomplete one, to ensure that consumers don't use partial data. Fix builds with different config or override environments failing when the authentication configuration changes, which was introduced by testcontainers#2646. Report errors from GetRegistryCredentials calls to avoid unexpected failures latter on in the authentication process. Split out the functionality to read a Dockerfile from an io.Reader into ExtractImagesFromReader, as required when processing from a tar archive. Deprecated function ContainerRequest.GetAuthConfigs will now panic if an error occurs, so that callers understand that an failure occured. BREAKING CHANGE Add support for determining the required authentication in when building an image from a ContextArchive, this requires ContextArchive support io.Seeker.
mdelapenya
added a commit
that referenced
this pull request
Aug 5, 2024
* main: feat: add grafana-lgtm module (#2660) Added valkey module (#2639) fix: container.Endpoint and wait.FortHTTP to use lowest internal port (#2641) chore: test cleanups (#2657) docs: fix compilation of examples (#2656) feat: add custom container registry substitutor (#2647) fix: couchbase containers intermittently hang on startup (#2650) chore(deps): bump Ryuk to 0.8.1 (#2648) fix: retry on label error (#2644) perf: optimise docker authentication config lookup (#2646)
stevenh
added a commit
to stevenh/testcontainers-go
that referenced
this pull request
Aug 9, 2024
Check and return errors in the process of determining authentication configs so that unexpected failures don't occur latter in the process of build an image or creating a container. BuildOptions will now return an empty result on error instead of an incomplete one, to ensure that consumers don't use partial data. Fix builds with different config or override environments failing when the authentication configuration changes, which was introduced by testcontainers#2646. Report errors from GetRegistryCredentials calls to avoid unexpected failures latter on in the authentication process. Split out the functionality to read a Dockerfile from an io.Reader into ExtractImagesFromReader, as required when processing from a tar archive. Deprecated function ContainerRequest.GetAuthConfigs will now panic if an error occurs, so that callers understand that an failure occured. BREAKING CHANGE Add support for determining the required authentication in when building an image from a ContextArchive, this requires ContextArchive support io.Seeker.
stevenh
added a commit
to stevenh/testcontainers-go
that referenced
this pull request
Aug 14, 2024
Check and return errors in the process of determining authentication configs so that unexpected failures don't occur latter in the process of build an image or creating a container. BuildOptions will now return an empty result on error instead of an incomplete one, to ensure that consumers don't use partial data. Fix builds with different config or override environments failing when the authentication configuration changes, which was introduced by testcontainers#2646. Report errors from GetRegistryCredentials calls to avoid unexpected failures latter on in the authentication process. Split out the functionality to read a Dockerfile from an io.Reader into ExtractImagesFromReader, as required when processing from a tar archive. Deprecated function ContainerRequest.GetAuthConfigs will now panic if an error occurs, so that callers understand that an failure occurred. Remove unused parameter t from prepareRedisImage. BREAKING CHANGE Add support for determining the required authentication in when building an image from a ContextArchive, this requires ContextArchive support io.Seeker.
stevenh
added a commit
to stevenh/testcontainers-go
that referenced
this pull request
Aug 14, 2024
Check and return errors in the process of determining authentication configs so that unexpected failures don't occur latter in the process of build an image or creating a container. BuildOptions will now return an empty result on error instead of an incomplete one, to ensure that consumers don't use partial data. Fix builds with different config or override environments failing when the authentication configuration changes, which was introduced by testcontainers#2646. Report errors from GetRegistryCredentials calls to avoid unexpected failures latter on in the authentication process. Split out the functionality to read a Dockerfile from an io.Reader into ExtractImagesFromReader, as required when processing from a tar archive. Deprecated function ContainerRequest.GetAuthConfigs will now panic if an error occurs, so that callers understand that an failure occurred. Remove unused parameter t from prepareRedisImage. BREAKING CHANGE Add support for determining the required authentication in when building an image from a ContextArchive, this requires ContextArchive support io.Seeker.
stevenh
added a commit
to stevenh/testcontainers-go
that referenced
this pull request
Aug 14, 2024
Check and return errors in the process of determining authentication configs so that unexpected failures don't occur latter in the process of build an image or creating a container. BuildOptions will now return an empty result on error instead of an incomplete one, to ensure that consumers don't use partial data. Fix builds with different config or override environments failing when the authentication configuration changes, which was introduced by testcontainers#2646. Report errors from GetRegistryCredentials calls to avoid unexpected failures latter on in the authentication process. Split out the functionality to read a Dockerfile from an io.Reader into ExtractImagesFromReader, as required when processing from a tar archive. Deprecated function ContainerRequest.GetAuthConfigs will now panic if an error occurs, so that callers understand that an failure occurred. Remove unused parameter t from prepareRedisImage. BREAKING CHANGE Add support for determining the required authentication in when building an image from a ContextArchive, this requires ContextArchive support io.Seeker.
mdelapenya
pushed a commit
that referenced
this pull request
Aug 16, 2024
Check and return errors in the process of determining authentication configs so that unexpected failures don't occur latter in the process of build an image or creating a container. BuildOptions will now return an empty result on error instead of an incomplete one, to ensure that consumers don't use partial data. Fix builds with different config or override environments failing when the authentication configuration changes, which was introduced by #2646. Report errors from GetRegistryCredentials calls to avoid unexpected failures latter on in the authentication process. Split out the functionality to read a Dockerfile from an io.Reader into ExtractImagesFromReader, as required when processing from a tar archive. Deprecated function ContainerRequest.GetAuthConfigs will now panic if an error occurs, so that callers understand that an failure occurred. Remove unused parameter t from prepareRedisImage. BREAKING CHANGE Add support for determining the required authentication in when building an image from a ContextArchive, this requires ContextArchive support io.Seeker.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Improve the performance of retrieving docker authentication configuration by performing requests in parallel and only performing lookups once.
For a simple double layer image with standard Google Cloud Registry configuration this reduced the time spent doing authentication config lookups from ~4.6s to ~400ms.