Skip to content

Commit

Permalink
Merge pull request #27048 from kargakis/ignore-only-409-in-scaler
Browse files Browse the repository at this point in the history 
Automatic merge from submit-queue

kubectl: ignore only update conflicts in the scaler

@kubernetes/kubectl is there any reason to retry any other errors?
  • Loading branch information
k8s-merge-robot authored Jul 3, 2016
2 parents 4e046a8 + c7140b3 commit 2f9feef
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 46 deletions.
55 changes: 31 additions & 24 deletions pkg/kubectl/scale.go
View file
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
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

0 comments on commit 2f9feef

Please sign in to comment.