-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[APMLP-876] Add config variables for imageresolver.cache TTL and enable/disable gradual rollout
#46195
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?
[APMLP-876] Add config variables for imageresolver.cache TTL and enable/disable gradual rollout
#46195
Changes from 6 commits
7f691ff
285b239
227466f
d738228
984b3a9
edfd02e
59c606d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,7 +124,6 @@ func (r *rcResolver) Resolve(registry string, repository string, tag string) (*R | |
| defer r.mu.RUnlock() | ||
|
|
||
| if len(r.imageMappings) == 0 { | ||
| log.Debugf("Cache empty, no resolution available") | ||
|
Contributor
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. This is so disruptive in reading the failed test logs, I can only imagine it's just as disruptive for real services. Removing 😅
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. Fair - this seems like valid/ "expected" behavior so probably no need to report it. |
||
| metrics.ImageResolutionAttempts.Inc(repository, tag, tag) | ||
| return nil, false | ||
| } | ||
|
|
@@ -275,6 +274,9 @@ func newBucketTagResolver(cfg Config) *bucketTagResolver { | |
| // New creates the appropriate Resolver based on whether | ||
| // a remote config client is available. | ||
| func New(cfg Config) Resolver { | ||
| if !cfg.Enabled { | ||
| return NewNoOpResolver() | ||
| } | ||
| if cfg.RCClient == nil || reflect.ValueOf(cfg.RCClient).IsNil() { | ||
| log.Debugf("No remote config client available") | ||
|
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. Nit: Not directly related to these changes, but would be nice to get more specific (about the impact) on this log as well.
Contributor
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. Yeah spoiler alert... I'm going to remove all of this remote config based code in a subsequent PR 😅 So it'll go away in a bit |
||
| return NewNoOpResolver() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # Each section from every release note are combined when the | ||
| # CHANGELOG.rst is rendered. So the text needs to be worded so that | ||
| # it does not depend on any information only available in another | ||
| # section. This may mean repeating some details, but each section | ||
| # must be readable independently of the other. | ||
| # | ||
| # Each section note must be formatted as reStructuredText. | ||
| --- | ||
| enhancements: | ||
| - | | ||
| Added support for two new configurations for tag-based gradual rollout in K8s SSI deployments. | ||
erikayasuda marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| The gradual rollout can be configured using the following parameters: | ||
| - ``DD_ADMISSION_CONTROLLER_AUTO_INSTRUMENTATION_GRADUAL_ROLLOUT_ENABLED``: Whether to enable gradual rollout (default: true) | ||
| - ``DD_ADMISSION_CONTROLLER_AUTO_INSTRUMENTATION_GRADUAL_ROLLOUT_CACHE_TTL_HOURS``: The cache TTL in hours for the gradual rollout image cache (default: 1) | ||
| - This cache is used to store the mapping of mutable tags -> image digest for the gradual rollout, and setting this TTL helps prevent the image resolution from becoming stale. | ||
erikayasuda marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
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.
Are you sure you want to lock in to hours here? Is there a case where a user might want a finer grained window?
Before these changes, DigestCacheTTL was just of type time.Duration; with these changes, it will always be on the order of hours.
If it's unlikely a user will care, leave as is.
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.
Yeah good call out. I just specified this as hours so it was human-readable when you configured it 🤔 I figured setting a TTL by minutes wouldn't be a very common use case...?