Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

feat: disabling windows updates by default #3073

Merged

Conversation

marosset
Copy link
Contributor

@marosset marosset commented Apr 15, 2020

Reason for Change:

Enabling automatic updates on Windows nodes has caused problems for cluster operators in the past and goes against guidance provided by the aks-engine team so it should not be enabled by default in aks-engine

Issue Fixed:

Requirements:

Notes:

@marosset marosset requested review from aadishjain2212, jackfrancis and ksubrmnn and removed request for aadishjain2212 April 15, 2020 21:10
ksubrmnn
ksubrmnn previously approved these changes Apr 15, 2020
@@ -223,7 +223,7 @@ const (
// https://docs.microsoft.com/en-us/azure/azure-subscription-service-limits#load-balancer.
DefaultMaximumLoadBalancerRuleCount = 250
// DefaultEnableAutomaticUpdates determines the aks-engine provided default for enabling automatic updates
DefaultEnableAutomaticUpdates = true
DefaultEnableAutomaticUpdates = false
Copy link
Member

Choose a reason for hiding this comment

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

For some reason I thought this already defaulted to false, so lgtm

PS this was being unit tested :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooo - i'll update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One funny thing is that we disable windows updates in the aks-engine VHDs we produce in the windows registry. I thought this was defaulting to false too.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's why I thought we have been doing this for a long time. I still think this change is good for folks who choose not to use the VHD.

@codecov
Copy link

codecov bot commented Apr 16, 2020

Codecov Report

Merging #3073 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3073      +/-   ##
==========================================
- Coverage   70.63%   70.58%   -0.05%     
==========================================
  Files         145      145              
  Lines       25151    25219      +68     
==========================================
+ Hits        17765    17802      +37     
- Misses       6283     6312      +29     
- Partials     1103     1105       +2     
Impacted Files Coverage Δ
pkg/engine/availabilitysets.go 87.75% <0.00%> (-12.25%) ⬇️
cmd/upgrade.go 45.25% <0.00%> (-1.85%) ⬇️
pkg/engine/templates_generated.go 30.99% <0.00%> (-0.59%) ⬇️
pkg/engine/virtualmachinescalesets.go 80.79% <0.00%> (-0.33%) ⬇️
pkg/engine/engine.go 86.31% <0.00%> (-0.28%) ⬇️
pkg/engine/cse.go 100.00% <0.00%> (ø)
pkg/engine/artifacts.go 100.00% <0.00%> (ø)
pkg/api/defaults-cloud-controller-manager.go 92.85% <0.00%> (ø)
pkg/api/addons.go 97.74% <0.00%> (+<0.01%) ⬆️
pkg/api/convertertoapi.go 93.65% <0.00%> (+0.02%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8fb97a2...e264d97. Read the comment docs.

@ksubrmnn ksubrmnn merged commit e959456 into Azure:master Apr 17, 2020
alexeldeib pushed a commit to alexeldeib/aks-engine that referenced this pull request Apr 21, 2020
* feat: disabling windows updates by default
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants