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

kubectl: ignore only update conflicts in the scaler #27048

Merged
merged 2 commits into from
Jul 3, 2016
Merged

kubectl: ignore only update conflicts in the scaler #27048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a74f700
kubectl: ignore only update conflicts in the scaler
0xmichalis Jun 8, 2016
c7140b3
kubectl: return more meaningful timeout errors
0xmichalis Jun 8, 2016
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
55 changes: 31 additions & 24 deletions pkg/kubectl/scale.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ type ScaleErrorType int
const (
ScaleGetFailure ScaleErrorType = iota
ScaleUpdateFailure
ScaleUpdateInvalidFailure
ScaleUpdateConflictFailure
)

// A ScaleError is returned when a scale request passes
Expand Down Expand Up @@ -115,11 +115,8 @@ func ScaleCondition(r Scaler, precondition *ScalePrecondition, namespace, name s
case nil:
return true, nil
case ScaleError:
// if it's invalid we shouldn't keep waiting
if e.FailureType == ScaleUpdateInvalidFailure {
return false, err
}
if e.FailureType == ScaleUpdateFailure {
// Retry only on update conflicts.
if e.FailureType == ScaleUpdateConflictFailure {
return false, nil
}
}
Expand Down Expand Up @@ -153,10 +150,9 @@ func (scaler *ReplicationControllerScaler) ScaleSimple(namespace, name string, p
}
}
controller.Spec.Replicas = int32(newSize)
// TODO: do retry on 409 errors here?
if _, err := scaler.c.ReplicationControllers(namespace).Update(controller); err != nil {
if errors.IsInvalid(err) {
return ScaleError{ScaleUpdateInvalidFailure, controller.ResourceVersion, err}
if errors.IsConflict(err) {
return ScaleError{ScaleUpdateConflictFailure, controller.ResourceVersion, err}
}
return ScaleError{ScaleUpdateFailure, controller.ResourceVersion, err}
}
Expand All @@ -183,8 +179,11 @@ func (scaler *ReplicationControllerScaler) Scale(namespace, name string, newSize
if err != nil {
return err
}
return wait.Poll(waitForReplicas.Interval, waitForReplicas.Timeout,
client.ControllerHasDesiredReplicas(scaler.c, rc))
err = wait.Poll(waitForReplicas.Interval, waitForReplicas.Timeout, client.ControllerHasDesiredReplicas(scaler.c, rc))
if err == wait.ErrWaitTimeout {
return fmt.Errorf("timed out waiting for %q to be synced", name)
}
return err
}
return nil
}
Expand Down Expand Up @@ -215,10 +214,9 @@ func (scaler *ReplicaSetScaler) ScaleSimple(namespace, name string, precondition
}
}
rs.Spec.Replicas = int32(newSize)
// TODO: do retry on 409 errors here?
if _, err := scaler.c.ReplicaSets(namespace).Update(rs); err != nil {
if errors.IsInvalid(err) {
return ScaleError{ScaleUpdateInvalidFailure, rs.ResourceVersion, err}
if errors.IsConflict(err) {
return ScaleError{ScaleUpdateConflictFailure, rs.ResourceVersion, err}
}
return ScaleError{ScaleUpdateFailure, rs.ResourceVersion, err}
}
Expand All @@ -245,8 +243,11 @@ func (scaler *ReplicaSetScaler) Scale(namespace, name string, newSize uint, prec
if err != nil {
return err
}
return wait.Poll(waitForReplicas.Interval, waitForReplicas.Timeout,
client.ReplicaSetHasDesiredReplicas(scaler.c, rs))
err = wait.Poll(waitForReplicas.Interval, waitForReplicas.Timeout, client.ReplicaSetHasDesiredReplicas(scaler.c, rs))
if err == wait.ErrWaitTimeout {
return fmt.Errorf("timed out waiting for %q to be synced", name)
}
return err
}
return nil
}
Expand Down Expand Up @@ -283,8 +284,8 @@ func (scaler *JobScaler) ScaleSimple(namespace, name string, preconditions *Scal
parallelism := int32(newSize)
job.Spec.Parallelism = &parallelism
if _, err := scaler.c.Jobs(namespace).Update(job); err != nil {
if errors.IsInvalid(err) {
return ScaleError{ScaleUpdateInvalidFailure, job.ResourceVersion, err}
if errors.IsConflict(err) {
return ScaleError{ScaleUpdateConflictFailure, job.ResourceVersion, err}
}
return ScaleError{ScaleUpdateFailure, job.ResourceVersion, err}
}
Expand All @@ -311,8 +312,11 @@ func (scaler *JobScaler) Scale(namespace, name string, newSize uint, preconditio
if err != nil {
return err
}
return wait.Poll(waitForReplicas.Interval, waitForReplicas.Timeout,
client.JobHasDesiredParallelism(scaler.c, job))
err = wait.Poll(waitForReplicas.Interval, waitForReplicas.Timeout, client.JobHasDesiredParallelism(scaler.c, job))
if err == wait.ErrWaitTimeout {
return fmt.Errorf("timed out waiting for %q to be synced", name)
}
return err
}
return nil
}
Expand Down Expand Up @@ -348,8 +352,8 @@ func (scaler *DeploymentScaler) ScaleSimple(namespace, name string, precondition
// For now I'm falling back to regular Deployment update operation.
deployment.Spec.Replicas = int32(newSize)
if _, err := scaler.c.Deployments(namespace).Update(deployment); err != nil {
if errors.IsInvalid(err) {
return ScaleError{ScaleUpdateInvalidFailure, deployment.ResourceVersion, err}
if errors.IsConflict(err) {
return ScaleError{ScaleUpdateConflictFailure, deployment.ResourceVersion, err}
}
return ScaleError{ScaleUpdateFailure, deployment.ResourceVersion, err}
}
Expand All @@ -375,8 +379,11 @@ func (scaler *DeploymentScaler) Scale(namespace, name string, newSize uint, prec
if err != nil {
return err
}
return wait.Poll(waitForReplicas.Interval, waitForReplicas.Timeout,
client.DeploymentHasDesiredReplicas(scaler.c, deployment))
err = wait.Poll(waitForReplicas.Interval, waitForReplicas.Timeout, client.DeploymentHasDesiredReplicas(scaler.c, deployment))
if err == wait.ErrWaitTimeout {
return fmt.Errorf("timed out waiting for %q to be synced", name)
}
return err
}
return nil
}
71 changes: 49 additions & 22 deletions pkg/kubectl/scale_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,27 +30,36 @@ import (

type ErrorReplicationControllers struct {
testclient.FakeReplicationControllers
invalid bool
conflict bool
invalid bool
}

func (c *ErrorReplicationControllers) Update(controller *api.ReplicationController) (*api.ReplicationController, error) {
if c.invalid {
switch {
case c.invalid:
return nil, kerrors.NewInvalid(api.Kind(controller.Kind), controller.Name, nil)
case c.conflict:
return nil, kerrors.NewConflict(api.Resource(controller.Kind), controller.Name, nil)
}
return nil, errors.New("Replication controller update failure")
}

type ErrorReplicationControllerClient struct {
testclient.Fake
invalid bool
conflict bool
invalid bool
}

func (c *ErrorReplicationControllerClient) ReplicationControllers(namespace string) client.ReplicationControllerInterface {
return &ErrorReplicationControllers{testclient.FakeReplicationControllers{Fake: &c.Fake, Namespace: namespace}, c.invalid}
return &ErrorReplicationControllers{
FakeReplicationControllers: testclient.FakeReplicationControllers{Fake: &c.Fake, Namespace: namespace},
conflict: c.conflict,
invalid: c.invalid,
}
}

func TestReplicationControllerScaleRetry(t *testing.T) {
fake := &ErrorReplicationControllerClient{Fake: testclient.Fake{}, invalid: false}
fake := &ErrorReplicationControllerClient{Fake: testclient.Fake{}, conflict: true}
scaler := ReplicationControllerScaler{fake}
preconditions := ScalePrecondition{-1, ""}
count := uint(3)
Expand All @@ -63,7 +72,7 @@ func TestReplicationControllerScaleRetry(t *testing.T) {
t.Errorf("Expected an update failure to return pass = false, got pass = %v", pass)
}
if err != nil {
t.Errorf("Did not expect an error on update failure, got %v", err)
t.Errorf("Did not expect an error on update conflict failure, got %v", err)
}
preconditions = ScalePrecondition{3, ""}
scaleFunc = ScaleCondition(&scaler, &preconditions, namespace, name, count)
Expand All @@ -87,7 +96,7 @@ func TestReplicationControllerScaleInvalid(t *testing.T) {
t.Errorf("Expected an update failure to return pass = false, got pass = %v", pass)
}
e, ok := err.(ScaleError)
if err == nil || !ok || e.FailureType != ScaleUpdateInvalidFailure {
if err == nil || !ok || e.FailureType != ScaleUpdateFailure {
t.Errorf("Expected error on invalid update failure, got %v", err)
}
}
Expand Down Expand Up @@ -250,12 +259,16 @@ func TestValidateReplicationController(t *testing.T) {

type ErrorJobs struct {
testclient.FakeJobsV1
invalid bool
conflict bool
invalid bool
}

func (c *ErrorJobs) Update(job *batch.Job) (*batch.Job, error) {
if c.invalid {
return nil, kerrors.NewInvalid(batch.Kind(job.Kind), job.Name, nil)
switch {
case c.invalid:
return nil, kerrors.NewInvalid(api.Kind(job.Kind), job.Name, nil)
case c.conflict:
return nil, kerrors.NewConflict(api.Resource(job.Kind), job.Name, nil)
}
return nil, errors.New("Job update failure")
}
Expand All @@ -271,15 +284,20 @@ func (c *ErrorJobs) Get(name string) (*batch.Job, error) {

type ErrorJobClient struct {
testclient.FakeBatch
invalid bool
conflict bool
invalid bool
}

func (c *ErrorJobClient) Jobs(namespace string) client.JobInterface {
return &ErrorJobs{testclient.FakeJobsV1{Fake: &c.FakeBatch, Namespace: namespace}, c.invalid}
return &ErrorJobs{
FakeJobsV1: testclient.FakeJobsV1{Fake: &c.FakeBatch, Namespace: namespace},
conflict: c.conflict,
invalid: c.invalid,
}
}

func TestJobScaleRetry(t *testing.T) {
fake := &ErrorJobClient{FakeBatch: testclient.FakeBatch{}, invalid: false}
fake := &ErrorJobClient{FakeBatch: testclient.FakeBatch{}, conflict: true}
scaler := JobScaler{fake}
preconditions := ScalePrecondition{-1, ""}
count := uint(3)
Expand Down Expand Up @@ -336,7 +354,7 @@ func TestJobScaleInvalid(t *testing.T) {
t.Errorf("Expected an update failure to return pass = false, got pass = %v", pass)
}
e, ok := err.(ScaleError)
if err == nil || !ok || e.FailureType != ScaleUpdateInvalidFailure {
if err == nil || !ok || e.FailureType != ScaleUpdateFailure {
t.Errorf("Expected error on invalid update failure, got %v", err)
}
}
Expand Down Expand Up @@ -491,12 +509,16 @@ func TestValidateJob(t *testing.T) {

type ErrorDeployments struct {
testclient.FakeDeployments
invalid bool
conflict bool
invalid bool
}

func (c *ErrorDeployments) Update(deployment *extensions.Deployment) (*extensions.Deployment, error) {
if c.invalid {
return nil, kerrors.NewInvalid(extensions.Kind(deployment.Kind), deployment.Name, nil)
switch {
case c.invalid:
return nil, kerrors.NewInvalid(api.Kind(deployment.Kind), deployment.Name, nil)
case c.conflict:
return nil, kerrors.NewConflict(api.Resource(deployment.Kind), deployment.Name, nil)
}
return nil, errors.New("deployment update failure")
}
Expand All @@ -511,15 +533,20 @@ func (c *ErrorDeployments) Get(name string) (*extensions.Deployment, error) {

type ErrorDeploymentClient struct {
testclient.FakeExperimental
invalid bool
conflict bool
invalid bool
}

func (c *ErrorDeploymentClient) Deployments(namespace string) client.DeploymentInterface {
return &ErrorDeployments{testclient.FakeDeployments{Fake: &c.FakeExperimental, Namespace: namespace}, c.invalid}
return &ErrorDeployments{
FakeDeployments: testclient.FakeDeployments{Fake: &c.FakeExperimental, Namespace: namespace},
invalid: c.invalid,
conflict: c.conflict,
}
}

func TestDeploymentScaleRetry(t *testing.T) {
fake := &ErrorDeploymentClient{FakeExperimental: testclient.FakeExperimental{Fake: &testclient.Fake{}}, invalid: false}
fake := &ErrorDeploymentClient{FakeExperimental: testclient.FakeExperimental{}, conflict: true}
scaler := &DeploymentScaler{fake}
preconditions := &ScalePrecondition{-1, ""}
count := uint(3)
Expand Down Expand Up @@ -563,7 +590,7 @@ func TestDeploymentScale(t *testing.T) {
}

func TestDeploymentScaleInvalid(t *testing.T) {
fake := &ErrorDeploymentClient{FakeExperimental: testclient.FakeExperimental{Fake: &testclient.Fake{}}, invalid: true}
fake := &ErrorDeploymentClient{FakeExperimental: testclient.FakeExperimental{}, invalid: true}
scaler := DeploymentScaler{fake}
preconditions := ScalePrecondition{-1, ""}
count := uint(3)
Expand All @@ -576,7 +603,7 @@ func TestDeploymentScaleInvalid(t *testing.T) {
t.Errorf("Expected an update failure to return pass = false, got pass = %v", pass)
}
e, ok := err.(ScaleError)
if err == nil || !ok || e.FailureType != ScaleUpdateInvalidFailure {
if err == nil || !ok || e.FailureType != ScaleUpdateFailure {
t.Errorf("Expected error on invalid update failure, got %v", err)
}
}
Expand Down