Merge pull request #26680 from olegshaldybin/fake-clientset-registry

Automatic merge from submit-queue

Track object modifications in fake clientset

Fake clientset is used by unit tests extensively but it has some
shortcomings:

- no filtering on namespace and name: tests that want to test objects in
  multiple namespaces end up getting all objects from this clientset,
  as it doesn't perform any filtering based on name and namespace;

- updates and deletes don't modify the clientset state, so some tests
  can get unexpected results if they modify/delete objects using the
  clientset;

- it's possible to insert multiple objects with the same
  kind/name/namespace, this leads to confusing behavior, as retrieval is
  based on the insertion order, but anchors on the last added object as
  long as no more objects are added.

This change changes core.ObjectRetriever implementation to track object
adds, updates and deletes.

Some unit tests were depending on the previous (and somewhat incorrect)
behavior. These are fixed in the following few commits.

pkg/controller/deployment/deployment_controller_test.go

@@ -44,6 +44,7 @@ func rs(name string, replicas int, selector map[string]string, timestamp unversi
 		ObjectMeta: api.ObjectMeta{
 			Name:              name,
 			CreationTimestamp: timestamp,
+			Namespace:         api.NamespaceDefault,
 		},
 		Spec: exp.ReplicaSetSpec{
 			Replicas: int32(replicas),
@@ -64,7 +65,8 @@ func newRSWithStatus(name string, specReplicas, statusReplicas int, selector map
 func deployment(name string, replicas int, maxSurge, maxUnavailable intstr.IntOrString, selector map[string]string) exp.Deployment {
 	return exp.Deployment{
 		ObjectMeta: api.ObjectMeta{
-			Name: name,
+			Name:      name,
+			Namespace: api.NamespaceDefault,
 		},
 		Spec: exp.DeploymentSpec{
 			Replicas: int32(replicas),
@@ -142,10 +144,6 @@ func newReplicaSet(d *exp.Deployment, name string, replicas int) *exp.ReplicaSet
 	}
 }
 
-func newListOptions() api.ListOptions {
-	return api.ListOptions{}
-}
-
 // TestScale tests proportional scaling of deployments. Note that fenceposts for
 // rolling out (maxUnavailable, maxSurge) have no meaning for simple scaling other
 // than recording maxSurge as part of the max-replicas annotation that is taken
@@ -966,22 +964,25 @@ type fixture struct {
 	// Actions expected to happen on the client. Objects from here are also
 	// preloaded into NewSimpleFake.
 	actions []core.Action
-	objects *api.List
+	objects []runtime.Object
 }
 
 func (f *fixture) expectUpdateDeploymentAction(d *exp.Deployment) {
 	f.actions = append(f.actions, core.NewUpdateAction(unversioned.GroupVersionResource{Resource: "deployments"}, d.Namespace, d))
-	f.objects.Items = append(f.objects.Items, d)
+}
+
+func (f *fixture) expectUpdateDeploymentStatusAction(d *exp.Deployment) {
+	action := core.NewUpdateAction(unversioned.GroupVersionResource{Resource: "deployments"}, d.Namespace, d)
+	action.Subresource = "status"
+	f.actions = append(f.actions, action)
 }
 
 func (f *fixture) expectCreateRSAction(rs *exp.ReplicaSet) {
 	f.actions = append(f.actions, core.NewCreateAction(unversioned.GroupVersionResource{Resource: "replicasets"}, rs.Namespace, rs))
-	f.objects.Items = append(f.objects.Items, rs)
 }
 
 func (f *fixture) expectUpdateRSAction(rs *exp.ReplicaSet) {
 	f.actions = append(f.actions, core.NewUpdateAction(unversioned.GroupVersionResource{Resource: "replicasets"}, rs.Namespace, rs))
-	f.objects.Items = append(f.objects.Items, rs)
 }
 
 func (f *fixture) expectListPodAction(namespace string, opt api.ListOptions) {
@@ -991,12 +992,12 @@ func (f *fixture) expectListPodAction(namespace string, opt api.ListOptions) {
 func newFixture(t *testing.T) *fixture {
 	f := &fixture{}
 	f.t = t
-	f.objects = &api.List{}
+	f.objects = []runtime.Object{}
 	return f
 }
 
 func (f *fixture) run(deploymentName string) {
-	f.client = fake.NewSimpleClientset(f.objects)
+	f.client = fake.NewSimpleClientset(f.objects...)
 	c := NewDeploymentController(f.client, controller.NoResyncPeriodFunc)
 	c.eventRecorder = &record.FakeRecorder{}
 	c.rsStoreSynced = alwaysReady
@@ -1040,16 +1041,13 @@ func TestSyncDeploymentCreatesReplicaSet(t *testing.T) {
 
 	d := newDeployment(1, nil)
 	f.dStore = append(f.dStore, d)
+	f.objects = append(f.objects, d)
 
-	// expect that one ReplicaSet with zero replicas is created
-	// then is updated to 1 replica
-	rs := newReplicaSet(d, "deploymentrs-4186632231", 0)
-	updatedRS := newReplicaSet(d, "deploymentrs-4186632231", 1)
+	rs := newReplicaSet(d, "deploymentrs-4186632231", 1)
 
 	f.expectCreateRSAction(rs)
 	f.expectUpdateDeploymentAction(d)
-	f.expectUpdateRSAction(updatedRS)
-	f.expectUpdateDeploymentAction(d)
+	f.expectUpdateDeploymentStatusAction(d)
 
 	f.run(getKey(d, t))
 }

pkg/controller/namespace/namespace_controller_test.go

@@ -133,8 +133,9 @@ func testSyncNamespaceThatIsTerminating(t *testing.T, versions *unversioned.APIV
 			testNamespace: testNamespacePendingFinalize,
 			kubeClientActionSet: sets.NewString(
 				strings.Join([]string{"get", "namespaces", ""}, "-"),
-				strings.Join([]string{"list", "pods", ""}, "-"),
 				strings.Join([]string{"create", "namespaces", "finalize"}, "-"),
+				strings.Join([]string{"list", "pods", ""}, "-"),
+				strings.Join([]string{"delete", "namespaces", ""}, "-"),
 			),
 			dynamicClientActionSet: dynamicClientActionSet,
 		},

pkg/controller/node/nodecontroller.go

@@ -1005,9 +1005,9 @@ func (nc *NodeController) markAllPodsNotReady(nodeName string) error {
 			if cond.Type == api.PodReady {
 				pod.Status.Conditions[i].Status = api.ConditionFalse
 				glog.V(2).Infof("Updating ready status of pod %v to false", pod.Name)
-				pod, err := nc.kubeClient.Core().Pods(pod.Namespace).UpdateStatus(&pod)
+				_, err := nc.kubeClient.Core().Pods(pod.Namespace).UpdateStatus(&pod)
 				if err != nil {
-					glog.Warningf("Failed to update status for pod %q: %v", format.Pod(pod), err)
+					glog.Warningf("Failed to update status for pod %q: %v", format.Pod(&pod), err)
 					errMsg = append(errMsg, fmt.Sprintf("%v", err))
 				}
 				break

pkg/controller/node/nodecontroller_test.go

@@ -1426,8 +1426,25 @@ func newNode(name string) *api.Node {
 }
 
 func newPod(name, host string) *api.Pod {
-	return &api.Pod{ObjectMeta: api.ObjectMeta{Name: name}, Spec: api.PodSpec{NodeName: host},
-		Status: api.PodStatus{Conditions: []api.PodCondition{{Type: api.PodReady, Status: api.ConditionTrue}}}}
+	pod := &api.Pod{
+		ObjectMeta: api.ObjectMeta{
+			Namespace: "default",
+			Name:      name,
+		},
+		Spec: api.PodSpec{
+			NodeName: host,
+		},
+		Status: api.PodStatus{
+			Conditions: []api.PodCondition{
+				{
+					Type:   api.PodReady,
+					Status: api.ConditionTrue,
+				},
+			},
+		},
+	}
+
+	return pod
 }
 
 func contains(node *api.Node, nodes []*api.Node) bool {

pkg/controller/resourcequota/resource_quota_controller_test.go

@@ -162,6 +162,10 @@ func TestSyncResourceQuota(t *testing.T) {
 
 func TestSyncResourceQuotaSpecChange(t *testing.T) {
 	resourceQuota := api.ResourceQuota{
+		ObjectMeta: api.ObjectMeta{
+			Namespace: "default",
+			Name:      "rq",
+		},
 		Spec: api.ResourceQuotaSpec{
 			Hard: api.ResourceList{
 				api.ResourceCPU: resource.MustParse("4"),
@@ -250,6 +254,10 @@ func TestSyncResourceQuotaSpecChange(t *testing.T) {
 
 func TestSyncResourceQuotaNoChange(t *testing.T) {
 	resourceQuota := api.ResourceQuota{
+		ObjectMeta: api.ObjectMeta{
+			Namespace: "default",
+			Name:      "rq",
+		},
 		Spec: api.ResourceQuotaSpec{
 			Hard: api.ResourceList{
 				api.ResourceCPU: resource.MustParse("4"),

pkg/controller/serviceaccount/tokens_controller_test.go

@@ -223,7 +223,7 @@ func TestTokenCreation(t *testing.T) {
 		ExpectedActions []core.Action
 	}{
 		"new serviceaccount with no secrets": {
-			ClientObjects: []runtime.Object{serviceAccount(emptySecretReferences()), createdTokenSecret()},
+			ClientObjects: []runtime.Object{serviceAccount(emptySecretReferences())},
 
 			AddedServiceAccount: serviceAccount(emptySecretReferences()),
 			ExpectedActions: []core.Action{
@@ -233,7 +233,7 @@ func TestTokenCreation(t *testing.T) {
 			},
 		},
 		"new serviceaccount with no secrets encountering create error": {
-			ClientObjects: []runtime.Object{serviceAccount(emptySecretReferences()), createdTokenSecret()},
+			ClientObjects: []runtime.Object{serviceAccount(emptySecretReferences())},
 			MaxRetries:    10,
 			IsAsync:       true,
 			Reactors: []reaction{{
@@ -250,7 +250,6 @@ func TestTokenCreation(t *testing.T) {
 					}
 				},
 			}},
-
 			AddedServiceAccount: serviceAccount(emptySecretReferences()),
 			ExpectedActions: []core.Action{
 				// Attempt 1
@@ -295,7 +294,7 @@ func TestTokenCreation(t *testing.T) {
 			},
 		},
 		"new serviceaccount with missing secrets": {
-			ClientObjects: []runtime.Object{serviceAccount(missingSecretReferences()), createdTokenSecret()},
+			ClientObjects: []runtime.Object{serviceAccount(missingSecretReferences())},
 
 			AddedServiceAccount: serviceAccount(missingSecretReferences()),
 			ExpectedActions: []core.Action{
@@ -305,7 +304,7 @@ func TestTokenCreation(t *testing.T) {
 			},
 		},
 		"new serviceaccount with non-token secrets": {
-			ClientObjects: []runtime.Object{serviceAccount(regularSecretReferences()), createdTokenSecret(), opaqueSecret()},
+			ClientObjects: []runtime.Object{serviceAccount(regularSecretReferences()), opaqueSecret()},
 
 			AddedServiceAccount: serviceAccount(regularSecretReferences()),
 			ExpectedActions: []core.Action{
@@ -329,9 +328,8 @@ func TestTokenCreation(t *testing.T) {
 				core.NewGetAction(unversioned.GroupVersionResource{Resource: "serviceaccounts"}, api.NamespaceDefault, "default"),
 			},
 		},
-
 		"updated serviceaccount with no secrets": {
-			ClientObjects: []runtime.Object{serviceAccount(emptySecretReferences()), createdTokenSecret()},
+			ClientObjects: []runtime.Object{serviceAccount(emptySecretReferences())},
 
 			UpdatedServiceAccount: serviceAccount(emptySecretReferences()),
 			ExpectedActions: []core.Action{
@@ -341,7 +339,7 @@ func TestTokenCreation(t *testing.T) {
 			},
 		},
 		"updated serviceaccount with missing secrets": {
-			ClientObjects: []runtime.Object{serviceAccount(missingSecretReferences()), createdTokenSecret()},
+			ClientObjects: []runtime.Object{serviceAccount(missingSecretReferences())},
 
 			UpdatedServiceAccount: serviceAccount(missingSecretReferences()),
 			ExpectedActions: []core.Action{
@@ -351,7 +349,7 @@ func TestTokenCreation(t *testing.T) {
 			},
 		},
 		"updated serviceaccount with non-token secrets": {
-			ClientObjects: []runtime.Object{serviceAccount(regularSecretReferences()), createdTokenSecret(), opaqueSecret()},
+			ClientObjects: []runtime.Object{serviceAccount(regularSecretReferences()), opaqueSecret()},
 
 			UpdatedServiceAccount: serviceAccount(regularSecretReferences()),
 			ExpectedActions: []core.Action{
@@ -367,7 +365,7 @@ func TestTokenCreation(t *testing.T) {
 			ExpectedActions:       []core.Action{},
 		},
 		"updated serviceaccount with no secrets with resource conflict": {
-			ClientObjects: []runtime.Object{updatedServiceAccount(emptySecretReferences()), createdTokenSecret()},
+			ClientObjects: []runtime.Object{updatedServiceAccount(emptySecretReferences())},
 
 			UpdatedServiceAccount: serviceAccount(emptySecretReferences()),
 			ExpectedActions: []core.Action{