Skip to content

Commit

Permalink
Merge pull request #39778 from kubernetes/revert-39088-unit-tests-for…
Browse files Browse the repository at this point in the history 
…-the-d-controller

Revert "controller: unit tests for overlapping and recreate deployments"
  • Loading branch information
@mikedanese
mikedanese committed Jan 12, 2017
2 parents 4ac5f27 + 3648eaa commit 93528bb
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 412 deletions.
23 changes: 3 additions & 20 deletions pkg/controller/deployment/deployment_controller.go
View file
Expand Up @@ -73,8 +73,6 @@ type DeploymentController struct {

// To allow injection of syncDeployment for testing.
syncHandler func(dKey string) error
// used for unit testing
enqueueDeployment func(deployment *extensions.Deployment)

// A store of deployments, populated by the dController
dLister *cache.StoreToDeploymentLister
Expand Down Expand Up @@ -136,8 +134,6 @@ func NewDeploymentController(dInformer informers.DeploymentInformer, rsInformer
})

dc.syncHandler = dc.syncDeployment
dc.enqueueDeployment = dc.enqueue

dc.dLister = dInformer.Lister()
dc.rsLister = rsInformer.Lister()
dc.podLister = podInformer.Lister()
Expand Down Expand Up @@ -347,7 +343,7 @@ func (dc *DeploymentController) deletePod(obj interface{}) {
}
}

func (dc *DeploymentController) enqueue(deployment *extensions.Deployment) {
func (dc *DeploymentController) enqueueDeployment(deployment *extensions.Deployment) {
key, err := controller.KeyFunc(deployment)
if err != nil {
utilruntime.HandleError(fmt.Errorf("Couldn't get key for object %#v: %v", deployment, err))
Expand Down Expand Up @@ -612,27 +608,14 @@ func (dc *DeploymentController) handleOverlap(d *extensions.Deployment, deployme
// deployments if this one has been marked deleted, we only update its status as long
// as it is not actually deleted.
if foundOverlaps && d.DeletionTimestamp == nil {
overlapping = true
// Look at the overlapping annotation in both deployments. If one of them has it and points
// to the other one then we don't need to compare their timestamps.
otherOverlapsWith := otherD.Annotations[util.OverlapAnnotation]
currentOverlapsWith := d.Annotations[util.OverlapAnnotation]
// The other deployment is already marked as overlapping with the current one.
if otherOverlapsWith == d.Name {
var err error
if d, err = dc.clearDeploymentOverlap(d, otherD.Name); err != nil {
errs = append(errs, err)
}
continue
}

otherCopy, err := util.DeploymentDeepCopy(otherD)
if err != nil {
return false, err
}
overlapping = true

// Skip syncing this one if older overlapping one is found.
if currentOverlapsWith == otherCopy.Name || util.SelectorUpdatedBefore(otherCopy, d) {
if util.SelectorUpdatedBefore(otherCopy, d) {
if _, err = dc.markDeploymentOverlap(d, otherCopy.Name); err != nil {
return false, err
}
Expand Down
281 changes: 9 additions & 272 deletions pkg/controller/deployment/deployment_controller_test.go
View file
Expand Up @@ -19,7 +19,6 @@ package deployment
import (
"fmt"
"testing"
"time"

"k8s.io/apimachinery/pkg/apimachinery/registered"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -31,7 +30,6 @@ import (
"k8s.io/kubernetes/pkg/client/record"
"k8s.io/kubernetes/pkg/client/testing/core"
"k8s.io/kubernetes/pkg/controller"
"k8s.io/kubernetes/pkg/controller/deployment/util"
"k8s.io/kubernetes/pkg/controller/informers"
"k8s.io/kubernetes/pkg/util/intstr"
"k8s.io/kubernetes/pkg/util/uuid"
Expand Down Expand Up @@ -69,10 +67,9 @@ func newDeployment(name string, replicas int, revisionHistoryLimit *int32, maxSu
d := extensions.Deployment{
TypeMeta: metav1.TypeMeta{APIVersion: registered.GroupOrDie(extensions.GroupName).GroupVersion.String()},
ObjectMeta: v1.ObjectMeta{
UID: uuid.NewUUID(),
Name: name,
Namespace: v1.NamespaceDefault,
Annotations: make(map[string]string),
UID: uuid.NewUUID(),
Name: name,
Namespace: v1.NamespaceDefault,
},
Spec: extensions.DeploymentSpec{
Strategy: extensions.DeploymentStrategy{
Expand Down Expand Up @@ -113,10 +110,8 @@ func newReplicaSet(d *extensions.Deployment, name string, replicas int) *extensi
ObjectMeta: v1.ObjectMeta{
Name: name,
Namespace: v1.NamespaceDefault,
Labels: d.Spec.Selector.MatchLabels,
},
Spec: extensions.ReplicaSetSpec{
Selector: d.Spec.Selector,
Replicas: func() *int32 { i := int32(replicas); return &i }(),
Template: d.Spec.Template,
},
Expand Down Expand Up @@ -147,6 +142,10 @@ type fixture struct {
objects []runtime.Object
}

func (f *fixture) expectUpdateDeploymentAction(d *extensions.Deployment) {
f.actions = append(f.actions, core.NewUpdateAction(schema.GroupVersionResource{Resource: "deployments"}, d.Namespace, d))
}

func (f *fixture) expectUpdateDeploymentStatusAction(d *extensions.Deployment) {
action := core.NewUpdateAction(schema.GroupVersionResource{Resource: "deployments"}, d.Namespace, d)
action.Subresource = "status"
Expand All @@ -164,7 +163,7 @@ func newFixture(t *testing.T) *fixture {
return f
}

func (f *fixture) newController() (*DeploymentController, informers.SharedInformerFactory) {
func (f *fixture) run(deploymentName string) {
f.client = fake.NewSimpleClientset(f.objects...)
informers := informers.NewSharedInformerFactory(f.client, nil, controller.NoResyncPeriodFunc())
c := NewDeploymentController(informers.Deployments(), informers.ReplicaSets(), informers.Pods(), f.client)
Expand All @@ -181,11 +180,6 @@ func (f *fixture) newController() (*DeploymentController, informers.SharedInform
for _, pod := range f.podLister {
c.podLister.Indexer.Add(pod)
}
return c, informers
}

func (f *fixture) run(deploymentName string) {
c, informers := f.newController()
stopCh := make(chan struct{})
defer close(stopCh)
informers.Start(stopCh)
Expand Down Expand Up @@ -224,7 +218,7 @@ func TestSyncDeploymentCreatesReplicaSet(t *testing.T) {
rs := newReplicaSet(d, "deploymentrs-4186632231", 1)

f.expectCreateRSAction(rs)
f.expectUpdateDeploymentStatusAction(d)
f.expectUpdateDeploymentAction(d)
f.expectUpdateDeploymentStatusAction(d)

f.run(getKey(d, t))
Expand Down Expand Up @@ -292,260 +286,3 @@ func filterInformerActions(actions []core.Action) []core.Action {

return ret
}

// TestOverlappingDeployment ensures that an overlapping deployment will not be synced by
// the controller.
func TestOverlappingDeployment(t *testing.T) {
f := newFixture(t)
now := metav1.Now()
later := metav1.Time{Time: now.Add(time.Minute)}

foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"})
foo.CreationTimestamp = now
bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"foo": "bar", "app": "baz"})
bar.CreationTimestamp = later

f.dLister = append(f.dLister, foo, bar)
f.objects = append(f.objects, foo, bar)

f.expectUpdateDeploymentStatusAction(bar)
f.run(getKey(bar, t))

actions := f.client.Actions()
d := actions[0].(core.UpdateAction).GetObject().(*extensions.Deployment)
if len(d.Annotations[util.OverlapAnnotation]) == 0 {
t.Errorf("annotations weren't updated for the overlapping deployment: %v", d.Annotations)
}
}

// TestSyncOverlappedDeployment ensures that from two overlapping deployments, the older
// one will be synced and the newer will be marked as overlapping. Note that in reality it's
// not always the older deployment that is the one that works vs the rest but the one which
// has the selector unchanged for longer time.
func TestSyncOverlappedDeployment(t *testing.T) {
f := newFixture(t)
now := metav1.Now()
later := metav1.Time{Time: now.Add(time.Minute)}

foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"})
foo.CreationTimestamp = now
bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"foo": "bar", "app": "baz"})
bar.CreationTimestamp = later

f.dLister = append(f.dLister, foo, bar)
f.objects = append(f.objects, foo, bar)

f.expectUpdateDeploymentStatusAction(bar)
f.expectCreateRSAction(newReplicaSet(foo, "foo-rs", 1))
f.expectUpdateDeploymentStatusAction(foo)
f.expectUpdateDeploymentStatusAction(foo)
f.run(getKey(foo, t))

actions := f.client.Actions()
d := actions[0].(core.UpdateAction).GetObject().(*extensions.Deployment)
if d.Annotations[util.OverlapAnnotation] != "foo" {
t.Errorf("annotations weren't updated for the overlapping deployment: %v", d.Annotations)
}
}

// TestSelectorUpdate ensures that from two overlapping deployments, the one that is working won't
// be marked as overlapping if its selector is updated but still overlaps with the other one.
func TestSelectorUpdate(t *testing.T) {
f := newFixture(t)
now := metav1.Now()
later := metav1.Time{Time: now.Add(time.Minute)}
selectorUpdated := metav1.Time{Time: later.Add(time.Minute)}

foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"})
foo.CreationTimestamp = now
foo.Annotations = map[string]string{util.SelectorUpdateAnnotation: selectorUpdated.Format(time.RFC3339)}
bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"foo": "bar", "app": "baz"})
bar.CreationTimestamp = later
bar.Annotations = map[string]string{util.OverlapAnnotation: "foo"}

f.dLister = append(f.dLister, foo, bar)
f.objects = append(f.objects, foo, bar)

f.expectCreateRSAction(newReplicaSet(foo, "foo-rs", 1))
f.expectUpdateDeploymentStatusAction(foo)
f.expectUpdateDeploymentStatusAction(foo)
f.run(getKey(foo, t))

for _, a := range filterInformerActions(f.client.Actions()) {
action, ok := a.(core.UpdateAction)
if !ok {
continue
}
d, ok := action.GetObject().(*extensions.Deployment)
if !ok {
continue
}

if d.Name == "foo" && len(d.Annotations[util.OverlapAnnotation]) > 0 {
t.Errorf("deployment %q should not have the overlapping annotation", d.Name)
}
if d.Name == "bar" && len(d.Annotations[util.OverlapAnnotation]) == 0 {
t.Errorf("deployment %q should have the overlapping annotation", d.Name)
}
}
}

// TestDeletedDeploymentShouldCleanupOverlaps ensures that the deletion of a deployment
// will cleanup any deployments that overlap with it.
func TestDeletedDeploymentShouldCleanupOverlaps(t *testing.T) {
f := newFixture(t)
now := metav1.Now()
earlier := metav1.Time{Time: now.Add(-time.Minute)}
later := metav1.Time{Time: now.Add(time.Minute)}

foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"})
foo.CreationTimestamp = earlier
foo.DeletionTimestamp = &now
bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"foo": "bar"})
bar.CreationTimestamp = later
bar.Annotations = map[string]string{util.OverlapAnnotation: "foo"}

f.dLister = append(f.dLister, foo, bar)
f.objects = append(f.objects, foo, bar)

f.expectUpdateDeploymentStatusAction(bar)
f.expectUpdateDeploymentStatusAction(foo)
f.run(getKey(foo, t))

for _, a := range filterInformerActions(f.client.Actions()) {
action, ok := a.(core.UpdateAction)
if !ok {
continue
}
d := action.GetObject().(*extensions.Deployment)
if d.Name != "bar" {
continue
}

if len(d.Annotations[util.OverlapAnnotation]) > 0 {
t.Errorf("annotations weren't cleaned up for the overlapping deployment: %v", d.Annotations)
}
}
}

// TestDeletedDeploymentShouldNotCleanupOtherOverlaps ensures that the deletion of
// a deployment will not cleanup deployments that overlap with another deployment.
func TestDeletedDeploymentShouldNotCleanupOtherOverlaps(t *testing.T) {
f := newFixture(t)
now := metav1.Now()
earlier := metav1.Time{Time: now.Add(-time.Minute)}
later := metav1.Time{Time: now.Add(time.Minute)}

foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"})
foo.CreationTimestamp = earlier
foo.DeletionTimestamp = &now
bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"bla": "bla"})
bar.CreationTimestamp = later
// Notice this deployment is overlapping with another deployment
bar.Annotations = map[string]string{util.OverlapAnnotation: "baz"}

f.dLister = append(f.dLister, foo, bar)
f.objects = append(f.objects, foo, bar)

f.expectUpdateDeploymentStatusAction(foo)
f.run(getKey(foo, t))

for _, a := range filterInformerActions(f.client.Actions()) {
action, ok := a.(core.UpdateAction)
if !ok {
continue
}
d := action.GetObject().(*extensions.Deployment)
if d.Name != "bar" {
continue
}

if len(d.Annotations[util.OverlapAnnotation]) == 0 {
t.Errorf("overlapping annotation should not be cleaned up for bar: %v", d.Annotations)
}
}
}

// TestPodDeletionEnqueuesRecreateDeployment ensures that the deletion of a pod
// will requeue a Recreate deployment iff there is no other pod returned from the
// client.
func TestPodDeletionEnqueuesRecreateDeployment(t *testing.T) {
f := newFixture(t)

foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"})
foo.Spec.Strategy.Type = extensions.RecreateDeploymentStrategyType
rs := newReplicaSet(foo, "foo-1", 1)
pod := generatePodFromRS(rs)

f.dLister = append(f.dLister, foo)
f.rsLister = append(f.rsLister, rs)
f.objects = append(f.objects, foo, rs)

c, informers := f.newController()
enqueued := false
c.enqueueDeployment = func(d *extensions.Deployment) {
if d.Name == "foo" {
enqueued = true
}
}
stopCh := make(chan struct{})
defer close(stopCh)
informers.Start(stopCh)

c.deletePod(pod)

if !enqueued {
t.Errorf("expected deployment %q to be queued after pod deletion", foo.Name)
}
}

// TestPodDeletionDoesntEnqueueRecreateDeployment ensures that the deletion of a pod
// will not requeue a Recreate deployment iff there are other pods returned from the
// client.
func TestPodDeletionDoesntEnqueueRecreateDeployment(t *testing.T) {
f := newFixture(t)

foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"})
foo.Spec.Strategy.Type = extensions.RecreateDeploymentStrategyType
rs := newReplicaSet(foo, "foo-1", 1)
pod := generatePodFromRS(rs)

f.dLister = append(f.dLister, foo)
f.rsLister = append(f.rsLister, rs)
// Let's pretend this is a different pod. The gist is that the pod lister needs to
// return a non-empty list.
f.podLister = append(f.podLister, pod)

c, informers := f.newController()
enqueued := false
c.enqueueDeployment = func(d *extensions.Deployment) {
if d.Name == "foo" {
enqueued = true
}
}
stopCh := make(chan struct{})
defer close(stopCh)
informers.Start(stopCh)

c.deletePod(pod)

if enqueued {
t.Errorf("expected deployment %q not to be queued after pod deletion", foo.Name)
}
}

// generatePodFromRS creates a pod, with the input ReplicaSet's selector and its template
func generatePodFromRS(rs *extensions.ReplicaSet) *v1.Pod {
trueVar := true
return &v1.Pod{
ObjectMeta: v1.ObjectMeta{
Name: rs.Name + "-pod",
Namespace: rs.Namespace,
Labels: rs.Spec.Selector.MatchLabels,
OwnerReferences: []metav1.OwnerReference{
{UID: rs.UID, APIVersion: "v1beta1", Kind: "ReplicaSet", Name: rs.Name, Controller: &trueVar},
},
},
Spec: rs.Spec.Template.Spec,
}
}

0 comments on commit 93528bb

Please sign in to comment.