consul-template: Add fault tolerant defaults #13041
Conversation
|
Since this will probably involve some docs changes, can we sneak in an explanation of the unit of "max_stale" here https://www.nomadproject.io/docs/configuration/client#max_stale (maybe the default value being there will make this obvious?) A user had mentioned that this was a bit confusing, and they didnt know if it was a time value or index diff or something like that. |
I've updated the default value in that last commit, but IIRC this text is lifted directly from the Consul docs. Any specific feedback about what is unclear? |
There was a problem hiding this comment.
The code change here LGTM.
I've left some suggestions on cleaning up the documentation while we're in here, but maybe that should be in a separate PR for backporting concerns. We'll also need a changelog entry and a section of the version-specific upgrade guide detailing the changes we're making here.
Co-authored-by: Tim Gross <[email protected]>
DerekStrickland
changed the title
| of time to wait before attempting to re-render a template. By default, Consul Template | ||
| will loop continually and re-render. If this value is set, Nomad will configure Consul |
There was a problem hiding this comment.
By default, Consul Template
Aren't 5s/4m the defaults?
There was a problem hiding this comment.
They are. I thought our convention was to put the defaults in these code snippet sections. Is that not the case?
There was a problem hiding this comment.
Oh hm, maybe I just misread this sentence. I think when I see:
By default X. If this value is set, Y.
I assume that the value in question is not set by default. The phrasing definitely isn't explicitly saying that, so maybe it's just my poor reading comprehension at work. 😅
The preceding and following sentences clear it all up, but maybe these 2 could be rephrased a bit:
Consul Template re-renders templates whenever rendered variables from Consul, Nomad, or Vault change. However in order to minimize how often tasks are restarted or reloaded, Nomad will configure .
My main point being is that I think the By default... in the first sentence makes users think this parameter changes that behavior when it doesn't. Re-rendering in a loop is just how Consul Template works and these parameters just tweak some timings.
There was a problem hiding this comment.
Ah ok. That makes sense. I'll stage a new PR with these doc changes.
There was a problem hiding this comment.
Aren't 5s/4m the defaults?
They are not. Default max in consul-template 4 times of Minimum value not 4 Minutes
|
@DerekStrickland I noticed that you incorrectly read consul-template code. |
|
@valodzka Thanks for catching this! |
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
closes #13020
This PR adds fault-tolerant defaults to the default agent config.
BlockQueryWaitTime: defaults to 5 min, matching the default value Consul usesMaxStale: defaults to 10 years matching the default value Consul usesWait.Min: defaults to 5 seconds overriding the default of 0 that Consul Template usesWait.Max: defaults to 4 minutes matching the default Consul Template usesConsulRetry.Attempts: defaults to 0 which is theunlimitedvalue - overrides the Consul Template default of 12ConsulRetry.Backoff: defaults to 250ms matching the default that Consul Template usesConsulRetry.MaxBackoff: defaults to 1 minute matching default that Consul Template usesVaultRetry.Attempts: defaults to 0 which is theunlimitedvalue - overrides the Consul Template default of 12VaultRetry.Backoff: defaults to 250ms matching the default that Consul Template usesVaultRetry.MaxBackoff: defaults to 1 minute matching default that Consul Template uses