From 241f4fbc98dab6f817f6c6fe37ca451ed9bbadb6 Mon Sep 17 00:00:00 2001 From: Janet Kuo Date: Fri, 22 Sep 2017 00:57:27 -0700 Subject: [PATCH 1/3] Move deployment collision avoidance e2e test to integration --- test/e2e/apps/deployment.go | 47 -------------- test/e2e/framework/rs_util.go | 23 +------ test/integration/deployment/BUILD | 1 + .../integration/deployment/deployment_test.go | 63 +++++++++++++++++++ test/integration/deployment/util.go | 4 ++ test/utils/BUILD | 1 + test/utils/replicaset.go | 52 +++++++++++++++ 7 files changed, 123 insertions(+), 68 deletions(-) create mode 100644 test/utils/replicaset.go diff --git a/test/e2e/apps/deployment.go b/test/e2e/apps/deployment.go index 62c173a3d4383..ce7130a4750a9 100644 --- a/test/e2e/apps/deployment.go +++ b/test/e2e/apps/deployment.go @@ -108,9 +108,6 @@ var _ = SIGDescribe("Deployment", func() { It("test Deployment ReplicaSet orphaning and adoption regarding controllerRef", func() { testDeploymentsControllerRef(f) }) - It("deployment can avoid hash collisions", func() { - testDeploymentHashCollisionAvoidance(f) - }) // TODO: add tests that cover deployment.Spec.MinReadySeconds once we solved clock-skew issues // See https://github.com/kubernetes/kubernetes/issues/29229 }) @@ -1270,47 +1267,3 @@ func orphanDeploymentReplicaSets(c clientset.Interface, d *extensions.Deployment deleteOptions.Preconditions = metav1.NewUIDPreconditions(string(d.UID)) return c.Extensions().Deployments(d.Namespace).Delete(d.Name, deleteOptions) } - -func testDeploymentHashCollisionAvoidance(f *framework.Framework) { - ns := f.Namespace.Name - c := f.ClientSet - - deploymentName := "test-hash-collision" - framework.Logf("Creating Deployment %q", deploymentName) - podLabels := map[string]string{"name": NginxImageName} - d := framework.NewDeployment(deploymentName, int32(0), podLabels, NginxImageName, NginxImage, extensions.RollingUpdateDeploymentStrategyType) - deployment, err := c.Extensions().Deployments(ns).Create(d) - Expect(err).NotTo(HaveOccurred()) - err = framework.WaitForDeploymentRevisionAndImage(c, ns, deploymentName, "1", NginxImage) - Expect(err).NotTo(HaveOccurred()) - - // TODO: Switch this to do a non-cascading deletion of the Deployment, mutate the ReplicaSet - // once it has no owner reference, then recreate the Deployment if we ever proceed with - // https://github.com/kubernetes/kubernetes/issues/44237 - framework.Logf("Mock a hash collision") - newRS, err := deploymentutil.GetNewReplicaSet(deployment, c.ExtensionsV1beta1()) - Expect(err).NotTo(HaveOccurred()) - var nilRs *extensions.ReplicaSet - Expect(newRS).NotTo(Equal(nilRs)) - _, err = framework.UpdateReplicaSetWithRetries(c, ns, newRS.Name, func(update *extensions.ReplicaSet) { - *update.Spec.Template.Spec.TerminationGracePeriodSeconds = int64(5) - }) - Expect(err).NotTo(HaveOccurred()) - - framework.Logf("Expect deployment collision counter to increment") - if err := wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { - d, err := c.Extensions().Deployments(ns).Get(deploymentName, metav1.GetOptions{}) - if err != nil { - framework.Logf("cannot get deployment %q: %v", deploymentName, err) - return false, nil - } - framework.Logf(spew.Sprintf("deployment status: %#v", d.Status)) - return d.Status.CollisionCount != nil && *d.Status.CollisionCount == int32(1), nil - }); err != nil { - framework.Failf("Failed to increment collision counter for deployment %q: %v", deploymentName, err) - } - - framework.Logf("Expect a new ReplicaSet to be created") - err = framework.WaitForDeploymentRevisionAndImage(c, ns, deploymentName, "2", NginxImage) - Expect(err).NotTo(HaveOccurred()) -} diff --git a/test/e2e/framework/rs_util.go b/test/e2e/framework/rs_util.go index 863a03bef5ff4..bd5eecedcd2dc 100644 --- a/test/e2e/framework/rs_util.go +++ b/test/e2e/framework/rs_util.go @@ -34,27 +34,8 @@ import ( type updateRsFunc func(d *extensions.ReplicaSet) -func UpdateReplicaSetWithRetries(c clientset.Interface, namespace, name string, applyUpdate updateRsFunc) (*extensions.ReplicaSet, error) { - var rs *extensions.ReplicaSet - var updateErr error - pollErr := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { - var err error - if rs, err = c.Extensions().ReplicaSets(namespace).Get(name, metav1.GetOptions{}); err != nil { - return false, err - } - // Apply the update, then attempt to push it to the apiserver. - applyUpdate(rs) - if rs, err = c.Extensions().ReplicaSets(namespace).Update(rs); err == nil { - Logf("Updating replica set %q", name) - return true, nil - } - updateErr = err - return false, nil - }) - if pollErr == wait.ErrWaitTimeout { - pollErr = fmt.Errorf("couldn't apply the provided updated to replicaset %q: %v", name, updateErr) - } - return rs, pollErr +func UpdateReplicaSetWithRetries(c clientset.Interface, namespace, name string, applyUpdate testutils.UpdateReplicaSetFunc) (*extensions.ReplicaSet, error) { + return testutils.UpdateReplicaSetWithRetries(c, namespace, name, applyUpdate, Logf) } // CheckNewRSAnnotations check if the new RS's annotation is as expected diff --git a/test/integration/deployment/BUILD b/test/integration/deployment/BUILD index 1262c0750117b..3d8145ff42f9d 100644 --- a/test/integration/deployment/BUILD +++ b/test/integration/deployment/BUILD @@ -20,6 +20,7 @@ go_test( "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/api/extensions/v1beta1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library", ], ) diff --git a/test/integration/deployment/deployment_test.go b/test/integration/deployment/deployment_test.go index 1aa271df87a35..30330d670876d 100644 --- a/test/integration/deployment/deployment_test.go +++ b/test/integration/deployment/deployment_test.go @@ -20,10 +20,12 @@ import ( "reflect" "strings" "testing" + "time" "k8s.io/api/core/v1" "k8s.io/api/extensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" deploymentutil "k8s.io/kubernetes/pkg/controller/deployment/util" "k8s.io/kubernetes/test/integration/framework" ) @@ -319,3 +321,64 @@ func TestScalePausedDeployment(t *testing.T) { t.Fatal(err) } } + +// Deployment rollout shouldn't be blocked on hash collisions +func TestDeploymentHashCollision(t *testing.T) { + s, closeFn, rm, dc, informers, c := dcSetup(t) + defer closeFn() + name := "test-hash-collision-deployment" + ns := framework.CreateTestingNamespace(name, s, t) + defer framework.DeleteTestingNamespace(ns, s, t) + + replicas := int32(1) + tester := &deploymentTester{t: t, c: c, deployment: newDeployment(name, ns.Name, replicas)} + + var err error + tester.deployment, err = c.ExtensionsV1beta1().Deployments(ns.Name).Create(tester.deployment) + if err != nil { + t.Fatalf("failed to create deployment %s: %v", tester.deployment.Name, err) + } + + // Start informer and controllers + stopCh := make(chan struct{}) + defer close(stopCh) + informers.Start(stopCh) + go rm.Run(5, stopCh) + go dc.Run(5, stopCh) + + // Wait for the Deployment to be updated to revision 1 + if err := tester.waitForDeploymentRevisionAndImage("1", fakeImage); err != nil { + t.Fatal(err) + } + + // Mock a hash collision + newRS, err := deploymentutil.GetNewReplicaSet(tester.deployment, c.ExtensionsV1beta1()) + if err != nil { + t.Fatalf("failed getting new replicaset of deployment %s: %v", tester.deployment.Name, err) + } + if newRS == nil { + t.Fatalf("unable to find new replicaset of deployment %s", tester.deployment.Name) + } + _, err = tester.updateReplicaSet(newRS.Name, func(update *v1beta1.ReplicaSet) { + *update.Spec.Template.Spec.TerminationGracePeriodSeconds = int64(5) + }) + if err != nil { + t.Fatalf("failed updating replicaset %s template: %v", newRS.Name, err) + } + + // Expect deployment collision counter to increment + if err := wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { + d, err := c.ExtensionsV1beta1().Deployments(ns.Name).Get(tester.deployment.Name, metav1.GetOptions{}) + if err != nil { + return false, nil + } + return d.Status.CollisionCount != nil && *d.Status.CollisionCount == int32(1), nil + }); err != nil { + t.Fatalf("Failed to increment collision counter for deployment %q: %v", tester.deployment.Name, err) + } + + // Expect a new ReplicaSet to be created + if err := tester.waitForDeploymentRevisionAndImage("2", fakeImage); err != nil { + t.Fatal(err) + } +} diff --git a/test/integration/deployment/util.go b/test/integration/deployment/util.go index 6201c1d303897..e907cd496ca71 100644 --- a/test/integration/deployment/util.go +++ b/test/integration/deployment/util.go @@ -249,6 +249,10 @@ func (d *deploymentTester) expectNewReplicaSet() (*v1beta1.ReplicaSet, error) { return rs, nil } +func (d *deploymentTester) updateReplicaSet(name string, applyUpdate testutil.UpdateReplicaSetFunc) (*v1beta1.ReplicaSet, error) { + return testutil.UpdateReplicaSetWithRetries(d.c, d.deployment.Namespace, name, applyUpdate, d.t.Logf) +} + func pauseFn() func(update *v1beta1.Deployment) { return func(update *v1beta1.Deployment) { update.Spec.Paused = true diff --git a/test/utils/BUILD b/test/utils/BUILD index 5202071b051ad..d4fbbd408528d 100644 --- a/test/utils/BUILD +++ b/test/utils/BUILD @@ -12,6 +12,7 @@ go_library( "density_utils.go", "deployment.go", "pod_store.go", + "replicaset.go", "runners.go", "tmpdir.go", ], diff --git a/test/utils/replicaset.go b/test/utils/replicaset.go new file mode 100644 index 0000000000000..0721a3d3c9371 --- /dev/null +++ b/test/utils/replicaset.go @@ -0,0 +1,52 @@ +/* +Copyright 2017 The Kubernetes Authors. + +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. +*/ + +package utils + +import ( + "fmt" + "time" + + extensions "k8s.io/api/extensions/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" + clientset "k8s.io/client-go/kubernetes" +) + +type UpdateReplicaSetFunc func(d *extensions.ReplicaSet) + +func UpdateReplicaSetWithRetries(c clientset.Interface, namespace, name string, applyUpdate UpdateReplicaSetFunc, logf LogfFn) (*extensions.ReplicaSet, error) { + var rs *extensions.ReplicaSet + var updateErr error + pollErr := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { + var err error + if rs, err = c.Extensions().ReplicaSets(namespace).Get(name, metav1.GetOptions{}); err != nil { + return false, err + } + // Apply the update, then attempt to push it to the apiserver. + applyUpdate(rs) + if rs, err = c.Extensions().ReplicaSets(namespace).Update(rs); err == nil { + logf("Updating replica set %q", name) + return true, nil + } + updateErr = err + return false, nil + }) + if pollErr == wait.ErrWaitTimeout { + pollErr = fmt.Errorf("couldn't apply the provided updated to replicaset %q: %v", name, updateErr) + } + return rs, pollErr +} From 3a0dabcaeab738207b0a9881d77732cf1e74e1fa Mon Sep 17 00:00:00 2001 From: Janet Kuo Date: Fri, 22 Sep 2017 01:06:59 -0700 Subject: [PATCH 2/3] Refactor function --- .../integration/deployment/deployment_test.go | 6 +++--- test/integration/deployment/util.go | 20 ++++++++----------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/test/integration/deployment/deployment_test.go b/test/integration/deployment/deployment_test.go index 30330d670876d..025ab734deea1 100644 --- a/test/integration/deployment/deployment_test.go +++ b/test/integration/deployment/deployment_test.go @@ -173,7 +173,7 @@ func TestPausedDeployment(t *testing.T) { } // Resume the deployment - tester.deployment, err = tester.updateDeployment(resumeFn()) + tester.deployment, err = tester.updateDeployment(resumeFn) if err != nil { t.Fatalf("failed to resume deployment %s: %v", tester.deployment.Name, err) } @@ -200,7 +200,7 @@ func TestPausedDeployment(t *testing.T) { // Pause the deployment. // The paused deployment shouldn't trigger a new rollout. - tester.deployment, err = tester.updateDeployment(pauseFn()) + tester.deployment, err = tester.updateDeployment(pauseFn) if err != nil { t.Fatalf("failed to pause deployment %s: %v", tester.deployment.Name, err) } @@ -283,7 +283,7 @@ func TestScalePausedDeployment(t *testing.T) { } // Pause the deployment. - tester.deployment, err = tester.updateDeployment(pauseFn()) + tester.deployment, err = tester.updateDeployment(pauseFn) if err != nil { t.Fatalf("failed to pause deployment %s: %v", tester.deployment.Name, err) } diff --git a/test/integration/deployment/util.go b/test/integration/deployment/util.go index e907cd496ca71..9cd60f2cc2ef5 100644 --- a/test/integration/deployment/util.go +++ b/test/integration/deployment/util.go @@ -45,6 +45,14 @@ const ( fakeImage = "fakeimage" ) +var pauseFn = func(update *v1beta1.Deployment) { + update.Spec.Paused = true +} + +var resumeFn = func(update *v1beta1.Deployment) { + update.Spec.Paused = false +} + type deploymentTester struct { t *testing.T c clientset.Interface @@ -252,15 +260,3 @@ func (d *deploymentTester) expectNewReplicaSet() (*v1beta1.ReplicaSet, error) { func (d *deploymentTester) updateReplicaSet(name string, applyUpdate testutil.UpdateReplicaSetFunc) (*v1beta1.ReplicaSet, error) { return testutil.UpdateReplicaSetWithRetries(d.c, d.deployment.Namespace, name, applyUpdate, d.t.Logf) } - -func pauseFn() func(update *v1beta1.Deployment) { - return func(update *v1beta1.Deployment) { - update.Spec.Paused = true - } -} - -func resumeFn() func(update *v1beta1.Deployment) { - return func(update *v1beta1.Deployment) { - update.Spec.Paused = false - } -} From 24eb21e6cf36da896ae6a410fad8947c129b39b0 Mon Sep 17 00:00:00 2001 From: Janet Kuo Date: Mon, 25 Sep 2017 14:17:43 -0700 Subject: [PATCH 3/3] Use PollImmediate and shorter interval in integration test --- test/e2e/framework/deployment_util.go | 2 +- test/e2e/framework/rs_util.go | 2 +- test/integration/deployment/deployment_test.go | 3 +-- test/integration/deployment/util.go | 8 ++++---- test/utils/deployment.go | 4 ++-- test/utils/replicaset.go | 4 ++-- 6 files changed, 11 insertions(+), 12 deletions(-) diff --git a/test/e2e/framework/deployment_util.go b/test/e2e/framework/deployment_util.go index 5146f8d3178ac..80c19dcbfd3e5 100644 --- a/test/e2e/framework/deployment_util.go +++ b/test/e2e/framework/deployment_util.go @@ -36,7 +36,7 @@ import ( ) func UpdateDeploymentWithRetries(c clientset.Interface, namespace, name string, applyUpdate testutils.UpdateDeploymentFunc) (*extensions.Deployment, error) { - return testutils.UpdateDeploymentWithRetries(c, namespace, name, applyUpdate, Logf) + return testutils.UpdateDeploymentWithRetries(c, namespace, name, applyUpdate, Logf, Poll, pollShortTimeout) } // Waits for the deployment to clean up old rcs. diff --git a/test/e2e/framework/rs_util.go b/test/e2e/framework/rs_util.go index bd5eecedcd2dc..a7c6ce5717242 100644 --- a/test/e2e/framework/rs_util.go +++ b/test/e2e/framework/rs_util.go @@ -35,7 +35,7 @@ import ( type updateRsFunc func(d *extensions.ReplicaSet) func UpdateReplicaSetWithRetries(c clientset.Interface, namespace, name string, applyUpdate testutils.UpdateReplicaSetFunc) (*extensions.ReplicaSet, error) { - return testutils.UpdateReplicaSetWithRetries(c, namespace, name, applyUpdate, Logf) + return testutils.UpdateReplicaSetWithRetries(c, namespace, name, applyUpdate, Logf, Poll, pollShortTimeout) } // CheckNewRSAnnotations check if the new RS's annotation is as expected diff --git a/test/integration/deployment/deployment_test.go b/test/integration/deployment/deployment_test.go index 025ab734deea1..0695754f38e03 100644 --- a/test/integration/deployment/deployment_test.go +++ b/test/integration/deployment/deployment_test.go @@ -20,7 +20,6 @@ import ( "reflect" "strings" "testing" - "time" "k8s.io/api/core/v1" "k8s.io/api/extensions/v1beta1" @@ -367,7 +366,7 @@ func TestDeploymentHashCollision(t *testing.T) { } // Expect deployment collision counter to increment - if err := wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { + if err := wait.PollImmediate(pollInterval, pollTimeout, func() (bool, error) { d, err := c.ExtensionsV1beta1().Deployments(ns.Name).Get(tester.deployment.Name, metav1.GetOptions{}) if err != nil { return false, nil diff --git a/test/integration/deployment/util.go b/test/integration/deployment/util.go index 9cd60f2cc2ef5..77547a7454c3b 100644 --- a/test/integration/deployment/util.go +++ b/test/integration/deployment/util.go @@ -38,7 +38,7 @@ import ( ) const ( - pollInterval = 1 * time.Second + pollInterval = 100 * time.Millisecond pollTimeout = 60 * time.Second fakeImageName = "fake-name" @@ -168,7 +168,7 @@ func (d *deploymentTester) markAllPodsReady() { d.t.Fatalf("failed to parse Deployment selector: %v", err) } var readyPods int32 - err = wait.Poll(100*time.Millisecond, pollTimeout, func() (bool, error) { + err = wait.PollImmediate(pollInterval, pollTimeout, func() (bool, error) { readyPods = 0 pods, err := d.c.Core().Pods(ns).List(metav1.ListOptions{LabelSelector: selector.String()}) if err != nil { @@ -217,7 +217,7 @@ func (d *deploymentTester) waitForDeploymentStatusValidAndMarkPodsReady() error } func (d *deploymentTester) updateDeployment(applyUpdate testutil.UpdateDeploymentFunc) (*v1beta1.Deployment, error) { - return testutil.UpdateDeploymentWithRetries(d.c, d.deployment.Namespace, d.deployment.Name, applyUpdate, d.t.Logf) + return testutil.UpdateDeploymentWithRetries(d.c, d.deployment.Namespace, d.deployment.Name, applyUpdate, d.t.Logf, pollInterval, pollTimeout) } func (d *deploymentTester) waitForObservedDeployment(desiredGeneration int64) error { @@ -258,5 +258,5 @@ func (d *deploymentTester) expectNewReplicaSet() (*v1beta1.ReplicaSet, error) { } func (d *deploymentTester) updateReplicaSet(name string, applyUpdate testutil.UpdateReplicaSetFunc) (*v1beta1.ReplicaSet, error) { - return testutil.UpdateReplicaSetWithRetries(d.c, d.deployment.Namespace, name, applyUpdate, d.t.Logf) + return testutil.UpdateReplicaSetWithRetries(d.c, d.deployment.Namespace, name, applyUpdate, d.t.Logf, pollInterval, pollTimeout) } diff --git a/test/utils/deployment.go b/test/utils/deployment.go index 9d0ff50d49b6e..a531e76950892 100644 --- a/test/utils/deployment.go +++ b/test/utils/deployment.go @@ -216,10 +216,10 @@ func containsImage(containers []v1.Container, imageName string) bool { type UpdateDeploymentFunc func(d *extensions.Deployment) -func UpdateDeploymentWithRetries(c clientset.Interface, namespace, name string, applyUpdate UpdateDeploymentFunc, logf LogfFn) (*extensions.Deployment, error) { +func UpdateDeploymentWithRetries(c clientset.Interface, namespace, name string, applyUpdate UpdateDeploymentFunc, logf LogfFn, pollInterval, pollTimeout time.Duration) (*extensions.Deployment, error) { var deployment *extensions.Deployment var updateErr error - pollErr := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { + pollErr := wait.PollImmediate(pollInterval, pollTimeout, func() (bool, error) { var err error if deployment, err = c.Extensions().Deployments(namespace).Get(name, metav1.GetOptions{}); err != nil { return false, err diff --git a/test/utils/replicaset.go b/test/utils/replicaset.go index 0721a3d3c9371..9a4b6d05b9007 100644 --- a/test/utils/replicaset.go +++ b/test/utils/replicaset.go @@ -28,10 +28,10 @@ import ( type UpdateReplicaSetFunc func(d *extensions.ReplicaSet) -func UpdateReplicaSetWithRetries(c clientset.Interface, namespace, name string, applyUpdate UpdateReplicaSetFunc, logf LogfFn) (*extensions.ReplicaSet, error) { +func UpdateReplicaSetWithRetries(c clientset.Interface, namespace, name string, applyUpdate UpdateReplicaSetFunc, logf LogfFn, pollInterval, pollTimeout time.Duration) (*extensions.ReplicaSet, error) { var rs *extensions.ReplicaSet var updateErr error - pollErr := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { + pollErr := wait.PollImmediate(pollInterval, pollTimeout, func() (bool, error) { var err error if rs, err = c.Extensions().ReplicaSets(namespace).Get(name, metav1.GetOptions{}); err != nil { return false, err