Browse code

Deleting deployment hook pods after a deployment completes

Abhishek Gupta authored on 2015/06/06 07:52:32
Showing 2 changed files
... ...
@@ -8,6 +8,7 @@ import (
8 8
 	kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api"
9 9
 	kerrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
10 10
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client/record"
11
+	kutil "github.com/GoogleCloudPlatform/kubernetes/pkg/util"
11 12
 
12 13
 	deployapi "github.com/openshift/origin/pkg/deploy/api"
13 14
 	deployutil "github.com/openshift/origin/pkg/deploy/util"
... ...
@@ -138,18 +139,29 @@ func (c *DeploymentController) Handle(deployment *kapi.ReplicationController) er
138 138
 		// Nothing to do in this terminal state.
139 139
 		glog.V(4).Infof("Ignoring deployment %s (status %s)", deployutil.LabelForDeployment(deployment), currentStatus)
140 140
 	case deployapi.DeploymentStatusComplete:
141
-		// Automatically clean up successful pods.
142
-		// TODO: Could probably do a lookup here to skip the delete call, but it's
143
-		// not worth adding yet since (delete retries will only normally occur
144
-		// during full a re-sync).
145
-		podName := deployutil.DeployerPodNameFor(deployment)
146
-		if err := c.podClient.deletePod(deployment.Namespace, podName); err != nil {
147
-			if !kerrors.IsNotFound(err) {
148
-				return fmt.Errorf("couldn't delete completed deployer pod %s/%s for deployment %s: %v", deployment.Namespace, podName, deployutil.LabelForDeployment(deployment), err)
141
+		// now list any pods in the namespace that have the specified label
142
+		deployerPods, err := c.podClient.getDeployerPodsFor(deployment.Namespace, deployment.Name)
143
+		if err != nil {
144
+			return fmt.Errorf("couldn't fetch deployer pods for %s after successful completion: %v", deployutil.LabelForDeployment(deployment), err)
145
+		}
146
+		glog.V(4).Infof("Deleting %d deployer pods for deployment %s", len(deployerPods), deployutil.LabelForDeployment(deployment))
147
+		cleanedAll := true
148
+		for _, deployerPod := range deployerPods {
149
+			if err := c.podClient.deletePod(deployerPod.Namespace, deployerPod.Name); err != nil {
150
+				if !kerrors.IsNotFound(err) {
151
+					// if the pod deletion failed, then log the error and continue
152
+					// we will try to delete any remaining deployer pods and return an error later
153
+					kutil.HandleError(fmt.Errorf("couldn't delete completed deployer pod %s/%s for deployment %s: %v", deployment.Namespace, deployerPod.Name, deployutil.LabelForDeployment(deployment), err))
154
+					cleanedAll = false
155
+				}
156
+				// Already deleted
157
+			} else {
158
+				glog.V(4).Infof("Deleted completed deployer pod %s/%s for deployment %s", deployment.Namespace, deployerPod.Name, deployutil.LabelForDeployment(deployment))
149 159
 			}
150
-			// Already deleted
151
-		} else {
152
-			glog.V(4).Infof("Deleted completed deployer pod %s/%s for deployment %s", deployment.Namespace, podName, deployutil.LabelForDeployment(deployment))
160
+		}
161
+
162
+		if !cleanedAll {
163
+			return fmt.Errorf("couldn't clean up all deployer pods for %s", deployutil.LabelForDeployment(deployment))
153 164
 		}
154 165
 	}
155 166
 
... ...
@@ -2,6 +2,8 @@ package deployment
2 2
 
3 3
 import (
4 4
 	"fmt"
5
+	"reflect"
6
+	"sort"
5 7
 	"testing"
6 8
 
7 9
 	kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api"
... ...
@@ -348,9 +350,8 @@ func TestHandle_noop(t *testing.T) {
348 348
 // TestHandle_cleanupPodOk ensures that deployer pods are cleaned up for
349 349
 // deployments in a completed state.
350 350
 func TestHandle_cleanupPodOk(t *testing.T) {
351
-	podName := "pod"
352
-	deletedPodName := ""
353
-	deletedPodNamespace := ""
351
+	deployerPodNames := []string{"pod1", "pod2", "pod3"}
352
+	deletedPodNames := []string{}
354 353
 
355 354
 	controller := &DeploymentController{
356 355
 		decodeConfig: func(deployment *kapi.ReplicationController) (*deployapi.DeploymentConfig, error) {
... ...
@@ -368,10 +369,18 @@ func TestHandle_cleanupPodOk(t *testing.T) {
368 368
 				return nil, nil
369 369
 			},
370 370
 			deletePodFunc: func(namespace, name string) error {
371
-				deletedPodNamespace = namespace
372
-				deletedPodName = name
371
+				deletedPodNames = append(deletedPodNames, name)
373 372
 				return nil
374 373
 			},
374
+			getDeployerPodsForFunc: func(namespace, name string) ([]kapi.Pod, error) {
375
+				pods := []kapi.Pod{}
376
+				for _, podName := range deployerPodNames {
377
+					pod := *ttlNonZeroPod()
378
+					pod.Name = podName
379
+					pods = append(pods, pod)
380
+				}
381
+				return pods, nil
382
+			},
375 383
 		},
376 384
 		makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
377 385
 			t.Fatalf("unexpected call to make container")
... ...
@@ -384,24 +393,22 @@ func TestHandle_cleanupPodOk(t *testing.T) {
384 384
 	config := deploytest.OkDeploymentConfig(1)
385 385
 	deployment, _ := deployutil.MakeDeployment(config, kapi.Codec)
386 386
 	deployment.Annotations[deployapi.DeploymentStatusAnnotation] = string(deployapi.DeploymentStatusComplete)
387
-	deployment.Annotations[deployapi.DeploymentPodAnnotation] = podName
388 387
 	err := controller.Handle(deployment)
389 388
 
390 389
 	if err != nil {
391 390
 		t.Fatalf("unexpected error: %v", err)
392 391
 	}
393 392
 
394
-	if e, a := deployment.Namespace, deletedPodNamespace; e != a {
395
-		t.Fatalf("expected deleted pod namespace %s, got %s", e, a)
393
+	sort.Strings(deployerPodNames)
394
+	sort.Strings(deletedPodNames)
395
+	if !reflect.DeepEqual(deletedPodNames, deletedPodNames) {
396
+		t.Fatalf("pod deletions - expected: %v, actual: %v", deployerPodNames, deletedPodNames)
396 397
 	}
397 398
 
398
-	if e, a := podName, deletedPodName; e != a {
399
-		t.Fatalf("expected deleted pod name %s, got %s", e, a)
400
-	}
401 399
 }
402 400
 
403
-// TestHandle_cleanupPodNoop ensures that repeated attempts to clean up an
404
-// already-deleted deployer pod for a completed deployment safely do nothing.
401
+// TestHandle_cleanupPodNoop ensures that an attempt to delete pods are not made
402
+// if the deployer pods are not listed based on a label query
405 403
 func TestHandle_cleanupPodNoop(t *testing.T) {
406 404
 	controller := &DeploymentController{
407 405
 		decodeConfig: func(deployment *kapi.ReplicationController) (*deployapi.DeploymentConfig, error) {
... ...
@@ -419,7 +426,11 @@ func TestHandle_cleanupPodNoop(t *testing.T) {
419 419
 				return nil, nil
420 420
 			},
421 421
 			deletePodFunc: func(namespace, name string) error {
422
-				return kerrors.NewNotFound("Pod", name)
422
+				t.Fatalf("unexpected call to delete pod")
423
+				return nil
424
+			},
425
+			getDeployerPodsForFunc: func(namespace, name string) ([]kapi.Pod, error) {
426
+				return []kapi.Pod{}, nil
423 427
 			},
424 428
 		},
425 429
 		makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
... ...
@@ -433,7 +444,6 @@ func TestHandle_cleanupPodNoop(t *testing.T) {
433 433
 	config := deploytest.OkDeploymentConfig(1)
434 434
 	deployment, _ := deployutil.MakeDeployment(config, kapi.Codec)
435 435
 	deployment.Annotations[deployapi.DeploymentStatusAnnotation] = string(deployapi.DeploymentStatusComplete)
436
-	deployment.Annotations[deployapi.DeploymentPodAnnotation] = "pod"
437 436
 	err := controller.Handle(deployment)
438 437
 
439 438
 	if err != nil {
... ...
@@ -462,6 +472,9 @@ func TestHandle_cleanupPodFail(t *testing.T) {
462 462
 			deletePodFunc: func(namespace, name string) error {
463 463
 				return kerrors.NewInternalError(fmt.Errorf("test error"))
464 464
 			},
465
+			getDeployerPodsForFunc: func(namespace, name string) ([]kapi.Pod, error) {
466
+				return []kapi.Pod{{}}, nil
467
+			},
465 468
 		},
466 469
 		makeContainer: func(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
467 470
 			t.Fatalf("unexpected call to make container")
... ...
@@ -474,7 +487,6 @@ func TestHandle_cleanupPodFail(t *testing.T) {
474 474
 	config := deploytest.OkDeploymentConfig(1)
475 475
 	deployment, _ := deployutil.MakeDeployment(config, kapi.Codec)
476 476
 	deployment.Annotations[deployapi.DeploymentStatusAnnotation] = string(deployapi.DeploymentStatusComplete)
477
-	deployment.Annotations[deployapi.DeploymentPodAnnotation] = "pod"
478 477
 	err := controller.Handle(deployment)
479 478
 
480 479
 	if err == nil {