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

deployment controller: cancel deployers on new cancelled deployments #8418

Merged
merged 1 commit into from
Apr 11, 2016
Merged

deployment controller: cancel deployers on new cancelled deployments #8418

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
60 changes: 35 additions & 25 deletions pkg/deploy/controller/deployment/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ func (c *DeploymentController) Handle(deployment *kapi.ReplicationController) er
// transition to failed immediately.
if deployutil.IsDeploymentCancelled(deployment) {
nextStatus = deployapi.DeploymentStatusFailed
if err := c.cancelDeployerPods(deployment); err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a larger question - when we cancel, why don't we update the RC and then delete the deployer pods? Is that going to race with the deployment process pod controller? Or are you just trying to ensure it happens before and after?

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting q... I do think that's a larger investigation. Pretty sure this patch will do what we want for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spawned #8439

}
break
}

Expand Down Expand Up @@ -153,31 +156,8 @@ func (c *DeploymentController) Handle(deployment *kapi.ReplicationController) er
// Then, the deployment config controller will scale down the failed deployment
// and scale back up the last successful completed deployment.
if deployutil.IsDeploymentCancelled(deployment) {
deployerPods, err := c.podClient.getDeployerPodsFor(deployment.Namespace, deployment.Name)
if err != nil {
return fmt.Errorf("couldn't fetch deployer pods for %s while trying to cancel deployment: %v", deployutil.LabelForDeployment(deployment), err)
}
glog.V(4).Infof("Cancelling %d deployer pods for deployment %s", len(deployerPods), deployutil.LabelForDeployment(deployment))
zeroDelay := int64(1)
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)
}
glog.V(4).Infof("Cancelled deployer pod %s for deployment %s", deployerPod.Name, deployutil.LabelForDeployment(deployment))
}
}
if config, decodeErr := c.decodeConfig(deployment); decodeErr == nil {
c.recorder.Eventf(config, kapi.EventTypeNormal, "Cancelled", "Cancelled deployment %s", deployutil.LabelForDeployment(deployment))
} else {
c.recorder.Eventf(deployment, kapi.EventTypeNormal, "Cancelled", "Cancelled deployment")
if err := c.cancelDeployerPods(deployment); err != nil {
return err
}
}
case deployapi.DeploymentStatusFailed:
Expand Down Expand Up @@ -299,6 +279,36 @@ func (c *DeploymentController) makeDeployerPod(deployment *kapi.ReplicationContr
return pod, nil
}

func (c *DeploymentController) cancelDeployerPods(deployment *kapi.ReplicationController) error {
deployerPods, err := c.podClient.getDeployerPodsFor(deployment.Namespace, deployment.Name)
if err != nil {
return fmt.Errorf("couldn't fetch deployer pods for %s while trying to cancel deployment: %v", deployutil.LabelForDeployment(deployment), err)
}
glog.V(4).Infof("Cancelling %d deployer pods for deployment %s", len(deployerPods), deployutil.LabelForDeployment(deployment))
zeroDelay := int64(1)
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)
}
glog.V(4).Infof("Cancelled deployer pod %s for deployment %s", deployerPod.Name, deployutil.LabelForDeployment(deployment))
}
}
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")
}
return nil
}

// deploymentClient abstracts access to deployments.
type deploymentClient interface {
getDeployment(namespace, name string) (*kapi.ReplicationController, error)
Expand Down
53 changes: 53 additions & 0 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 @@ -640,6 +640,9 @@ func TestHandle_cancelNew(t *testing.T) {
t.Fatalf("unexpected call to make container")
return nil, nil
},
getDeployerPodsForFunc: func(namespace, name string) ([]kapi.Pod, error) {
return []kapi.Pod{}, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
return okContainer(), nil
Expand All @@ -661,6 +664,56 @@ func TestHandle_cancelNew(t *testing.T) {
}
}

func TestHandle_cancelNewWithDeployers(t *testing.T) {
var updatedDeployment *kapi.ReplicationController
updatedDeployer := false

deployment, _ := deployutil.MakeDeployment(deploytest.OkDeploymentConfig(1), kapi.Codecs.LegacyCodec(deployapi.SchemeGroupVersion))
deployment.Annotations[deployapi.DeploymentStatusAnnotation] = string(deployapi.DeploymentStatusNew)
deployment.Annotations[deployapi.DeploymentCancelledAnnotation] = deployapi.DeploymentCancelledAnnotationValue

controller := &DeploymentController{
decodeConfig: func(deployment *kapi.ReplicationController) (*deployapi.DeploymentConfig, error) {
return deployutil.DecodeDeploymentConfig(deployment, kapi.Codecs.LegacyCodec(deployapi.SchemeGroupVersion))
},
deploymentClient: &deploymentClientImpl{
updateDeploymentFunc: func(namespace string, deployment *kapi.ReplicationController) (*kapi.ReplicationController, error) {
updatedDeployment = deployment
return updatedDeployment, nil
},
},
podClient: &podClientImpl{
createPodFunc: func(namespace string, pod *kapi.Pod) (*kapi.Pod, error) {
t.Fatalf("unexpected call to make container")
return nil, nil
},
updatePodFunc: func(namespace string, pod *kapi.Pod) (*kapi.Pod, error) {
updatedDeployer = true
return nil, nil
},
getDeployerPodsForFunc: func(namespace, name string) ([]kapi.Pod, error) {
return []kapi.Pod{*relatedPod(deployment)}, nil
},
},
makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
return okContainer(), nil
},
recorder: &record.FakeRecorder{},
}

err := controller.Handle(deployment)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}

if e, a := deployapi.DeploymentStatusFailed, deployutil.DeploymentStatusFor(updatedDeployment); e != a {
t.Fatalf("expected deployment status %s, got %s", e, a)
}
if !updatedDeployer {
t.Fatalf("expected deployer update")
}
}

// TestHandle_cancelPendingRunning ensures that deployer pods are terminated
// for deployments in post-New phases.
func TestHandle_cancelPendingRunning(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/deploy/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func LabelForDeployment(deployment *api.ReplicationController) string {

// LabelForDeploymentConfig builds a string identifier for a DeploymentConfig.
func LabelForDeploymentConfig(config *deployapi.DeploymentConfig) string {
return fmt.Sprintf("%s/%s:%d", config.Namespace, config.Name, config.Status.LatestVersion)
return fmt.Sprintf("%s/%s", config.Namespace, config.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this change intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is confusing since we already name deployments after their version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this going to cause backwards compatibility issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? AFAIK we use it only for logging

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, my grep-fuu failed me.

}

// DeploymentNameForConfigVersion returns the name of the version-th deployment
Expand Down