Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix avaibility reporting for DC with MinReadySeconds set #14936

Merged
merged 2 commits into from
Jul 4, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions pkg/deploy/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -407,8 +407,9 @@ func MakeDeploymentV1(config *deployapi.DeploymentConfig, codec runtime.Codec) (
},
Spec: v1.ReplicationControllerSpec{
// The deployment should be inactive initially
Replicas: &zero,
Selector: selector,
Replicas: &zero,
Selector: selector,
MinReadySeconds: config.Spec.MinReadySeconds,
Template: &v1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: podLabels,
Expand Down
103 changes: 68 additions & 35 deletions test/extended/deployments/deployments.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package deployments

import (
//"errors"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop it, not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also commented parts of code which use it this is because this PR was split in half and the next one uncomments it. Also felt good to leave it because a had to rename kubernetes errors package to kerrors because of it

"fmt"
"math/rand"
"strings"
Expand All @@ -9,11 +10,11 @@ import (
g "github.com/onsi/ginkgo"
o "github.com/onsi/gomega"

"k8s.io/apimachinery/pkg/api/errors"
kerrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/apimachinery/pkg/watch"
kapiv1 "k8s.io/kubernetes/pkg/api/v1"
kcontroller "k8s.io/kubernetes/pkg/controller"
e2e "k8s.io/kubernetes/test/e2e/framework"
Expand Down Expand Up @@ -382,7 +383,7 @@ var _ = g.Describe("deploymentconfigs", func() {
var istag *imageapi.ImageStreamTag
pollErr := wait.PollImmediate(100*time.Millisecond, 1*time.Minute, func() (bool, error) {
istag, err = oc.Client().ImageStreamTags(oc.Namespace()).Get("sample-stream", "deployed")
if errors.IsNotFound(err) {
if kerrors.IsNotFound(err) {
return false, nil
}
if err != nil {
Expand Down Expand Up @@ -882,53 +883,85 @@ var _ = g.Describe("deploymentconfigs", func() {
})

g.Describe("with minimum ready seconds set [Conformance]", func() {
dc := readDCFixtureOrDie(minReadySecondsFixture)
rcName := func(i int) string { return fmt.Sprintf("%s-%d", dc.Name, i) }
g.AfterEach(func() {
failureTrap(oc, "minreadytest", g.CurrentGinkgoTestDescription().Failed)
failureTrap(oc, dc.Name, g.CurrentGinkgoTestDescription().Failed)
})

g.It("should not transition the deployment to Complete before satisfied", func() {
_, name, err := createFixture(oc, minReadySecondsFixture)
namespace := oc.Namespace()
watcher, err := oc.KubeClient().CoreV1().ReplicationControllers(namespace).Watch(metav1.SingleObject(metav1.ObjectMeta{Name: rcName(1), ResourceVersion: ""}))
o.Expect(err).NotTo(o.HaveOccurred())

g.By("verifying the deployment is marked running")
o.Expect(waitForLatestCondition(oc, name, deploymentRunTimeout, deploymentRunning)).NotTo(o.HaveOccurred())

g.By("verifying that all pods are ready")
config, err := oc.Client().DeploymentConfigs(oc.Namespace()).Get(name, metav1.GetOptions{})
o.Expect(dc.Spec.Triggers).To(o.BeNil())
// FIXME: remove when tests are migrated to the new client
// (the old one incorrectly translates nil into an empty array)
dc.Spec.Triggers = append(dc.Spec.Triggers, deployapi.DeploymentTriggerPolicy{Type: deployapi.DeploymentTriggerOnConfigChange})
dc, err = oc.Client().DeploymentConfigs(namespace).Create(dc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing error check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad rebase, fixing it. thx. And I guess I know where that check went :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

o.Expect(err).NotTo(o.HaveOccurred())

selector := labels.Set(config.Spec.Selector).AsSelector()
opts := metav1.ListOptions{LabelSelector: selector.String()}
ready := 0
if err := wait.PollImmediate(500*time.Millisecond, 3*time.Minute, func() (bool, error) {
pods, err := oc.KubeClient().CoreV1().Pods(oc.Namespace()).List(opts)
if err != nil {
return false, nil
g.By("verifying the deployment is created")
rcEvent, err := watch.Until(deploymentChangeTimeout, watcher, func(event watch.Event) (bool, error) {
if event.Type == watch.Added {
return true, nil
}
return false, fmt.Errorf("different kind of event appeared while waiting for Added event: %#v", event)
})
o.Expect(err).NotTo(o.HaveOccurred())
rc1 := rcEvent.Object.(*kapiv1.ReplicationController)

ready = 0
for i := range pods.Items {
pod := pods.Items[i]
if kapiv1.IsPodReady(&pod) {
ready++
}
}
g.By("verifying that all pods are ready")
rc1, err = waitForRCModification(oc, namespace, rc1.Name, deploymentRunTimeout,
rc1.GetResourceVersion(), func(rc *kapiv1.ReplicationController) (bool, error) {
return rc.Status.ReadyReplicas == dc.Spec.Replicas, nil
})
o.Expect(err).NotTo(o.HaveOccurred())
o.Expect(rc1.Status.AvailableReplicas).To(o.BeZero())

return len(pods.Items) == ready, nil
}); err != nil {
o.Expect(fmt.Errorf("deployment config %q never became ready (ready: %d, desired: %d)",
config.Name, ready, config.Spec.Replicas)).NotTo(o.HaveOccurred())
g.By("verifying that the deployment is still running")
if deployutil.IsTerminatedDeployment(rc1) {
o.Expect(fmt.Errorf("expected deployment %q not to have terminated", rc1.Name)).NotTo(o.HaveOccurred())
}

g.By("verifying that the deployment is still running")
latestName := deployutil.DeploymentNameForConfigVersion(name, config.Status.LatestVersion)
latest, err := oc.InternalKubeClient().Core().ReplicationControllers(oc.Namespace()).Get(latestName, metav1.GetOptions{})
o.Expect(err).NotTo(o.HaveOccurred())
g.By("waiting for the deployment to finish")
rc1, err = waitForRCModification(oc, namespace, rc1.Name,
deploymentChangeTimeout+time.Duration(dc.Spec.MinReadySeconds)*time.Second,
rc1.GetResourceVersion(), func(rc *kapiv1.ReplicationController) (bool, error) {
if rc.Status.AvailableReplicas == dc.Spec.Replicas {
return true, nil
}

if deployutil.IsTerminatedDeployment(latest) {
o.Expect(fmt.Errorf("expected deployment %q not to have terminated", latest.Name)).NotTo(o.HaveOccurred())
// FIXME: There is a race between deployer pod updating phase and RC updating AvailableReplicas
// FIXME: Enable this when we switch pod acceptors to use RC AvailableReplicas with MinReadySecondsSet
//if deployutil.DeploymentStatusFor(rc) == deployapi.DeploymentStatusComplete {
// e2e.Logf("Failed RC: %#v", rc)
// return false, errors.New("deployment shouldn't be completed before ReadyReplicas become AvailableReplicas")
//}
return false, nil
})
o.Expect(err).NotTo(o.HaveOccurred())
o.Expect(rc1.Status.AvailableReplicas).To(o.Equal(dc.Spec.Replicas))
// FIXME: There is a race between deployer pod updating phase and RC updating AvailableReplicas
// FIXME: Enable this when we switch pod acceptors to use RC AvailableReplicas with MinReadySecondsSet
//// Deployment status can't be updated yet but should be right after
//o.Expect(deployutil.DeploymentStatusFor(rc1)).To(o.Equal(deployapi.DeploymentStatusRunning))
// It should finish right after
// FIXME: remove this condition when the above is fixed
if deployutil.DeploymentStatusFor(rc1) != deployapi.DeploymentStatusComplete {
// FIXME: remove this assertion when the above is fixed
o.Expect(deployutil.DeploymentStatusFor(rc1)).To(o.Equal(deployapi.DeploymentStatusRunning))
rc1, err = waitForRCModification(oc, namespace, rc1.Name, deploymentChangeTimeout,
rc1.GetResourceVersion(), func(rc *kapiv1.ReplicationController) (bool, error) {
return deployutil.DeploymentStatusFor(rc) == deployapi.DeploymentStatusComplete, nil
})
o.Expect(err).NotTo(o.HaveOccurred())
}
o.Expect(waitForLatestCondition(oc, name, deploymentRunTimeout, deploymentRunning)).NotTo(o.HaveOccurred())

// We might check that minReadySecond passed between pods becoming ready
// and available but I don't think there is a way to get a timestamp from events
// and other ways are just flaky.
// But since we are reusing MinReadySeconds and AvailableReplicas from RC it should be tested there
})
})

Expand Down
8 changes: 8 additions & 0 deletions test/extended/deployments/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -563,3 +563,11 @@ func readDCFixture(path string) (*deployapi.DeploymentConfig, error) {
err = deployapiv1.Convert_v1_DeploymentConfig_To_apps_DeploymentConfig(dcv1, dc, nil)
return dc, err
}

func readDCFixtureOrDie(path string) *deployapi.DeploymentConfig {
data, err := readDCFixture(path)
if err != nil {
panic(err)
}
return data
}
2 changes: 1 addition & 1 deletion test/extended/testdata/bindata.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ metadata:
name: minreadytest
spec:
replicas: 2
minReadySeconds: 500
minReadySeconds: 60
selector:
name: minreadytest
template:
Expand Down