VPA updater: fix unready DaemonSet updates

Signed-off-by: Luke Addison <lukeaddison785@gmail.com>

Author: dippynark

vertical-pod-autoscaler/pkg/updater/restriction/pods_restriction_factory.go

@@ -148,10 +148,15 @@ func (f *PodsRestrictionFactoryImpl) getReplicaCount(creator podReplicaCreator)
 		if !ok {
 			return 0, errors.New("failed to parse DaemonSet")
 		}
-		if ds.Status.NumberReady == 0 {
-			return 0, fmt.Errorf("daemon set %s/%s has no number ready pods", creator.Namespace, creator.Name)
+		// DaemonSets have no replicas config, so we use the number of nodes the daemon
+		// pod is expected to run on. This must not be derived from readiness (e.g.
+		// Status.NumberReady): a DaemonSet whose pods are all unready (for example
+		// crash looping because the recommendation is too low) would otherwise be
+		// skipped here and could never be updated to recover.
+		if ds.Status.DesiredNumberScheduled == 0 {
+			return 0, fmt.Errorf("daemon set %s/%s has no desired scheduled pods", creator.Namespace, creator.Name)
 		}
-		return int(ds.Status.NumberReady), nil
+		return int(ds.Status.DesiredNumberScheduled), nil
 	}
 	return 0, nil
 }

vertical-pod-autoscaler/pkg/updater/restriction/pods_restriction_factory_test.go

@@ -650,7 +650,8 @@ func TestEvictReplicatedByDaemonSet(t *testing.T) {
 			Kind: "DaemonSet",
 		},
 		Status: appsv1.DaemonSetStatus{
-			NumberReady: livePods,
+			DesiredNumberScheduled: livePods,
+			NumberReady:            livePods,
 		},
 	}
 
@@ -680,6 +681,53 @@ func TestEvictReplicatedByDaemonSet(t *testing.T) {
 	}
 }
 
+func TestEvictReplicatedByDaemonSetWithNoReadyPods(t *testing.T) {
+	// A DaemonSet whose pods are all unready (e.g. crash looping because the applied
+	// recommendation is too low) must still be updatable, otherwise it can never
+	// recover.
+	livePods := int32(5)
+
+	ds := appsv1.DaemonSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "ds",
+			Namespace: "default",
+		},
+		TypeMeta: metav1.TypeMeta{
+			Kind: "DaemonSet",
+		},
+		Status: appsv1.DaemonSetStatus{
+			DesiredNumberScheduled: livePods,
+			NumberReady:            0,
+		},
+	}
+
+	pods := make([]*corev1.Pod, livePods)
+	for i := range pods {
+		pods[i] = test.Pod().WithName(getTestPodName(i)).WithCreator(&ds.ObjectMeta, &ds.TypeMeta).Get()
+	}
+
+	basicVpa := getBasicVpa()
+	factory, err := getRestrictionFactory(nil, nil, nil, &ds, 2, 0.5, nil, nil, nil, false)
+	assert.NoError(t, err)
+	creatorToSingleGroupStatsMap, podToReplicaCreatorMap, err := factory.GetCreatorMaps(pods, basicVpa)
+	assert.NoError(t, err)
+	assert.Len(t, creatorToSingleGroupStatsMap, 1, "DaemonSet should not be skipped when it has no ready pods")
+	eviction := factory.NewPodsEvictionRestriction(creatorToSingleGroupStatsMap, podToReplicaCreatorMap)
+
+	for _, pod := range pods {
+		assert.True(t, eviction.CanEvict(pod))
+	}
+
+	for _, pod := range pods[:2] {
+		err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
+		assert.Nil(t, err, "Should evict with no error")
+	}
+	for _, pod := range pods[2:] {
+		err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
+		assert.Error(t, err, "Error expected")
+	}
+}
+
 func TestEvictReplicatedByJob(t *testing.T) {
 	job := batchv1.Job{
 		ObjectMeta: metav1.ObjectMeta{