Skip to content

Bug 1329138: stop emitting events on update conflicts #8652

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

Merged
openshift-bot merged 1 commit into from
May 11, 2016
Merged

Bug 1329138: stop emitting events on update conflicts #8652

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/deploy/api/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,10 @@ func validateDeploymentStrategy(strategy *deployapi.DeploymentStrategy, pod *kap
} else {
errs = append(errs, validateCustomParams(strategy.CustomParams, fldPath.Child("customParams"))...)
}
case "":
errs = append(errs, field.Required(fldPath.Child("type"), "strategy type is required"))
default:
errs = append(errs, field.Invalid(fldPath.Child("type"), strategy.Type, "unsupported strategy type, use \"Custom\" instead and specify your own strategy"))
}

if strategy.Labels != nil {
Expand Down
71 changes: 25 additions & 46 deletions pkg/deploy/controller/deployment/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ type DeploymentController struct {
// podClient provides access to pods.
podClient podClient
// makeContainer knows how to make a container appropriate to execute a deployment strategy.
makeContainer func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error)
makeContainer func(strategy *deployapi.DeploymentStrategy) *kapi.Container
// decodeConfig knows how to decode the deploymentConfig from a deployment's annotations.
decodeConfig func(deployment *kapi.ReplicationController) (*deployapi.DeploymentConfig, error)
// recorder is used to record events.
Expand All @@ -46,6 +46,11 @@ type fatalError string

func (e fatalError) Error() string { return "fatal error handling deployment: " + string(e) }

// actionableError is an error on which users can act
type actionableError string

func (e actionableError) Error() string { return string(e) }

// Handle processes deployment and either creates a deployer pod or responds
// to a terminal deployment status.
func (c *DeploymentController) Handle(deployment *kapi.ReplicationController) error {
Expand Down Expand Up @@ -73,11 +78,6 @@ func (c *DeploymentController) Handle(deployment *kapi.ReplicationController) er
// annotation on it, and throw a retryable error.
existingPod, err := c.podClient.getPod(deployment.Namespace, deployutil.DeployerPodNameForDeployment(deployment.Name))
if err != nil && !kerrors.IsNotFound(err) {
if config, decodeErr := c.decodeConfig(deployment); decodeErr == nil {
c.recorder.Eventf(config, kapi.EventTypeWarning, "FailedCreate", "Error getting existing deployer pod for %s: %v", deployutil.LabelForDeployment(deployment), err)
} else {
c.recorder.Eventf(deployment, kapi.EventTypeWarning, "FailedCreate", "Error getting existing deployer pod for %s: %v", deployutil.LabelForDeployment(deployment), err)
}
return fmt.Errorf("couldn't fetch existing deployer pod for %s: %v", deployutil.LabelForDeployment(deployment), err)
}
if err == nil && existingPod != nil {
Expand All @@ -92,11 +92,7 @@ func (c *DeploymentController) Handle(deployment *kapi.ReplicationController) er
if deployutil.DeploymentNameFor(existingPod) != deployment.Name {
nextStatus = deployapi.DeploymentStatusFailed
deployment.Annotations[deployapi.DeploymentStatusReasonAnnotation] = deployapi.DeploymentFailedUnrelatedDeploymentExists
if config, decodeErr := c.decodeConfig(deployment); decodeErr == nil {
c.recorder.Eventf(config, kapi.EventTypeWarning, "FailedCreate", "Error creating deployer pod for %s since another pod with the same name (%q) exists", deployutil.LabelForDeployment(deployment), existingPod.Name)
} else {
c.recorder.Eventf(deployment, kapi.EventTypeWarning, "FailedCreate", "Error creating deployer pod for %s since another pod with the same name (%q) exists", deployutil.LabelForDeployment(deployment), existingPod.Name)
}
c.emitDeploymentEvent(deployment, kapi.EventTypeWarning, "FailedCreate", fmt.Sprintf("Error creating deployer pod since another pod with the same name (%q) exists", existingPod.Name))
glog.V(2).Infof("Couldn't create deployer pod for %s since an unrelated pod with the same name (%q) exists", deployutil.LabelForDeployment(deployment), existingPod.Name)
} else {
// Update to pending relative to the existing validated deployer pod.
Expand All @@ -110,22 +106,18 @@ func (c *DeploymentController) Handle(deployment *kapi.ReplicationController) er
// Generate a deployer pod spec.
podTemplate, err := c.makeDeployerPod(deployment)
if err != nil {
// TODO: Make this an oc status error
return fatalError(fmt.Sprintf("couldn't make deployer pod for %s: %v", deployutil.LabelForDeployment(deployment), err))
}
// Create the deployer pod.
deploymentPod, err := c.podClient.createPod(deployment.Namespace, podTemplate)
// Retry on error.
if err != nil {
if config, decodeErr := c.decodeConfig(deployment); decodeErr == nil {
c.recorder.Eventf(config, kapi.EventTypeWarning, "FailedCreate", "Error creating deployer pod for %s: %v", deployutil.LabelForDeployment(deployment), err)
} else {
c.recorder.Eventf(deployment, kapi.EventTypeWarning, "FailedCreate", "Error creating deployer pod for %s: %v", deployutil.LabelForDeployment(deployment), err)
}
return fmt.Errorf("couldn't create deployer pod for %s: %v", deployutil.LabelForDeployment(deployment), err)
}
deployment.Annotations[deployapi.DeploymentPodAnnotation] = deploymentPod.Name
nextStatus = deployapi.DeploymentStatusPending
glog.V(4).Infof("Created pod %s for deployment %s", deploymentPod.Name, deployutil.LabelForDeployment(deployment))
glog.V(4).Infof("Created deployer pod %s for deployment %s", deploymentPod.Name, deployutil.LabelForDeployment(deployment))
case deployapi.DeploymentStatusPending, deployapi.DeploymentStatusRunning:
// If the deployer pod has vanished, consider the deployment a failure.
deployerPodName := deployutil.DeployerPodNameForDeployment(deployment.Name)
Expand All @@ -134,11 +126,7 @@ func (c *DeploymentController) Handle(deployment *kapi.ReplicationController) er
nextStatus = deployapi.DeploymentStatusFailed
deployment.Annotations[deployapi.DeploymentStatusAnnotation] = string(nextStatus)
deployment.Annotations[deployapi.DeploymentStatusReasonAnnotation] = deployapi.DeploymentFailedDeployerPodNoLongerExists
if config, decodeErr := c.decodeConfig(deployment); decodeErr == nil {
c.recorder.Eventf(config, kapi.EventTypeWarning, "Failed", "Deployer pod %q has gone missing", deployerPodName)
} else {
c.recorder.Eventf(deployment, kapi.EventTypeWarning, "Failed", "Deployer pod %q has gone missing", deployerPodName)
}
c.emitDeploymentEvent(deployment, kapi.EventTypeWarning, "Failed", fmt.Sprintf("Deployer pod %q has gone missing", deployerPodName))
glog.V(4).Infof("Failing deployment %q because its deployer pod %q disappeared", deployutil.LabelForDeployment(deployment), deployerPodName)
break
} else {
Expand Down Expand Up @@ -195,18 +183,13 @@ func (c *DeploymentController) Handle(deployment *kapi.ReplicationController) er
}

if !cleanedAll {
return fmt.Errorf("couldn't clean up all deployer pods for %s", deployutil.LabelForDeployment(deployment))
return actionableError(fmt.Sprintf("couldn't clean up all deployer pods for %s", deployment.Name))
}
}

if deployutil.CanTransitionPhase(currentStatus, nextStatus) || deploymentScaled {
deployment.Annotations[deployapi.DeploymentStatusAnnotation] = string(nextStatus)
if _, err := c.deploymentClient.updateDeployment(deployment.Namespace, deployment); err != nil {
if config, decodeErr := c.decodeConfig(deployment); decodeErr == nil {
c.recorder.Eventf(config, kapi.EventTypeWarning, "FailedUpdate", "Cannot update deployment %s status to %s: %v", deployutil.LabelForDeployment(deployment), nextStatus, err)
} else {
c.recorder.Eventf(deployment, kapi.EventTypeWarning, "FailedUpdate", "Cannot update deployment %s status to %s: %v", deployutil.LabelForDeployment(deployment), nextStatus, err)
}
return fmt.Errorf("couldn't update deployment %s to status %s: %v", deployutil.LabelForDeployment(deployment), nextStatus, err)
}
glog.V(4).Infof("Updated deployment %s status from %s to %s (scale: %d)", deployutil.LabelForDeployment(deployment), currentStatus, nextStatus, deployment.Spec.Replicas)
Expand All @@ -222,10 +205,7 @@ func (c *DeploymentController) makeDeployerPod(deployment *kapi.ReplicationContr
return nil, err
}

container, err := c.makeContainer(&deploymentConfig.Spec.Strategy)
if err != nil {
return nil, err
}
container := c.makeContainer(&deploymentConfig.Spec.Strategy)

// Add deployment environment variables to the container.
envVars := []kapi.EnvVar{}
Expand Down Expand Up @@ -284,33 +264,32 @@ func (c *DeploymentController) cancelDeployerPods(deployment *kapi.ReplicationCo
}
glog.V(4).Infof("Cancelling %d deployer pods for deployment %s", len(deployerPods), deployutil.LabelForDeployment(deployment))
zeroDelay := int64(1)
anyCancelled := false
cleanedAll := len(deployerPods) > 0
for _, deployerPod := range deployerPods {
// Set the ActiveDeadlineSeconds on the pod so it's terminated very soon.
if deployerPod.Spec.ActiveDeadlineSeconds == nil || *deployerPod.Spec.ActiveDeadlineSeconds != zeroDelay {
deployerPod.Spec.ActiveDeadlineSeconds = &zeroDelay
if _, err := c.podClient.updatePod(deployerPod.Namespace, &deployerPod); err != nil {
if config, decodeErr := c.decodeConfig(deployment); decodeErr == nil {
c.recorder.Eventf(config, kapi.EventTypeWarning, "FailedCancellation", "Error cancelling deployer pod %s for deployment %s: %v", deployerPod.Name, deployutil.LabelForDeployment(deployment), err)
} else {
c.recorder.Eventf(deployment, kapi.EventTypeWarning, "FailedCancellation", "Error cancelling deployer pod %s for deployment %s: %v", deployerPod.Name, deployutil.LabelForDeployment(deployment), err)
}
return fmt.Errorf("couldn't cancel deployer pod %s for deployment %s: %v", deployerPod.Name, deployutil.LabelForDeployment(deployment), err)
cleanedAll = false
utilruntime.HandleError(fmt.Errorf("couldn't cancel deployer pod %s for deployment %s: %v", deployerPod.Name, deployutil.LabelForDeployment(deployment), err))
}
anyCancelled = true
glog.V(4).Infof("Cancelled deployer pod %s for deployment %s", deployerPod.Name, deployutil.LabelForDeployment(deployment))
}
}
if anyCancelled {
if config, decodeErr := c.decodeConfig(deployment); decodeErr == nil && len(deployerPods) > 0 {
c.recorder.Eventf(config, kapi.EventTypeNormal, "Cancelled", "Cancelled deployer pods for deployment %s", deployutil.LabelForDeployment(deployment))
} else if len(deployerPods) > 0 {
c.recorder.Eventf(deployment, kapi.EventTypeNormal, "Cancelled", "Cancelled deployer pods")
}
if cleanedAll {
c.emitDeploymentEvent(deployment, kapi.EventTypeNormal, "Cancelled", "Cancelled all deployer pods")
}
return nil
}

func (c *DeploymentController) emitDeploymentEvent(deployment *kapi.ReplicationController, eventType, title, message string) {
if config, _ := c.decodeConfig(deployment); config != nil {
c.recorder.Eventf(config, eventType, title, fmt.Sprintf("%s: %s", deployment.Name, message))
} else {
c.recorder.Eventf(deployment, eventType, title, message)
}
}

// deploymentClient abstracts access to deployments.
type deploymentClient interface {
getDeployment(namespace, name string) (*kapi.ReplicationController, error)
Expand Down
66 changes: 33 additions & 33 deletions pkg/deploy/controller/deployment/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ func TestHandle_createPodOk(t *testing.T) {
return pod, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
return expectedContainer, nil
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
return expectedContainer
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -132,7 +132,7 @@ func TestHandle_makeContainerFail(t *testing.T) {

controller := &DeploymentController{
decodeConfig: func(deployment *kapi.ReplicationController) (*deployapi.DeploymentConfig, error) {
return deployutil.DecodeDeploymentConfig(deployment, kapi.Codecs.LegacyCodec(deployapi.SchemeGroupVersion))
return nil, fmt.Errorf("invalid serialized object reference")
},
deploymentClient: &deploymentClientImpl{
updateDeploymentFunc: func(namespace string, deployment *kapi.ReplicationController) (*kapi.ReplicationController, error) {
Expand All @@ -149,8 +149,8 @@ func TestHandle_makeContainerFail(t *testing.T) {
return nil, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
return nil, fmt.Errorf("couldn't make container")
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
return nil
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -192,8 +192,8 @@ func TestHandle_createPodFail(t *testing.T) {
return nil, fmt.Errorf("Failed to create pod %s", pod.Name)
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
return okContainer(), nil
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
return okContainer()
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -241,8 +241,8 @@ func TestHandle_deployerPodAlreadyExists(t *testing.T) {
return nil, kerrors.NewAlreadyExists(kapi.Resource("Pod"), pod.Name)
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
return okContainer(), nil
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
return okContainer()
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -291,8 +291,8 @@ func TestHandle_unrelatedPodAlreadyExists(t *testing.T) {
return nil, kerrors.NewAlreadyExists(kapi.Resource("Pod"), pod.Name)
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
return okContainer(), nil
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
return okContainer()
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -338,9 +338,9 @@ func TestHandle_noop(t *testing.T) {
return &kapi.Pod{}, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
t.Fatalf("unexpected call to make container")
return nil, nil
return nil
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -392,9 +392,9 @@ func TestHandle_failedTest(t *testing.T) {
return nil, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
t.Fatalf("unexpected call to make container")
return nil, nil
return nil
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -452,9 +452,9 @@ func TestHandle_cleanupPodOk(t *testing.T) {
return pods, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
t.Fatalf("unexpected call to make container")
return nil, nil
return nil
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -513,9 +513,9 @@ func TestHandle_cleanupPodOkTest(t *testing.T) {
return pods, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
t.Fatalf("unexpected call to make container")
return nil, nil
return nil
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -570,9 +570,9 @@ func TestHandle_cleanupPodNoop(t *testing.T) {
return []kapi.Pod{}, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
t.Fatalf("unexpected call to make container")
return nil, nil
return nil
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -613,9 +613,9 @@ func TestHandle_cleanupPodFail(t *testing.T) {
return []kapi.Pod{{}}, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
t.Fatalf("unexpected call to make container")
return nil, nil
return nil
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -653,8 +653,8 @@ func TestHandle_cancelNew(t *testing.T) {
return []kapi.Pod{}, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
return okContainer(), nil
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
return okContainer()
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -704,8 +704,8 @@ func TestHandle_cancelNewWithDeployers(t *testing.T) {
return []kapi.Pod{*relatedPod(deployment)}, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
return okContainer(), nil
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
return okContainer()
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -756,8 +756,8 @@ func TestHandle_cancelPendingRunning(t *testing.T) {
return pods, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
return okContainer(), nil
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
return okContainer()
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -810,8 +810,8 @@ func TestHandle_deployerPodDisappeared(t *testing.T) {
return nil, kerrors.NewNotFound(kapi.Resource("Pod"), name)
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
return okContainer(), nil
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
return okContainer()
},
recorder: &record.FakeRecorder{},
}
Expand Down Expand Up @@ -857,8 +857,8 @@ func TestDeployerCustomLabelsAndAnnotations(t *testing.T) {
return pod, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
return okContainer(), nil
makeContainer: func(strategy *deployapi.DeploymentStrategy) *kapi.Container {
return okContainer()
},
}

Expand Down
Loading