Repository: kubernetes/kops (go)

Issue:
Bootstrap channel DaemonSets have updateStrategy: RollingUpdate
**1. What `kops` version are you running? The command `kops version`, will display
 this information.**

Version 1.14.1 (git-b7c25f9a9)

**2. What Kubernetes version are you running? `kubectl version` will print the
 version if a cluster is running or provide the Kubernetes version specified as
 a `kops` flag.**

Client Version: version.Info{Major:"1", Minor:"16", GitVersion:"v1.16.0", GitCommit:"2bd9643cee5b3b3a5ecbd3af49d09018f0773c77", GitTreeState:"archive", BuildDate:"2019-09-21T09:43:39Z", GoVersion:"go1.12.8", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"12", GitVersion:"v1.12.10", GitCommit:"e3c134023df5dea457638b614ee17ef234dc34a6", GitTreeState:"clean", BuildDate:"2019-07-08T03:40:54Z", GoVersion:"go1.10.8", Compiler:"gc", Platform:"linux/amd64"}

**3. What cloud provider are you using?**

AWS

**4. What commands did you run?  What is the simplest way to reproduce this issue?**

`kops update cluster --target cloudformation --out /tmp` using kops 1.14.1, on a cluster using weave for networking that was last updated with kops 1.12.3.

**5. What happened after the commands executed?**

As soon as the new version of weave was installed, the cluster initiated a rolling restart of the weave pods, which caused a brief but visible network interruption on each node.

**6. What did you expect to happen?**

I would not expect any weave pods to be replaced until I ask kops to perform a rolling update, given that they are essential for the pods on a node to function correctly.

**7. Please provide your cluster manifest. Execute
  `kops get --name my.example.com -o yaml` to display your cluster manifest.
  You may want to remove your cluster name and other sensitive information.**

https://gist.github.com/stevenjm/c7eccd8b3fc20c675df61dfa3d6e9ecc

**9. Anything else do we need to know?**

In the case of weave, this is an [explicit setting](https://github.com/kubernetes/kops/blob/1.14.1/upup/models/cloudup/resources/addons/networking.weave/k8s-1.12.yaml.template#L257) that appears to have been inherited from upstream. I suspect this makes sense for weave upstream, as they can't assume the cluster is managed in such a way that nodes can easily be updated in place, but kops allows us to make this assumption and set updatePolicy to OnDelete. This will let a `kops rolling-update` replace these pods for us.

For this to work correctly, the node's user data must include a checksum of the bootstrap channel so that kops knows that nodes needs to be replaced when weave is updated. See also my suggested approach to fixing https://github.com/kubernetes/kops/issues/7970.

Pull request description:
Make it possible to use OnDelete update strategy on addon daemonset
Fixes #7971

Or at least makes it possible to fix it for the addons we need.

Works by adding another element to the addon spec. If it is set, will make channels patch an annotation on the relevant nodes that rolling update uses to determine if an instance needs updating.

Patch:
diff --git a/channels/pkg/api/channel.go b/channels/pkg/api/channel.go
index 32eb826643..41e1a6397f 100644
--- a/channels/pkg/api/channel.go
+++ b/channels/pkg/api/channel.go
@@ -62,6 +62,11 @@ type AddonSpec struct {
 	// version of the software we are packaging.  But we always want to reinstall when we
 	// switch kubernetes versions.
 	Id string `json:"id,omitempty"`
+
+	// NeedsRollingUpdate determines if we should mark nodes as needing an update.
+	// Legal values are control-plane, workers, and all
+	// Empty value means no update needed
+	NeedsRollingUpdate string `json:"needsRollingUpdate,omitempty"`
 }
 
 func (a *Addons) Verify() error {
diff --git a/channels/pkg/channels/addon.go b/channels/pkg/channels/addon.go
index 327cf479ef..c3dada651f 100644
--- a/channels/pkg/channels/addon.go
+++ b/channels/pkg/channels/addon.go
@@ -18,13 +18,18 @@ package channels
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"net/url"
 
+	"k8s.io/apimachinery/pkg/types"
+
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/klog/v2"
 	"k8s.io/kops/channels/pkg/api"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 // Addon is a wrapper around a single version of an addon
@@ -144,6 +149,13 @@ func (a *Addon) EnsureUpdated(ctx context.Context, k8sClient kubernetes.Interfac
 		return nil, fmt.Errorf("error applying update from %q: %v", manifestURL, err)
 	}
 
+	if a.Spec.NeedsRollingUpdate != "" {
+		err = a.AddNeedsUpdateLabel(ctx, k8sClient)
+		if err != nil {
+			return nil, fmt.Errorf("error adding needs-update label: %v", err)
+		}
+	}
+
 	channel := a.buildChannel()
 	err = channel.SetInstalledVersion(ctx, k8sClient, a.ChannelVersion())
 	if err != nil {
@@ -152,3 +164,36 @@ func (a *Addon) EnsureUpdated(ctx context.Context, k8sClient kubernetes.Interfac
 
 	return required, nil
 }
+
+func (a *Addon) AddNeedsUpdateLabel(ctx context.Context, k8sClient kubernetes.Interface) error {
+	klog.Infof("addon %v wants to update %v nodes", a.Name, a.Spec.NeedsRollingUpdate)
+	selector := ""
+	switch a.Spec.NeedsRollingUpdate {
+	case "control-plane":
+		selector = "node-role.kubernetes.io/master="
+	case "worker":
+		selector = "node-role.kubernetes.io/node="
+	}
+
+	annotationPatch := &annotationPatch{Metadata: annotationPatchMetadata{Annotations: map[string]string{
+		"kops.k8s.io/needs-update": "",
+	}}}
+	annotationPatchJSON, err := json.Marshal(annotationPatch)
+	if err != nil {
+		return err
+	}
+
+	nodeInterface := k8sClient.CoreV1().Nodes()
+	nodes, err := nodeInterface.List(ctx, metav1.ListOptions{LabelSelector: selector})
+	if err != nil {
+		return err
+	}
+	for _, node := range nodes.Items {
+		_, err = nodeInterface.Patch(ctx, node.Name, types.StrategicMergePatchType, annotationPatchJSON, metav1.PatchOptions{})
+
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
diff --git a/cmd/kops/get_instances.go b/cmd/kops/get_instances.go
index 1e7234c154..92b0553af6 100644
--- a/cmd/kops/get_instances.go
+++ b/cmd/kops/get_instances.go
@@ -126,6 +126,7 @@ func RunGetInstances(ctx context.Context, f *util.Factory, out io.Writer, option
 	var cloudInstances []*cloudinstances.CloudInstance
 
 	cloudGroups, err := cloud.GetCloudGroups(cluster, instanceGroups, false, nodeList.Items)
+
 	if err != nil {
 		return err
 	}
@@ -133,6 +134,7 @@ func RunGetInstances(ctx context.Context, f *util.Factory, out io.Writer, option
 	for _, cg := range cloudGroups {
 		cloudInstances = append(cloudInstances, cg.Ready...)
 		cloudInstances = append(cloudInstances, cg.NeedUpdate...)
+		cg.AdjustNeedUpdate()
 	}
 
 	switch options.output {
diff --git a/cmd/kops/rollingupdatecluster.go b/cmd/kops/rollingupdatecluster.go
index 567bf85013..84aa3ce237 100644
--- a/cmd/kops/rollingupdatecluster.go
+++ b/cmd/kops/rollingupdatecluster.go
@@ -350,7 +350,7 @@ func RunRollingUpdateCluster(ctx context.Context, f *util.Factory, out io.Writer
 		ValidateSuccessDuration: 10 * time.Second,
 	}
 
-	err = d.AdjustNeedUpdate(groups, list)
+	err = d.AdjustNeedUpdate(groups)
 	if err != nil {
 		return err
 	}
diff --git a/docs/cluster_spec.md b/docs/cluster_spec.md
index 4ae5ee2cb7..530ae1d82c 100644
--- a/docs/cluster_spec.md
+++ b/docs/cluster_spec.md
@@ -663,8 +663,6 @@ spec:
     horizontalPodAutoscalerDownscaleDelay: 5m0s
     horizontalPodAutoscalerDownscaleStabilization: 5m
     horizontalPodAutoscalerUpscaleDelay: 3m0s
-    horizontalPodAutoscalerInitialReadinessDelay: 30s
-    horizontalPodAutoscalerCpuInitializationPeriod: 5m
     horizontalPodAutoscalerTolerance: 0.1
     experimentalClusterSigningDuration: 8760h0m0s
     enableProfiling: false
diff --git a/k8s/crds/kops.k8s.io_clusters.yaml b/k8s/crds/kops.k8s.io_clusters.yaml
index 7cb1993089..74e79c63e7 100644
--- a/k8s/crds/kops.k8s.io_clusters.yaml
+++ b/k8s/crds/kops.k8s.io_clusters.yaml
@@ -653,7 +653,7 @@ spec:
                       description: 'Provider is the provider used to run etcd: Manager, Legacy. Defaults to Manager.'
                       type: string
                     version:
-                      description: Version is the version of etcd to run.
+                      description: Version is the version of etcd to run i.e. 2.1.2, 3.0.17 etcd
                       type: string
                   type: object
                 type: array
@@ -1216,18 +1216,12 @@ spec:
                       type: string
                     description: FeatureGates is set of key=value pairs that describe feature gates for alpha/experimental features.
                     type: object
-                  horizontalPodAutoscalerCpuInitializationPeriod:
-                    description: HorizontalPodAutoscalerCPUInitializationPeriod is the period after pod start when CPU samples might be skipped. (default 5m)
-                    type: string
                   horizontalPodAutoscalerDownscaleDelay:
                     description: HorizontalPodAutoscalerDownscaleDelay is a duration that specifies how long the autoscaler has to wait before another downscale operation can be performed after the current one has completed.
                     type: string
                   horizontalPodAutoscalerDownscaleStabilization:
                     description: HorizontalPodAutoscalerDownscaleStabilization is the period for which autoscaler will look backwards and not scale down below any recommendation it made during that period.
                     type: string
-                  horizontalPodAutoscalerInitialReadinessDelay:
-                    description: HorizontalPodAutoscalerInitialReadinessDelay is the period after pod start during which readiness changes will be treated as initial readiness. (default 30s)
-                    type: string
                   horizontalPodAutoscalerSyncPeriod:
                     description: HorizontalPodAutoscalerSyncPeriod is the amount of time between syncs During each period, the controller manager queries the resource utilization against the metrics specified in each HorizontalPodAutoscaler definition.
                     type: string
diff --git a/pkg/apis/kops/cluster.go b/pkg/apis/kops/cluster.go
index 868bae5f32..f38bfe4fc6 100644
--- a/pkg/apis/kops/cluster.go
+++ b/pkg/apis/kops/cluster.go
@@ -477,7 +477,7 @@ type EtcdClusterSpec struct {
 	EnableEtcdTLS bool `json:"enableEtcdTLS,omitempty"`
 	// EnableTLSAuth indicates client and peer TLS auth should be enforced
 	EnableTLSAuth bool `json:"enableTLSAuth,omitempty"`
-	// Version is the version of etcd to run.
+	// Version is the version of etcd to run i.e. 2.1.2, 3.0.17 etcd
 	Version string `json:"version,omitempty"`
 	// LeaderElectionTimeout is the time (in milliseconds) for an etcd leader election timeout
 	LeaderElectionTimeout *metav1.Duration `json:"leaderElectionTimeout,omitempty"`
diff --git a/pkg/apis/kops/componentconfig.go b/pkg/apis/kops/componentconfig.go
index 74b6dbce7c..d5bcacfc87 100644
--- a/pkg/apis/kops/componentconfig.go
+++ b/pkg/apis/kops/componentconfig.go
@@ -551,12 +551,6 @@ type KubeControllerManagerConfig struct {
 	// long the autoscaler has to wait before another upscale operation can
 	// be performed after the current one has completed.
 	HorizontalPodAutoscalerUpscaleDelay *metav1.Duration `json:"horizontalPodAutoscalerUpscaleDelay,omitempty" flag:"horizontal-pod-autoscaler-upscale-delay"`
-	// HorizontalPodAutoscalerInitialReadinessDelay is the period after pod start
-	// during which readiness changes will be treated as initial readiness. (default 30s)
-	HorizontalPodAutoscalerInitialReadinessDelay *metav1.Duration `json:"horizontalPodAutoscalerInitialReadinessDelay,omitempty" flag:"horizontal-pod-autoscaler-initial-readiness-delay"`
-	// HorizontalPodAutoscalerCPUInitializationPeriod is the period after pod start
-	// when CPU samples might be skipped. (default 5m)
-	HorizontalPodAutoscalerCPUInitializationPeriod *metav1.Duration `json:"horizontalPodAutoscalerCpuInitializationPeriod,omitempty" flag:"horizontal-pod-autoscaler-cpu-initialization-period"`
 	// HorizontalPodAutoscalerTolerance is the minimum change (from 1.0) in the
 	// desired-to-actual metrics ratio for the horizontal pod autoscaler to
 	// consider scaling.
diff --git a/pkg/apis/kops/v1alpha2/cluster.go b/pkg/apis/kops/v1alpha2/cluster.go
index 0bef5ebf16..d0ff0d5d49 100644
--- a/pkg/apis/kops/v1alpha2/cluster.go
+++ b/pkg/apis/kops/v1alpha2/cluster.go
@@ -474,7 +474,7 @@ type EtcdClusterSpec struct {
 	EnableEtcdTLS bool `json:"enableEtcdTLS,omitempty"`
 	// EnableTLSAuth indicates client and peer TLS auth should be enforced
 	EnableTLSAuth bool `json:"enableTLSAuth,omitempty"`
-	// Version is the version of etcd to run.
+	// Version is the version of etcd to run i.e. 2.1.2, 3.0.17 etcd
 	Version string `json:"version,omitempty"`
 	// LeaderElectionTimeout is the time (in milliseconds) for an etcd leader election timeout
 	LeaderElectionTimeout *metav1.Duration `json:"leaderElectionTimeout,omitempty"`
diff --git a/pkg/apis/kops/v1alpha2/componentconfig.go b/pkg/apis/kops/v1alpha2/componentconfig.go
index fca173b5bd..f9c4252e7d 100644
--- a/pkg/apis/kops/v1alpha2/componentconfig.go
+++ b/pkg/apis/kops/v1alpha2/componentconfig.go
@@ -551,12 +551,6 @@ type KubeControllerManagerConfig struct {
 	// long the autoscaler has to wait before another upscale operation can
 	// be performed after the current one has completed.
 	HorizontalPodAutoscalerUpscaleDelay *metav1.Duration `json:"horizontalPodAutoscalerUpscaleDelay,omitempty" flag:"horizontal-pod-autoscaler-upscale-delay"`
-	// HorizontalPodAutoscalerInitialReadinessDelay is the period after pod start
-	// during which readiness changes will be treated as initial readiness. (default 30s)
-	HorizontalPodAutoscalerInitialReadinessDelay *metav1.Duration `json:"horizontalPodAutoscalerInitialReadinessDelay,omitempty" flag:"horizontal-pod-autoscaler-initial-readiness-delay"`
-	// HorizontalPodAutoscalerCPUInitializationPeriod is the period after pod start
-	// when CPU samples might be skipped. (default 5m)
-	HorizontalPodAutoscalerCPUInitializationPeriod *metav1.Duration `json:"horizontalPodAutoscalerCpuInitializationPeriod,omitempty" flag:"horizontal-pod-autoscaler-cpu-initialization-period"`
 	// HorizontalPodAutoscalerTolerance is the minimum change (from 1.0) in the
 	// desired-to-actual metrics ratio for the horizontal pod autoscaler to
 	// consider scaling.
diff --git a/pkg/apis/kops/v1alpha2/zz_generated.conversion.go b/pkg/apis/kops/v1alpha2/zz_generated.conversion.go
index 18bbc9447c..be54ebc673 100644
--- a/pkg/apis/kops/v1alpha2/zz_generated.conversion.go
+++ b/pkg/apis/kops/v1alpha2/zz_generated.conversion.go
@@ -4138,8 +4138,6 @@ func autoConvert_v1alpha2_KubeControllerManagerConfig_To_kops_KubeControllerMana
 	out.HorizontalPodAutoscalerDownscaleDelay = in.HorizontalPodAutoscalerDownscaleDelay
 	out.HorizontalPodAutoscalerDownscaleStabilization = in.HorizontalPodAutoscalerDownscaleStabilization
 	out.HorizontalPodAutoscalerUpscaleDelay = in.HorizontalPodAutoscalerUpscaleDelay
-	out.HorizontalPodAutoscalerInitialReadinessDelay = in.HorizontalPodAutoscalerInitialReadinessDelay
-	out.HorizontalPodAutoscalerCPUInitializationPeriod = in.HorizontalPodAutoscalerCPUInitializationPeriod
 	out.HorizontalPodAutoscalerTolerance = in.HorizontalPodAutoscalerTolerance
 	out.HorizontalPodAutoscalerUseRestClients = in.HorizontalPodAutoscalerUseRestClients
 	out.ExperimentalClusterSigningDuration = in.ExperimentalClusterSigningDuration
@@ -4203,8 +4201,6 @@ func autoConvert_kops_KubeControllerManagerConfig_To_v1alpha2_KubeControllerMana
 	out.HorizontalPodAutoscalerDownscaleDelay = in.HorizontalPodAutoscalerDownscaleDelay
 	out.HorizontalPodAutoscalerDownscaleStabilization = in.HorizontalPodAutoscalerDownscaleStabilization
 	out.HorizontalPodAutoscalerUpscaleDelay = in.HorizontalPodAutoscalerUpscaleDelay
-	out.HorizontalPodAutoscalerInitialReadinessDelay = in.HorizontalPodAutoscalerInitialReadinessDelay
-	out.HorizontalPodAutoscalerCPUInitializationPeriod = in.HorizontalPodAutoscalerCPUInitializationPeriod
 	out.HorizontalPodAutoscalerTolerance = in.HorizontalPodAutoscalerTolerance
 	out.HorizontalPodAutoscalerUseRestClients = in.HorizontalPodAutoscalerUseRestClients
 	out.ExperimentalClusterSigningDuration = in.ExperimentalClusterSigningDuration
diff --git a/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go b/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go
index 8c815e8899..7818d80459 100644
--- a/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go
+++ b/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go
@@ -2505,16 +2505,6 @@ func (in *KubeControllerManagerConfig) DeepCopyInto(out *KubeControllerManagerCo
 		*out = new(v1.Duration)
 		**out = **in
 	}
-	if in.HorizontalPodAutoscalerInitialReadinessDelay != nil {
-		in, out := &in.HorizontalPodAutoscalerInitialReadinessDelay, &out.HorizontalPodAutoscalerInitialReadinessDelay
-		*out = new(v1.Duration)
-		**out = **in
-	}
-	if in.HorizontalPodAutoscalerCPUInitializationPeriod != nil {
-		in, out := &in.HorizontalPodAutoscalerCPUInitializationPeriod, &out.HorizontalPodAutoscalerCPUInitializationPeriod
-		*out = new(v1.Duration)
-		**out = **in
-	}
 	if in.HorizontalPodAutoscalerTolerance != nil {
 		in, out := &in.HorizontalPodAutoscalerTolerance, &out.HorizontalPodAutoscalerTolerance
 		x := (*in).DeepCopy()
diff --git a/pkg/apis/kops/zz_generated.deepcopy.go b/pkg/apis/kops/zz_generated.deepcopy.go
index 2b6c2130f8..2e6bd12af2 100644
--- a/pkg/apis/kops/zz_generated.deepcopy.go
+++ b/pkg/apis/kops/zz_generated.deepcopy.go
@@ -2687,16 +2687,6 @@ func (in *KubeControllerManagerConfig) DeepCopyInto(out *KubeControllerManagerCo
 		*out = new(v1.Duration)
 		**out = **in
 	}
-	if in.HorizontalPodAutoscalerInitialReadinessDelay != nil {
-		in, out := &in.HorizontalPodAutoscalerInitialReadinessDelay, &out.HorizontalPodAutoscalerInitialReadinessDelay
-		*out = new(v1.Duration)
-		**out = **in
-	}
-	if in.HorizontalPodAutoscalerCPUInitializationPeriod != nil {
-		in, out := &in.HorizontalPodAutoscalerCPUInitializationPeriod, &out.HorizontalPodAutoscalerCPUInitializationPeriod
-		*out = new(v1.Duration)
-		**out = **in
-	}
 	if in.HorizontalPodAutoscalerTolerance != nil {
 		in, out := &in.HorizontalPodAutoscalerTolerance, &out.HorizontalPodAutoscalerTolerance
 		x := (*in).DeepCopy()
diff --git a/pkg/cloudinstances/cloud_instance_group.go b/pkg/cloudinstances/cloud_instance_group.go
index 341d8faa63..4f60c30ad4 100644
--- a/pkg/cloudinstances/cloud_instance_group.go
+++ b/pkg/cloudinstances/cloud_instance_group.go
@@ -75,6 +75,29 @@ func (c *CloudInstanceGroup) Status() string {
 	return "NeedsUpdate"
 }
 
+func (group *CloudInstanceGroup) AdjustNeedUpdate() {
+
+	if group.Ready != nil {
+		var newReady []*CloudInstance
+		for _, member := range group.Ready {
+			makeNotReady := false
+			if member.Node != nil && member.Node.Annotations != nil {
+				if _, ok := member.Node.Annotations["kops.k8s.io/needs-update"]; ok {
+					makeNotReady = true
+				}
+			}
+
+			if makeNotReady {
+				group.NeedUpdate = append(group.NeedUpdate, member)
+				member.Status = CloudInstanceStatusNeedsUpdate
+			} else {
+				newReady = append(newReady, member)
+			}
+		}
+		group.Ready = newReady
+	}
+}
+
 // GetNodeMap returns a list of nodes keyed by their external id
 func GetNodeMap(nodes []v1.Node, cluster *kopsapi.Cluster) map[string]*v1.Node {
 	nodeMap := make(map[string]*v1.Node)
diff --git a/pkg/instancegroups/instancegroups.go b/pkg/instancegroups/instancegroups.go
index 8c5d5c2ab2..1a3a8518a8 100644
--- a/pkg/instancegroups/instancegroups.go
+++ b/pkg/instancegroups/instancegroups.go
@@ -491,8 +491,9 @@ func (c *RollingUpdateCluster) validateClusterWithTimeout(validateCount int, gro
 func hasFailureRelevantToGroup(failures []*validation.ValidationError, group *cloudinstances.CloudInstanceGroup) bool {
 	// Ignore non critical validation errors in other instance groups like below target size errors
 	for _, failure := range failures {
-		// Certain failures like a system-critical-pod failure and dns server related failures
-		// set their InstanceGroup to nil, since we cannot associate the failure to any one group
+		// Determining InstanceGroups for certain resources like Pods, ComponentStatus is not straightforward.
+		// Till we are able to determine the InstanceGroups for these resources without ambiguity, the
+		// InstanceGroup field of the ValidationErrors for these resources will be nil
 		if failure.InstanceGroup == nil {
 			return true
 		}
diff --git a/pkg/instancegroups/rollingupdate.go b/pkg/instancegroups/rollingupdate.go
index 70a6366ad6..01a4f0b45d 100644
--- a/pkg/instancegroups/rollingupdate.go
+++ b/pkg/instancegroups/rollingupdate.go
@@ -80,27 +80,9 @@ type RollingUpdateCluster struct {
 }
 
 // AdjustNeedUpdate adjusts the set of instances that need updating, using factors outside those known by the cloud implementation
-func (c *RollingUpdateCluster) AdjustNeedUpdate(groups map[string]*cloudinstances.CloudInstanceGroup, instanceGroups *api.InstanceGroupList) error {
+func (*RollingUpdateCluster) AdjustNeedUpdate(groups map[string]*cloudinstances.CloudInstanceGroup) error {
 	for _, group := range groups {
-		if group.Ready != nil {
-			var newReady []*cloudinstances.CloudInstance
-			for _, member := range group.Ready {
-				makeNotReady := false
-				if member.Node != nil && member.Node.Annotations != nil {
-					if _, ok := member.Node.Annotations["kops.k8s.io/needs-update"]; ok {
-						makeNotReady = true
-					}
-				}
-
-				if makeNotReady {
-					group.NeedUpdate = append(group.NeedUpdate, member)
-					member.Status = cloudinstances.CloudInstanceStatusNeedsUpdate
-				} else {
-					newReady = append(newReady, member)
-				}
-			}
-			group.Ready = newReady
-		}
+		group.AdjustNeedUpdate()
 	}
 	return nil
 }
diff --git a/pkg/model/components/apiserver.go b/pkg/model/components/apiserver.go
index 6a79ba19ac..3b95304394 100644
--- a/pkg/model/components/apiserver.go
+++ b/pkg/model/components/apiserver.go
@@ -96,6 +96,39 @@ func (b *KubeAPIServerOptionsBuilder) BuildOptions(o interface{}) error {
 		clusterSpec.KubeAPIServer.AuthorizationMode = fi.String(strings.Join(modes, ","))
 	}
 
+	if clusterSpec.KubeAPIServer.EtcdQuorumRead == nil {
+		// 1.9 changed etcd-quorum-reads default to true
+		// There's a balance between some bugs which are attributed to not having etcd-quorum-reads,
+		// and the poor implementation of quorum-reads in etcd2.
+
+		etcdHA := false
+		etcdV2 := true
+		for _, c := range clusterSpec.EtcdClusters {
+			if len(c.Members) > 1 {
+				etcdHA = true
+			}
+			if c.Version != "" && !strings.HasPrefix(c.Version, "2.") {
+				etcdV2 = false
+			}
+		}
+
+		if !etcdV2 {
+			// etcd3 quorum reads are cheap.  Stick with default (which is to enable quorum reads)
+			clusterSpec.KubeAPIServer.EtcdQuorumRead = nil
+		} else {
+			// etcd2 quorum reads go through raft => write to disk => expensive
+			if !etcdHA {
+				// Turn off quorum reads - they still go through raft, but don't serve any purpose in non-HA clusters.
+				clusterSpec.KubeAPIServer.EtcdQuorumRead = fi.Bool(false)
+			} else {
+				// The problematic case.  We risk exposing more bugs, but against that we have to balance performance.
+				// For now we turn off quorum reads - it's a bad enough performance regression
+				// We'll likely make this default to true once we can set IOPS on the etcd volume and can easily upgrade to etcd3
+				clusterSpec.KubeAPIServer.EtcdQuorumRead = fi.Bool(false)
+			}
+		}
+	}
+
 	if err := b.configureAggregation(clusterSpec); err != nil {
 		return nil
 	}
diff --git a/pkg/validation/validate_cluster.go b/pkg/validation/validate_cluster.go
index dc66565fe6..817d8adce3 100644
--- a/pkg/validation/validate_cluster.go
+++ b/pkg/validation/validate_cluster.go
@@ -159,9 +159,9 @@ func (v *clusterValidatorImpl) Validate() (*ValidationCluster, error) {
 	if err != nil {
 		return nil, err
 	}
-	readyNodes, nodeInstanceGroupMapping := validation.validateNodes(cloudGroups, v.instanceGroups)
+	readyNodes := validation.validateNodes(cloudGroups, v.instanceGroups)
 
-	if err := validation.collectPodFailures(ctx, v.k8sClient, readyNodes, nodeInstanceGroupMapping); err != nil {
+	if err := validation.collectPodFailures(ctx, v.k8sClient, readyNodes, v.instanceGroups); err != nil {
 		return nil, fmt.Errorf("cannot get pod health for %q: %v", clusterName, err)
 	}
 
@@ -175,7 +175,7 @@ var masterStaticPods = []string{
 }
 
 func (v *ValidationCluster) collectPodFailures(ctx context.Context, client kubernetes.Interface, nodes []v1.Node,
-	nodeInstanceGroupMapping map[string]*kops.InstanceGroup) error {
+	groups []*kops.InstanceGroup) error {
 	masterWithoutPod := map[string]map[string]bool{}
 	nodeByAddress := map[string]string{}
 
@@ -187,7 +187,6 @@ func (v *ValidationCluster) collectPodFailures(ctx context.Context, client kuber
 				masterWithoutPod[node.Name][pod] = true
 			}
 		}
-
 		for _, nodeAddress := range node.Status.Addresses {
 			nodeByAddress[nodeAddress.Address] = node.Name
 		}
@@ -211,26 +210,20 @@ func (v *ValidationCluster) collectPodFailures(ctx context.Context, client kuber
 			return nil
 		}
 
-		var podNode *kops.InstanceGroup
-		if priority == "system-node-critical" {
-			podNode = nodeInstanceGroupMapping[nodeByAddress[pod.Status.HostIP]]
-		}
-
+		// TODO: Add logic to figure out the InstanceGroup given a pod
 		if pod.Status.Phase == v1.PodPending {
 			v.addError(&ValidationError{
-				Kind:          "Pod",
-				Name:          pod.Namespace + "/" + pod.Name,
-				Message:       fmt.Sprintf("%s pod %q is pending", priority, pod.Name),
-				InstanceGroup: podNode,
+				Kind:    "Pod",
+				Name:    pod.Namespace + "/" + pod.Name,
+				Message: fmt.Sprintf("%s pod %q is pending", priority, pod.Name),
 			})
 			return nil
 		}
 		if pod.Status.Phase == v1.PodUnknown {
 			v.addError(&ValidationError{
-				Kind:          "Pod",
-				Name:          pod.Namespace + "/" + pod.Name,
-				Message:       fmt.Sprintf("%s pod %q is unknown phase", priority, pod.Name),
-				InstanceGroup: podNode,
+				Kind:    "Pod",
+				Name:    pod.Namespace + "/" + pod.Name,
+				Message: fmt.Sprintf("%s pod %q is unknown phase", priority, pod.Name),
 			})
 			return nil
 		}
@@ -242,10 +235,9 @@ func (v *ValidationCluster) collectPodFailures(ctx context.Context, client kuber
 		}
 		if len(notready) != 0 {
 			v.addError(&ValidationError{
-				Kind:          "Pod",
-				Name:          pod.Namespace + "/" + pod.Name,
-				Message:       fmt.Sprintf("%s pod %q is not ready (%s)", priority, pod.Name, strings.Join(notready, ",")),
-				InstanceGroup: podNode,
+				Kind:    "Pod",
+				Name:    pod.Namespace + "/" + pod.Name,
+				Message: fmt.Sprintf("%s pod %q is not ready (%s)", priority, pod.Name, strings.Join(notready, ",")),
 			})
 
 		}
@@ -258,10 +250,9 @@ func (v *ValidationCluster) collectPodFailures(ctx context.Context, client kuber
 	for node, nodeMap := range masterWithoutPod {
 		for app := range nodeMap {
 			v.addError(&ValidationError{
-				Kind:          "Node",
-				Name:          node,
-				Message:       fmt.Sprintf("master %q is missing %s pod", node, app),
-				InstanceGroup: nodeInstanceGroupMapping[node],
+				Kind:    "Node",
+				Name:    node,
+				Message: fmt.Sprintf("master %q is missing %s pod", node, app),
 			})
 		}
 	}
@@ -269,10 +260,9 @@ func (v *ValidationCluster) collectPodFailures(ctx context.Context, client kuber
 	return nil
 }
 
-func (v *ValidationCluster) validateNodes(cloudGroups map[string]*cloudinstances.CloudInstanceGroup, groups []*kops.InstanceGroup) ([]v1.Node, map[string]*kops.InstanceGroup) {
+func (v *ValidationCluster) validateNodes(cloudGroups map[string]*cloudinstances.CloudInstanceGroup, groups []*kops.InstanceGroup) []v1.Node {
 	var readyNodes []v1.Node
 	groupsSeen := map[string]bool{}
-	nodeInstanceGroupMapping := map[string]*kops.InstanceGroup{}
 
 	for _, cloudGroup := range cloudGroups {
 		var allMembers []*cloudinstances.CloudInstance
@@ -319,8 +309,6 @@ func (v *ValidationCluster) validateNodes(cloudGroups map[string]*cloudinstances
 				continue
 			}
 
-			nodeInstanceGroupMapping[node.Name] = cloudGroup.InstanceGroup
-
 			role := strings.ToLower(string(cloudGroup.InstanceGroup.Spec.Role))
 			if role == "" {
 				role = "node"
@@ -381,5 +369,5 @@ func (v *ValidationCluster) validateNodes(cloudGroups map[string]*cloudinstances
 		}
 	}
 
-	return readyNodes, nodeInstanceGroupMapping
+	return readyNodes
 }
diff --git a/upup/models/bindata.go b/upup/models/bindata.go
index 54b6a79832..3e698ceffd 100644
--- a/upup/models/bindata.go
+++ b/upup/models/bindata.go
@@ -3889,9 +3889,7 @@ spec:
           secretName: cilium-ipsec-keys
 {{ end }}
   updateStrategy:
-    rollingUpdate:
-      maxUnavailable: 2
-    type: RollingUpdate
+    type: OnDelete
 ---
 apiVersion: apps/v1
 kind: Deployment
diff --git a/upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12-v1.8.yaml.template b/upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12-v1.8.yaml.template
index 03da0b1a99..24276fb492 100644
--- a/upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12-v1.8.yaml.template
+++ b/upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12-v1.8.yaml.template
@@ -685,9 +685,7 @@ spec:
           secretName: cilium-ipsec-keys
 {{ end }}
   updateStrategy:
-    rollingUpdate:
-      maxUnavailable: 2
-    type: RollingUpdate
+    type: OnDelete
 ---
 apiVersion: apps/v1
 kind: Deployment
diff --git a/upup/pkg/fi/cloudup/bootstrapchannelbuilder.go b/upup/pkg/fi/cloudup/bootstrapchannelbuilder.go
index 3aed99761f..faefc6cf32 100644
--- a/upup/pkg/fi/cloudup/bootstrapchannelbuilder.go
+++ b/upup/pkg/fi/cloudup/bootstrapchannelbuilder.go
@@ -877,11 +877,12 @@ func (b *BootstrapChannelBuilder) buildAddons(c *fi.ModelBuilderContext) (*chann
 				location := key + "/" + id + "-v1.8.yaml"
 
 				addons.Spec.Addons = append(addons.Spec.Addons, &channelsapi.AddonSpec{
-					Name:     fi.String(key),
-					Version:  fi.String(version),
-					Selector: networkingSelector,
-					Manifest: fi.String(location),
-					Id:       id,
+					Name:               fi.String(key),
+					Version:            fi.String(version),
+					Selector:           networkingSelector,
+					Manifest:           fi.String(location),
+					Id:                 id,
+					NeedsRollingUpdate: "all",
 				})
 			}
 		}


Test patch:
diff --git a/pkg/instancegroups/rollingupdate_test.go b/pkg/instancegroups/rollingupdate_test.go
index 441ec51a53..9cfa4edbff 100644
--- a/pkg/instancegroups/rollingupdate_test.go
+++ b/pkg/instancegroups/rollingupdate_test.go
@@ -736,7 +736,7 @@ func TestAddAnnotatedNodesToNeedsUpdate(t *testing.T) {
 	addNeedsUpdateAnnotation(groups["node-2"], "node-2a")
 	addNeedsUpdateAnnotation(groups["master-1"], "master-1b")
 
-	err := c.AdjustNeedUpdate(groups, &kopsapi.InstanceGroupList{})
+	err := c.AdjustNeedUpdate(groups)
 	assert.NoError(t, err, "AddAnnotatedNodesToGroups")
 
 	assertGroupNeedUpdate(t, groups, "node-1", "node-1a", "node-1b")
@@ -759,7 +759,7 @@ func TestAddAnnotatedNodesToNeedsUpdateCloudonly(t *testing.T) {
 	c.CloudOnly = true
 	c.ClusterValidator = &assertNotCalledClusterValidator{T: t}
 
-	err := c.AdjustNeedUpdate(groups, &kopsapi.InstanceGroupList{})
+	err := c.AdjustNeedUpdate(groups)
 	assert.NoError(t, err, "AddAnnotatedNodesToGroups")
 
 	assertGroupNeedUpdate(t, groups, "node-1", "node-1a", "node-1b")
@@ -776,7 +776,7 @@ func TestAddAnnotatedNodesToNeedsUpdateNodesMissing(t *testing.T) {
 	groups["node-1"].Ready[0].Node = nil
 	groups["node-1"].NeedUpdate[0].Node = nil
 
-	err := c.AdjustNeedUpdate(groups, &kopsapi.InstanceGroupList{})
+	err := c.AdjustNeedUpdate(groups)
 	assert.NoError(t, err, "AddAnnotatedNodesToGroups")
 }
 
diff --git a/pkg/validation/validate_cluster_test.go b/pkg/validation/validate_cluster_test.go
index f3ebf22324..7e622078d5 100644
--- a/pkg/validation/validate_cluster_test.go
+++ b/pkg/validation/validate_cluster_test.go
@@ -543,10 +543,9 @@ func Test_ValidateMasterStaticPods(t *testing.T) {
 			},
 		}...)
 		expectedFailures = append(expectedFailures, &ValidationError{
-			Kind:          "Node",
-			Name:          "master-1b",
-			Message:       "master \"master-1b\" is missing " + pod + " pod",
-			InstanceGroup: groups["node-1"].InstanceGroup,
+			Kind:    "Node",
+			Name:    "master-1b",
+			Message: "master \"master-1b\" is missing " + pod + " pod",
 		})
 	}
 
@@ -617,76 +616,6 @@ func Test_ValidateNoPodFailures(t *testing.T) {
 }
 
 func Test_ValidatePodFailure(t *testing.T) {
-	groups := make(map[string]*cloudinstances.CloudInstanceGroup)
-	groups["node-1"] = &cloudinstances.CloudInstanceGroup{
-		InstanceGroup: &kopsapi.InstanceGroup{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "node-1",
-			},
-			Spec: kopsapi.InstanceGroupSpec{
-				Role: kopsapi.InstanceGroupRoleNode,
-			},
-		},
-		MinSize:    1,
-		TargetSize: 1,
-		Ready: []*cloudinstances.CloudInstance{
-			{
-				ID: "i-00001",
-				Node: &v1.Node{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "node-1a",
-					},
-					Status: v1.NodeStatus{
-						Addresses: []v1.NodeAddress{
-							{
-								Address: "1.2.3.4",
-							},
-						},
-						Conditions: []v1.NodeCondition{
-							{Type: "Ready", Status: v1.ConditionTrue},
-						},
-					},
-				},
-			},
-			{
-				ID: "i-00001",
-				Node: &v1.Node{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "node-1b",
-					},
-					Status: v1.NodeStatus{
-						Addresses: []v1.NodeAddress{
-							{
-								Address: "5.6.7.8",
-							},
-						},
-						Conditions: []v1.NodeCondition{
-							{Type: "Ready", Status: v1.ConditionTrue},
-						},
-					},
-				},
-			},
-			{
-				ID: "i-00001",
-				Node: &v1.Node{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "node-1c",
-					},
-					Status: v1.NodeStatus{
-						Addresses: []v1.NodeAddress{
-							{
-								Address: "9.10.11.12",
-							},
-						},
-						Conditions: []v1.NodeCondition{
-							{Type: "Ready", Status: v1.ConditionTrue},
-						},
-					},
-				},
-			},
-		},
-	}
-
 	for _, tc := range []struct {
 		name     string
 		phase    v1.PodPhase
@@ -710,40 +639,30 @@ func Test_ValidatePodFailure(t *testing.T) {
 	} {
 		for _, priority := range []string{"node", "cluster"} {
 			for _, namespace := range []string{"kube-system", "otherNamespace"} {
-				for _, hostIp := range []string{"1.2.3.4", "5.6.7.8", "9.10.11.12"} {
-					t.Run(fmt.Sprintf("%s-%s-%s", tc.name, priority, namespace), func(t *testing.T) {
-						v, err := testValidate(t, groups, makePodList(
-							[]map[string]string{
-								{
-									"name":              "pod1",
-									"namespace":         namespace,
-									"priorityClassName": fmt.Sprintf("system-%s-critical", priority),
-									"ready":             "false",
-									"phase":             string(tc.phase),
-									"hostip":            hostIp,
-								},
+				t.Run(fmt.Sprintf("%s-%s-%s", tc.name, priority, namespace), func(t *testing.T) {
+					v, err := testValidate(t, nil, makePodList(
+						[]map[string]string{
+							{
+								"name":              "pod1",
+								"namespace":         namespace,
+								"priorityClassName": fmt.Sprintf("system-%s-critical", priority),
+								"ready":             "false",
+								"phase":             string(tc.phase),
 							},
-						))
-
-						var podInstanceGroup *kopsapi.InstanceGroup
-						if priority == "node" {
-							podInstanceGroup = groups["node-1"].InstanceGroup
-						}
-
-						expected := ValidationError{
-							Kind:          "Pod",
-							Name:          fmt.Sprintf("%s/pod1", namespace),
-							Message:       fmt.Sprintf("system-%s-critical pod \"pod1\" is %s", priority, tc.expected),
-							InstanceGroup: podInstanceGroup,
-						}
-
-						require.NoError(t, err)
-						if !assert.Len(t, v.Failures, 1) ||
-							!assert.Equal(t, &expected, v.Failures[0]) {
-							printDebug(t, v)
-						}
-					})
-				}
+						},
+					))
+					expected := ValidationError{
+						Kind:    "Pod",
+						Name:    fmt.Sprintf("%s/pod1", namespace),
+						Message: fmt.Sprintf("system-%s-critical pod \"pod1\" is %s", priority, tc.expected),
+					}
+
+					require.NoError(t, err)
+					if !assert.Len(t, v.Failures, 1) ||
+						!assert.Equal(t, &expected, v.Failures[0]) {
+						printDebug(t, v)
+					}
+				})
 			}
 		}
 	}
diff --git a/upup/pkg/fi/cloudup/tests/bootstrapchannelbuilder/cilium/manifest.yaml b/upup/pkg/fi/cloudup/tests/bootstrapchannelbuilder/cilium/manifest.yaml
index 84b53f6a47..dd841f1004 100644
--- a/upup/pkg/fi/cloudup/tests/bootstrapchannelbuilder/cilium/manifest.yaml
+++ b/upup/pkg/fi/cloudup/tests/bootstrapchannelbuilder/cilium/manifest.yaml
@@ -70,8 +70,9 @@ spec:
     version: 1.15.0
   - id: k8s-1.12
     manifest: networking.cilium.io/k8s-1.12-v1.8.yaml
-    manifestHash: d3ec6a179bbf7de2fdc6d34190cc31819ece539b
+    manifestHash: 67853b52a456f1b701ada61ecab28c8c1f615183
     name: networking.cilium.io
+    needsRollingUpdate: all
     selector:
       role.kubernetes.io/networking: "1"
     version: 1.8.0-kops.1