Merge pull request kubevirt#2583 from slintes/fix-pod-conditions

Fixed multiple addition of PodScheduled condition on VMI

pkg/controller/BUILD.bazel

@@ -51,6 +51,7 @@ go_library(
 go_test(
     name = "go_default_test",
     srcs = [
+        "conditions_test.go",
         "controller_ref_manager_test.go",
         "controller_suite_test.go",
         "expectations_test.go",

pkg/controller/conditions.go

@@ -1,3 +1,21 @@
+/*
+ * This file is part of the KubeVirt project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Copyright 2019 Red Hat, Inc.
+ *
+ */
 package controller
 
 import (

pkg/controller/conditions_test.go

@@ -0,0 +1,50 @@
+/*
+ * This file is part of the KubeVirt project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Copyright 2019 Red Hat, Inc.
+ *
+ */
+package controller
+
+import (
+	"testing"
+
+	v12 "k8s.io/api/core/v1"
+
+	v1 "kubevirt.io/client-go/api/v1"
+)
+
+func TestAddPodCondition(t *testing.T) {
+
+	vmi := v1.NewMinimalVMI("test")
+
+	pc1 := &v12.PodCondition{
+		Type:   v12.PodScheduled,
+		Status: v12.ConditionFalse,
+	}
+	pc2 := &v12.PodCondition{
+		Type:   v12.PodScheduled,
+		Status: v12.ConditionTrue,
+	}
+
+	cm := NewVirtualMachineInstanceConditionManager()
+
+	cm.AddPodCondition(vmi, pc1)
+	cm.AddPodCondition(vmi, pc2)
+
+	if len(vmi.Status.Conditions) != 1 {
+		t.Errorf("There should be exactly 1 condition when muliple conditions of the same type were added")
+	}
+}

pkg/virt-controller/watch/vmi.go

@@ -300,6 +300,10 @@ func (c *VMIController) updateStatus(vmi *virtv1.VirtualMachineInstance, pod *k8
 					Message: syncErr.Error(),
 					Status:  k8sv1.ConditionFalse,
 				}
+				cm := controller.NewVirtualMachineInstanceConditionManager()
+				if cm.HasCondition(vmiCopy, condition.Type) {
+					cm.RemoveCondition(vmiCopy, condition.Type)
+				}
 				vmiCopy.Status.Conditions = append(vmiCopy.Status.Conditions, condition)
 			}
 		}

pkg/virt-controller/watch/vmi_test.go

@@ -202,6 +202,7 @@ var _ = Describe("VirtualMachineInstance watcher", func() {
 		vmiSource.Add(vmi)
 		mockQueue.Wait()
 	}
+
 	Context("On valid VirtualMachineInstance given with DataVolume source", func() {
 
 		It("should create a corresponding Pod on VMI creation when DataVolume is ready", func() {
@@ -457,6 +458,62 @@ var _ = Describe("VirtualMachineInstance watcher", func() {
 			Expect(mockQueue.GetRateLimitedEnqueueCount()).To(Equal(0))
 			testutils.ExpectEvent(recorder, SuccessfulCreatePodReason)
 		})
+		It("should create PodScheduled and Synchronized conditions exactly once each for repeated FailedPvcNotFoundReason sync error", func() {
+
+			expectConditions := func(vmi *v1.VirtualMachineInstance) {
+				// PodScheduled and Synchronized
+				Expect(len(vmi.Status.Conditions)).To(Equal(2), "there should be exactly 2 conditions")
+
+				getType := func(c v1.VirtualMachineInstanceCondition) string { return string(c.Type) }
+				getReason := func(c v1.VirtualMachineInstanceCondition) string { return c.Reason }
+				Expect(vmi.Status.Conditions).To(
+					And(
+						ContainElement(
+							WithTransform(getType, Equal(string(v1.VirtualMachineInstanceSynchronized))),
+						),
+						ContainElement(
+							And(
+								WithTransform(getType, Equal(string(k8sv1.PodScheduled))),
+								WithTransform(getReason, Equal(k8sv1.PodReasonUnschedulable)),
+							),
+						),
+					),
+				)
+			}
+
+			vmi := NewPendingVirtualMachine("testvmi")
+			vmi.Spec.Volumes = []v1.Volume{
+				{
+					Name: "test",
+					VolumeSource: v1.VolumeSource{
+						PersistentVolumeClaim: &k8sv1.PersistentVolumeClaimVolumeSource{
+							ClaimName: "something",
+						},
+					},
+				},
+			}
+			addVirtualMachine(vmi)
+			update := vmiInterface.EXPECT().Update(gomock.Any())
+			update.Do(func(vmi *v1.VirtualMachineInstance) {
+				expectConditions(vmi)
+				vmiInformer.GetStore().Update(vmi)
+				update.Return(vmi, nil)
+			})
+
+			controller.Execute()
+			Expect(controller.Queue.Len()).To(Equal(0))
+			Expect(mockQueue.GetRateLimitedEnqueueCount()).To(Equal(1))
+
+			// make sure that during next iteration we do not add the same condition again
+			vmiInterface.EXPECT().Update(gomock.Any()).Do(func(vmi *v1.VirtualMachineInstance) {
+				expectConditions(vmi)
+			}).Return(vmi, nil)
+
+			controller.Execute()
+			Expect(controller.Queue.Len()).To(Equal(0))
+			Expect(mockQueue.GetRateLimitedEnqueueCount()).To(Equal(2))
+		})
+
 		table.DescribeTable("should move the vmi to scheduling state if a pod exists", func(phase k8sv1.PodPhase, isReady bool) {
 			vmi := NewPendingVirtualMachine("testvmi")
 			pod := NewPodForVirtualMachine(vmi, phase)

tests/storage_test.go

@@ -650,10 +650,24 @@ var _ = Describe("Storage", func() {
 					if len(vmi.Status.Conditions) == 0 {
 						return false
 					}
-					Expect(vmi.Status.Conditions[0].Type).To(Equal(v1.VirtualMachineInstanceConditionType(k8sv1.PodScheduled)))
-					Expect(vmi.Status.Conditions[0].Reason).To(Equal(k8sv1.PodReasonUnschedulable))
-					Expect(vmi.Status.Conditions[0].Status).To(Equal(k8sv1.ConditionFalse))
-					Expect(vmi.Status.Conditions[0].Message).To(Equal(fmt.Sprintf("failed to render launch manifest: didn't find PVC %v", pvcName)))
+
+					expectPodScheduledCondition := func(vmi *v1.VirtualMachineInstance) {
+						getType := func(c v1.VirtualMachineInstanceCondition) string { return string(c.Type) }
+						getReason := func(c v1.VirtualMachineInstanceCondition) string { return c.Reason }
+						getStatus := func(c v1.VirtualMachineInstanceCondition) k8sv1.ConditionStatus { return c.Status }
+						getMessage := func(c v1.VirtualMachineInstanceCondition) string { return c.Message }
+						Expect(vmi.Status.Conditions).To(
+							ContainElement(
+								And(
+									WithTransform(getType, Equal(string(k8sv1.PodScheduled))),
+									WithTransform(getReason, Equal(k8sv1.PodReasonUnschedulable)),
+									WithTransform(getStatus, Equal(k8sv1.ConditionFalse)),
+									WithTransform(getMessage, Equal(fmt.Sprintf("failed to render launch manifest: didn't find PVC %v", pvcName))),
+								),
+							),
+						)
+					}
+					expectPodScheduledCondition(vmi)
 					return true
 				}, time.Duration(10)*time.Second).Should(BeTrue(), "Timed out waiting for VMI to get Unschedulable condition")