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

kubectl: respect deployment strategy parameters for rollout status #41809

Merged
merged 1 commit into from
Mar 5, 2017
Merged

kubectl: respect deployment strategy parameters for rollout status #41809

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
13 changes: 12 additions & 1 deletion pkg/controller/deployment/util/deployment_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,14 +416,25 @@ func SetReplicasAnnotations(rs *extensions.ReplicaSet, desiredReplicas, maxRepli

// MaxUnavailable returns the maximum unavailable pods a rolling deployment can take.
func MaxUnavailable(deployment extensions.Deployment) int32 {
if !IsRollingUpdate(&deployment) {
if !IsRollingUpdate(&deployment) || *(deployment.Spec.Replicas) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

What if deployment's replicas is greater than 0 but smaller than maxUnavailable? Should we return *(deployment.Spec.Replicas) if it's smaller?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, maybe we should handle this in MinAvailable and uses MinAvailable instead of *(deployment.Spec.Replicas)-MaxUnavailable(*deployment) in those status checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MinAvailable is already calling MaxUnavailable so we should better fix it here.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO the logic should exist in func MinAvailable rather than func MaxUnavailable. We have MaxUnavailable field in the spec, and func MaxUnavailable should return its callers the int32 value of that field, regardless of the number of replicas; otherwise the returned value is surprising to the callers. OTOH, func MinAvailable should return a reasonable min number of replicas that should be available, taking the number of replicas into consideration.

return int32(0)
}
// Error caught by validation
_, maxUnavailable, _ := ResolveFenceposts(deployment.Spec.Strategy.RollingUpdate.MaxSurge, deployment.Spec.Strategy.RollingUpdate.MaxUnavailable, *(deployment.Spec.Replicas))
return maxUnavailable
}

// MaxUnavailableInternal returns the maximum unavailable pods a rolling deployment can take.
// TODO: remove the duplicate
func MaxUnavailableInternal(deployment internalextensions.Deployment) int32 {
if !(deployment.Spec.Strategy.Type == internalextensions.RollingUpdateDeploymentStrategyType) || deployment.Spec.Replicas == 0 {
return int32(0)
}
// Error caught by validation
_, maxUnavailable, _ := ResolveFenceposts(&deployment.Spec.Strategy.RollingUpdate.MaxSurge, &deployment.Spec.Strategy.RollingUpdate.MaxUnavailable, deployment.Spec.Replicas)
return maxUnavailable
}

// MinAvailable returns the minimum available pods of a given deployment
func MinAvailable(deployment *extensions.Deployment) int32 {
if !IsRollingUpdate(deployment) {
Expand Down
5 changes: 3 additions & 2 deletions pkg/kubectl/rollout_status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,9 @@ func (s *DeploymentStatusViewer) Status(namespace, name string, revision int64)
if deployment.Status.Replicas > deployment.Status.UpdatedReplicas {
return fmt.Sprintf("Waiting for rollout to finish: %d old replicas are pending termination...\n", deployment.Status.Replicas-deployment.Status.UpdatedReplicas), false, nil
}
if deployment.Status.AvailableReplicas < deployment.Status.UpdatedReplicas {
return fmt.Sprintf("Waiting for rollout to finish: %d of %d updated replicas are available...\n", deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas), false, nil
minRequired := deployment.Spec.Replicas - util.MaxUnavailableInternal(*deployment)
if deployment.Status.AvailableReplicas < minRequired {
return fmt.Sprintf("Waiting for rollout to finish: %d of %d updated replicas are available (minimum required: %d)...\n", deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas, minRequired), false, nil
}
return fmt.Sprintf("deployment %q successfully rolled out\n", name), true, nil
}
Expand Down
56 changes: 45 additions & 11 deletions pkg/kubectl/rollout_status_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,24 @@ import (
"testing"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
intstrutil "k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/kubernetes/pkg/apis/extensions"
"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
)

func intOrStringP(i int) *intstrutil.IntOrString {
intstr := intstrutil.FromInt(i)
return &intstr
}

func TestDeploymentStatusViewerStatus(t *testing.T) {
tests := []struct {
generation int64
specReplicas int32
status extensions.DeploymentStatus
msg string
done bool
generation int64
specReplicas int32
maxUnavailable *intstrutil.IntOrString
status extensions.DeploymentStatus
msg string
done bool
}{
{
generation: 0,
Expand Down Expand Up @@ -61,8 +68,9 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
done: false,
},
{
generation: 1,
specReplicas: 2,
generation: 1,
specReplicas: 2,
maxUnavailable: intOrStringP(0),
status: extensions.DeploymentStatus{
ObservedGeneration: 1,
Replicas: 2,
Expand All @@ -71,7 +79,7 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
UnavailableReplicas: 1,
},

msg: "Waiting for rollout to finish: 1 of 2 updated replicas are available...\n",
msg: "Waiting for rollout to finish: 1 of 2 updated replicas are available (minimum required: 2)...\n",
done: false,
},
{
Expand Down Expand Up @@ -102,9 +110,26 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
msg: "Waiting for deployment spec update to be observed...\n",
done: false,
},
{
generation: 1,
specReplicas: 2,
maxUnavailable: intOrStringP(1),
status: extensions.DeploymentStatus{
ObservedGeneration: 1,
Replicas: 2,
UpdatedReplicas: 2,
AvailableReplicas: 1,
UnavailableReplicas: 0,
},

msg: "deployment \"foo\" successfully rolled out\n",
done: true,
},
}

for _, test := range tests {
for i := range tests {
test := tests[i]
t.Logf("testing scenario %d", i)
d := &extensions.Deployment{
ObjectMeta: metav1.ObjectMeta{
Namespace: "bar",
Expand All @@ -113,18 +138,27 @@ func TestDeploymentStatusViewerStatus(t *testing.T) {
Generation: test.generation,
},
Spec: extensions.DeploymentSpec{
Strategy: extensions.DeploymentStrategy{
Type: extensions.RollingUpdateDeploymentStrategyType,
RollingUpdate: &extensions.RollingUpdateDeployment{
MaxSurge: *intOrStringP(1),
},
},
Replicas: test.specReplicas,
},
Status: test.status,
}
if test.maxUnavailable != nil {
d.Spec.Strategy.RollingUpdate.MaxUnavailable = *test.maxUnavailable
}
client := fake.NewSimpleClientset(d).Extensions()
dsv := &DeploymentStatusViewer{c: client}
msg, done, err := dsv.Status("bar", "foo", 0)
if err != nil {
t.Fatalf("DeploymentStatusViewer.Status(): %v", err)
t.Fatalf("unexpected error: %v", err)
}
if done != test.done || msg != test.msg {
t.Errorf("DeploymentStatusViewer.Status() for deployment with generation %d, %d replicas specified, and status %+v returned %q, %t, want %q, %t",
t.Errorf("deployment with generation %d, %d replicas specified, and status:\n%+v\nreturned:\n%q, %t\nwant:\n%q, %t",
test.generation,
test.specReplicas,
test.status,
Expand Down