Skip to content

resolver: fix resolver reuse#1622

Merged
AkihiroSuda merged 3 commits intomoby:masterfrom
tonistiigi:resolver-performance
Aug 3, 2020
Merged

resolver: fix resolver reuse#1622
AkihiroSuda merged 3 commits intomoby:masterfrom
tonistiigi:resolver-performance

Conversation

@tonistiigi
Copy link
Member

  • Containerd config loading repeatedly calls hosts config callback, creating new authorizer. Now authorizer is cached. Also client was recreated if the host had a custom configuration.
  • Keep-alive was disabled with HTTP2 but shouldn't have been. HTTP/2 remains disabled.
  • EnsureManifestLoaded check (for gcr broken auth) had regressed and wasn't working because a struct was cloned

This should significantly improve performance for registry communication. Still a lot of things to improve but at least all regressions should be gone now. For further changes, I'd need to make more auth helper functions public in containerd to avoid copy-paste.

@sipsma @fuweid

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Was disabled with http2 but shouldn’t have been.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi tonistiigi force-pushed the resolver-performance branch from 8b37573 to 22061b1 Compare August 2, 2020 16:33
@sipsma
Copy link
Collaborator

sipsma commented Aug 2, 2020

Changes LGTM but should there additionally be a similar fix to withLocalResolver to make its counter be *int64 instead of int64? Unless I'm misunderstanding it looks to have the same issue being fixed in cachedResolver.

@tonistiigi
Copy link
Member Author

@sipsma I think it is ok atm. The difference is that cachedResolver is copied as a value, while withLocalResolver shouldn't be. Also withLocalResolver just falls back to internal cachedResolver

@AkihiroSuda AkihiroSuda merged commit 2f5e825 into moby:master Aug 3, 2020
Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM(nb)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants