fix(defaults): Checks for set backoff and rate limit values#3890
fix(defaults): Checks for set backoff and rate limit values#3890thomastaylor312 wants to merge 1 commit intoAzure:masterfrom
Conversation
…faulting The defaults file was always setting rate limits and backoff to true for the azure cloud config. This checks if the value exists before setting defaults
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: thomastaylor312 If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| } | ||
| if o.KubernetesConfig.CloudProviderBackoffRetries == 0 { | ||
| o.KubernetesConfig.CloudProviderBackoffRetries = DefaultKubernetesCloudProviderBackoffRetries | ||
| if o.KubernetesConfig.CloudProviderBackoff { |
There was a problem hiding this comment.
where is the default for CloudProviderBackoff being set if it's nil?
There was a problem hiding this comment.
It is a bool, so the zero value is false
There was a problem hiding this comment.
IMO we should still set it to the default constant https://github.com/Azure/acs-engine/blob/master/pkg/acsengine/const.go#L82 in case we ever decide to change that default
There was a problem hiding this comment.
I agree, but that probably shouldn't be set here. It could be the user has set it to false or that it defaulted to false
| } | ||
| if o.KubernetesConfig.CloudProviderRateLimitBucket == 0 { | ||
| o.KubernetesConfig.CloudProviderRateLimitBucket = DefaultKubernetesCloudProviderRateLimitBucket | ||
| if o.KubernetesConfig.CloudProviderRateLimit && k8sSemVer.GTE(minVersion) { |
There was a problem hiding this comment.
same question with o.KubernetesConfig.CloudProviderRateLimit
| if o.KubernetesConfig.CloudProviderRateLimitQPS == 0 { | ||
| o.KubernetesConfig.CloudProviderRateLimitQPS = DefaultKubernetesCloudProviderRateLimitQPS | ||
| } | ||
| if o.KubernetesConfig.CloudProviderRateLimitBucket == 0 { |
There was a problem hiding this comment.
Could we have a helper method to get the values for these? The helper method would return a default value is the said field has nil or its equivalent as its value.
There was a problem hiding this comment.
These were pre-existing. I do think that would be a good idea though.
Codecov Report
@@ Coverage Diff @@
## master #3890 +/- ##
==========================================
- Coverage 56.43% 56.32% -0.11%
==========================================
Files 109 109
Lines 16569 16569
==========================================
- Hits 9350 9333 -17
- Misses 6414 6428 +14
- Partials 805 808 +3 |
|
@jackfrancis put together a more fully featured PR in #3891 that addresses this. Closing. |
4 participants
What this PR does / why we need it:
The defaults file was always setting rate limits and backoff to true for the azure cloud config. This checks if the value exists before setting defaults
Release note: