Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

fix(defaults): Checks for set backoff and rate limit values #3890

Closed
thomastaylor312 wants to merge 1 commit into from
Closed

fix(defaults): Checks for set backoff and rate limit values #3890

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 21 additions & 18 deletions pkg/acsengine/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,30 +351,33 @@ func setOrchestratorDefaults(cs *api.ContainerService, isUpdate bool) {
o.KubernetesConfig.ServiceCIDR = DefaultKubernetesServiceCIDR
}
// Enforce sane cloudprovider backoff defaults, if CloudProviderBackoff is true in KubernetesConfig
o.KubernetesConfig.CloudProviderBackoff = true
if o.KubernetesConfig.CloudProviderBackoffDuration == 0 {
o.KubernetesConfig.CloudProviderBackoffDuration = DefaultKubernetesCloudProviderBackoffDuration
}
if o.KubernetesConfig.CloudProviderBackoffExponent == 0 {
o.KubernetesConfig.CloudProviderBackoffExponent = DefaultKubernetesCloudProviderBackoffExponent
}
if o.KubernetesConfig.CloudProviderBackoffJitter == 0 {
o.KubernetesConfig.CloudProviderBackoffJitter = DefaultKubernetesCloudProviderBackoffJitter
}
if o.KubernetesConfig.CloudProviderBackoffRetries == 0 {
o.KubernetesConfig.CloudProviderBackoffRetries = DefaultKubernetesCloudProviderBackoffRetries
if o.KubernetesConfig.CloudProviderBackoff {

@CecileRobertMichon CecileRobertMichon Sep 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

where is the default for CloudProviderBackoff being set if it's nil?

@thomastaylor312 thomastaylor312 Sep 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is a bool, so the zero value is false

@CecileRobertMichon CecileRobertMichon Sep 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@thomastaylor312 thomastaylor312 Sep 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.CloudProviderBackoffDuration == 0 {
o.KubernetesConfig.CloudProviderBackoffDuration = DefaultKubernetesCloudProviderBackoffDuration
}
if o.KubernetesConfig.CloudProviderBackoffExponent == 0 {
o.KubernetesConfig.CloudProviderBackoffExponent = DefaultKubernetesCloudProviderBackoffExponent
}
if o.KubernetesConfig.CloudProviderBackoffJitter == 0 {
o.KubernetesConfig.CloudProviderBackoffJitter = DefaultKubernetesCloudProviderBackoffJitter
}
if o.KubernetesConfig.CloudProviderBackoffRetries == 0 {
o.KubernetesConfig.CloudProviderBackoffRetries = DefaultKubernetesCloudProviderBackoffRetries
}
}

k8sSemVer, _ := semver.Make(k8sVersion)
minVersion, _ := semver.Make("1.6.6")
// Enforce sane cloudprovider rate limit defaults, if CloudProviderRateLimit is true in KubernetesConfig
// For k8s version greater or equal to 1.6.6, we will set the default CloudProviderRate* settings
o.KubernetesConfig.CloudProviderRateLimit = true
if o.KubernetesConfig.CloudProviderRateLimit && k8sSemVer.GTE(minVersion) {
if o.KubernetesConfig.CloudProviderRateLimitQPS == 0 {
o.KubernetesConfig.CloudProviderRateLimitQPS = DefaultKubernetesCloudProviderRateLimitQPS
}
if o.KubernetesConfig.CloudProviderRateLimitBucket == 0 {
o.KubernetesConfig.CloudProviderRateLimitBucket = DefaultKubernetesCloudProviderRateLimitBucket
if o.KubernetesConfig.CloudProviderRateLimit && k8sSemVer.GTE(minVersion) {

@CecileRobertMichon CecileRobertMichon Sep 21, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same question with o.KubernetesConfig.CloudProviderRateLimit

if o.KubernetesConfig.CloudProviderRateLimitQPS == 0 {
o.KubernetesConfig.CloudProviderRateLimitQPS = DefaultKubernetesCloudProviderRateLimitQPS
}
if o.KubernetesConfig.CloudProviderRateLimitBucket == 0 {

@tariq1890 tariq1890 Sep 21, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@thomastaylor312 thomastaylor312 Sep 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These were pre-existing. I do think that would be a good idea though.

o.KubernetesConfig.CloudProviderRateLimitBucket = DefaultKubernetesCloudProviderRateLimitBucket
}
}
}

Expand Down