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.

Sign up for GitHub

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

Jump to bottom

Add MinReadySeconds to rolling updater #28111

Merged
merged 1 commit into from
Jul 2, 2016
Merged

Add MinReadySeconds to rolling updater #28111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 20 additions & 10 deletions pkg/kubectl/rolling_updater.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (

"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/errors"
"k8s.io/kubernetes/pkg/api/unversioned"
client "k8s.io/kubernetes/pkg/client/unversioned"
"k8s.io/kubernetes/pkg/labels"
"k8s.io/kubernetes/pkg/runtime"
Expand Down Expand Up @@ -58,6 +59,8 @@ type RollingUpdaterConfig struct {
Interval time.Duration
// Timeout is the time to wait for controller updates before giving up.
Timeout time.Duration
// MinReadySeconds is the number of seconds to wait after the pods are ready
MinReadySeconds int32
// CleanupPolicy defines the cleanup action to take after the deployment is
// complete.
CleanupPolicy RollingUpdaterCleanupPolicy
Expand Down Expand Up @@ -118,7 +121,9 @@ type RollingUpdater struct {
// cleanup performs post deployment cleanup tasks for newRc and oldRc.
cleanup func(oldRc, newRc *api.ReplicationController, config *RollingUpdaterConfig) error
// getReadyPods returns the amount of old and new ready pods.
getReadyPods func(oldRc, newRc *api.ReplicationController) (int32, int32, error)
getReadyPods func(oldRc, newRc *api.ReplicationController, minReadySeconds int32) (int32, int32, error)
// nowFn returns the current time used to calculate the minReadySeconds
nowFn func() unversioned.Time
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't need to be a field in the rolling updater. Make it a global variable (I know... I hate those)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will rather prefer to keep it as the unexported field for updater, so I don't override it globally for every updater in testing. Also it is unexported, so should cause no harm.

}

// NewRollingUpdater creates a RollingUpdater from a client.
Expand All @@ -132,6 +137,7 @@ func NewRollingUpdater(namespace string, client client.Interface) *RollingUpdate
updater.getOrCreateTargetController = updater.getOrCreateTargetControllerWithClient
updater.getReadyPods = updater.readyPods
updater.cleanup = updater.cleanupWithClients
updater.nowFn = func() unversioned.Time { return unversioned.Now() }
return updater
}

Expand Down Expand Up @@ -340,7 +346,7 @@ func (r *RollingUpdater) scaleDown(newRc, oldRc *api.ReplicationController, desi
// Get ready pods. We shouldn't block, otherwise in case both old and new
// pods are unavailable then the rolling update process blocks.
// Timeout-wise we are already covered by the progress check.
_, newAvailable, err := r.getReadyPods(oldRc, newRc)
_, newAvailable, err := r.getReadyPods(oldRc, newRc, config.MinReadySeconds)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -397,10 +403,13 @@ func (r *RollingUpdater) scaleAndWaitWithScaler(rc *api.ReplicationController, r
// readyPods returns the old and new ready counts for their pods.
// If a pod is observed as being ready, it's considered ready even
// if it later becomes notReady.
func (r *RollingUpdater) readyPods(oldRc, newRc *api.ReplicationController) (int32, int32, error) {
func (r *RollingUpdater) readyPods(oldRc, newRc *api.ReplicationController, minReadySeconds int32) (int32, int32, error) {
controllers := []*api.ReplicationController{oldRc, newRc}
oldReady := int32(0)
newReady := int32(0)
if r.nowFn == nil {
r.nowFn = func() unversioned.Time { return unversioned.Now() }
}

for i := range controllers {
controller := controllers[i]
Expand All @@ -411,13 +420,14 @@ func (r *RollingUpdater) readyPods(oldRc, newRc *api.ReplicationController) (int
return 0, 0, err
}
for _, pod := range pods.Items {
if api.IsPodReady(&pod) {
switch controller.Name {
case oldRc.Name:
oldReady++
case newRc.Name:
newReady++
}
if !deployment.IsPodAvailable(&pod, minReadySeconds, r.nowFn().Time) {
continue
}
switch controller.Name {
case oldRc.Name:
oldReady++
case newRc.Name:
newReady++
}
}
}
Expand Down
66 changes: 55 additions & 11 deletions pkg/kubectl/rolling_updater_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/testapi"
apitesting "k8s.io/kubernetes/pkg/api/testing"
"k8s.io/kubernetes/pkg/api/unversioned"
"k8s.io/kubernetes/pkg/client/restclient"
client "k8s.io/kubernetes/pkg/client/unversioned"
"k8s.io/kubernetes/pkg/client/unversioned/fake"
Expand Down Expand Up @@ -810,7 +811,7 @@ Scaling foo-v2 up to 2
},
}
// Set up a mock readiness check which handles the test assertions.
updater.getReadyPods = func(oldRc, newRc *api.ReplicationController) (int32, int32, error) {
updater.getReadyPods = func(oldRc, newRc *api.ReplicationController, minReadySecondsDeadline int32) (int32, int32, error) {
// Return simulated readiness, and throw an error if this call has no
// expectations defined.
oldReady := next(&oldReady)
Expand Down Expand Up @@ -860,7 +861,7 @@ func TestUpdate_progressTimeout(t *testing.T) {
return nil
},
}
updater.getReadyPods = func(oldRc, newRc *api.ReplicationController) (int32, int32, error) {
updater.getReadyPods = func(oldRc, newRc *api.ReplicationController, minReadySeconds int32) (int32, int32, error) {
// Coerce a timeout by pods never becoming ready.
return 0, 0, nil
}
Expand Down Expand Up @@ -913,7 +914,7 @@ func TestUpdate_assignOriginalAnnotation(t *testing.T) {
cleanup: func(oldRc, newRc *api.ReplicationController, config *RollingUpdaterConfig) error {
return nil
},
getReadyPods: func(oldRc, newRc *api.ReplicationController) (int32, int32, error) {
getReadyPods: func(oldRc, newRc *api.ReplicationController, minReadySeconds int32) (int32, int32, error) {
return 1, 1, nil
},
}
Expand Down Expand Up @@ -1555,7 +1556,8 @@ func TestAddDeploymentHash(t *testing.T) {
}

func TestRollingUpdater_readyPods(t *testing.T) {
mkpod := func(owner *api.ReplicationController, ready bool) *api.Pod {
now := unversioned.Date(2016, time.April, 1, 1, 0, 0, 0, time.UTC)
mkpod := func(owner *api.ReplicationController, ready bool, readyTime unversioned.Time) *api.Pod {
labels := map[string]string{}
for k, v := range owner.Spec.Selector {
labels[k] = v
Expand All @@ -1572,8 +1574,9 @@ func TestRollingUpdater_readyPods(t *testing.T) {
Status: api.PodStatus{
Conditions: []api.PodCondition{
{
Type: api.PodReady,
Status: status,
Type: api.PodReady,
Status: status,
LastTransitionTime: readyTime,
},
},
},
Expand All @@ -1589,6 +1592,11 @@ func TestRollingUpdater_readyPods(t *testing.T) {
// pods owned by the rcs; indicate whether they're ready
oldPods []bool
newPods []bool
// specify additional time to wait for deployment to wait on top of the
// pod ready time
minReadySeconds int32
podReadyTimeFn func() unversioned.Time
nowFn func() unversioned.Time
}{
{
oldRc: oldRc(4, 4),
Expand Down Expand Up @@ -1632,25 +1640,61 @@ func TestRollingUpdater_readyPods(t *testing.T) {
false,
},
},
{
oldRc: oldRc(4, 4),
newRc: newRc(4, 4),
oldReady: 0,
newReady: 0,
oldPods: []bool{
true,
},
newPods: []bool{
true,
},
minReadySeconds: 5,
nowFn: func() unversioned.Time { return now },
},
{
oldRc: oldRc(4, 4),
newRc: newRc(4, 4),
oldReady: 1,
newReady: 1,
oldPods: []bool{
true,
},
newPods: []bool{
true,
},
minReadySeconds: 5,
nowFn: func() unversioned.Time { return unversioned.Time{Time: now.Add(time.Duration(6 * time.Second))} },
podReadyTimeFn: func() unversioned.Time { return now },
},
}

for i, test := range tests {
t.Logf("evaluating test %d", i)
if test.nowFn == nil {
test.nowFn = func() unversioned.Time { return now }
}
if test.podReadyTimeFn == nil {
test.podReadyTimeFn = test.nowFn
}
// Populate the fake client with pods associated with their owners.
pods := []runtime.Object{}
for _, ready := range test.oldPods {
pods = append(pods, mkpod(test.oldRc, ready))
pods = append(pods, mkpod(test.oldRc, ready, test.podReadyTimeFn()))
}
for _, ready := range test.newPods {
pods = append(pods, mkpod(test.newRc, ready))
pods = append(pods, mkpod(test.newRc, ready, test.podReadyTimeFn()))
}
client := testclient.NewSimpleFake(pods...)

updater := &RollingUpdater{
ns: "default",
c: client,
ns: "default",
c: client,
nowFn: test.nowFn,
}
oldReady, newReady, err := updater.readyPods(test.oldRc, test.newRc)
oldReady, newReady, err := updater.readyPods(test.oldRc, test.newRc, test.minReadySeconds)
if err != nil {
t.Errorf("unexpected error: %v", err)
}
Expand Down
7 changes: 4 additions & 3 deletions pkg/util/deployment/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,14 +354,15 @@ func GetAvailablePodsForDeployment(c clientset.Interface, deployment *extensions
func countAvailablePods(pods []api.Pod, minReadySeconds int32) int32 {
availablePodCount := int32(0)
for _, pod := range pods {
if IsPodAvailable(&pod, minReadySeconds) {
// TODO: Make the time.Now() as argument to allow unit test this.
if IsPodAvailable(&pod, minReadySeconds, time.Now()) {
availablePodCount++
}
}
return availablePodCount
}

func IsPodAvailable(pod *api.Pod, minReadySeconds int32) bool {
func IsPodAvailable(pod *api.Pod, minReadySeconds int32, now time.Time) bool {
if !controller.IsPodActive(*pod) {
return false
}
Expand All @@ -374,7 +375,7 @@ func IsPodAvailable(pod *api.Pod, minReadySeconds int32) bool {
// 1. minReadySeconds == 0, or
// 2. LastTransitionTime (is set) + minReadySeconds (>0) < current time
minReadySecondsDuration := time.Duration(minReadySeconds) * time.Second
if minReadySeconds == 0 || !c.LastTransitionTime.IsZero() && c.LastTransitionTime.Add(minReadySecondsDuration).Before(time.Now()) {
if minReadySeconds == 0 || !c.LastTransitionTime.IsZero() && c.LastTransitionTime.Add(minReadySecondsDuration).Before(now) {
return true
}
}
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/framework/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3209,7 +3209,7 @@ func WaitForPodsReady(c *clientset.Clientset, ns, name string, minReadySeconds i
return false, nil
}
for _, pod := range pods.Items {
if !deploymentutil.IsPodAvailable(&pod, int32(minReadySeconds)) {
if !deploymentutil.IsPodAvailable(&pod, int32(minReadySeconds), time.Now()) {
return false, nil
}
}
Expand Down Expand Up @@ -3260,7 +3260,7 @@ func logPodsOfDeployment(c clientset.Interface, deployment *extensions.Deploymen
if err == nil {
for _, pod := range podList.Items {
availability := "not available"
if deploymentutil.IsPodAvailable(&pod, minReadySeconds) {
if deploymentutil.IsPodAvailable(&pod, minReadySeconds, time.Now()) {
availability = "available"
}
Logf("Pod %s is %s: %+v", pod.Name, availability, pod)
Expand Down