Enable custom deployments to have rolling/recreate as well

Allow customParams to be specified when type is Recreate or Rolling.
Allow image to be empty, add validation for resources and environment.
Add a custom deployment extended test.

Author: smarterclayton

pkg/deploy/api/types.go

@@ -29,13 +29,16 @@ type DeploymentStrategy struct {
 	// Type is the name of a deployment strategy.
 	Type DeploymentStrategyType
 
-	// CustomParams are the input to the Custom deployment strategy.
-	CustomParams *CustomDeploymentStrategyParams
 	// RecreateParams are the input to the Recreate deployment strategy.
 	RecreateParams *RecreateDeploymentStrategyParams
 	// RollingParams are the input to the Rolling deployment strategy.
 	RollingParams *RollingDeploymentStrategyParams
 
+	// CustomParams are the input to the Custom deployment strategy, and may also
+	// be specified for the Recreate and Rolling strategies to customize the execution
+	// process that runs the deployment.
+	CustomParams *CustomDeploymentStrategyParams
+
 	// Resources contains resource requirements to execute the deployment
 	Resources kapi.ResourceRequirements
 	// Labels is a set of key, value pairs added to custom deployer and lifecycle pre/post hook pods.
@@ -50,7 +53,7 @@ type DeploymentStrategyType string
 const (
 	// DeploymentStrategyTypeRecreate is a simple strategy suitable as a default.
 	DeploymentStrategyTypeRecreate DeploymentStrategyType = "Recreate"
-	// DeploymentStrategyTypeCustom is a user defined strategy.
+	// DeploymentStrategyTypeCustom is a user defined strategy. It is optional to set.
 	DeploymentStrategyTypeCustom DeploymentStrategyType = "Custom"
 	// DeploymentStrategyTypeRolling uses the Kubernetes RollingUpdater.
 	DeploymentStrategyTypeRolling DeploymentStrategyType = "Rolling"

pkg/deploy/api/validation/validation.go

@@ -86,6 +86,10 @@ func validateDeploymentStrategy(strategy *deployapi.DeploymentStrategy, pod *kap
 		errs = append(errs, field.Required(fldPath.Child("type"), ""))
 	}
 
+	if strategy.CustomParams != nil {
+		errs = append(errs, validateCustomParams(strategy.CustomParams, fldPath.Child("customParams"))...)
+	}
+
 	switch strategy.Type {
 	case deployapi.DeploymentStrategyTypeRecreate:
 		if strategy.RecreateParams != nil {
@@ -100,8 +104,12 @@ func validateDeploymentStrategy(strategy *deployapi.DeploymentStrategy, pod *kap
 	case deployapi.DeploymentStrategyTypeCustom:
 		if strategy.CustomParams == nil {
 			errs = append(errs, field.Required(fldPath.Child("customParams"), ""))
-		} else {
-			errs = append(errs, validateCustomParams(strategy.CustomParams, fldPath.Child("customParams"))...)
+		}
+		if strategy.RollingParams != nil {
+			errs = append(errs, validateRollingParams(strategy.RollingParams, pod, fldPath.Child("rollingParams"))...)
+		}
+		if strategy.RecreateParams != nil {
+			errs = append(errs, validateRecreateParams(strategy.RecreateParams, pod, fldPath.Child("recreateParams"))...)
 		}
 	}
 
@@ -112,17 +120,15 @@ func validateDeploymentStrategy(strategy *deployapi.DeploymentStrategy, pod *kap
 		errs = append(errs, validation.ValidateAnnotations(strategy.Annotations, fldPath.Child("annotations"))...)
 	}
 
-	// TODO: validate resource requirements (prereq: https://github.com/kubernetes/kubernetes/pull/7059)
+	errs = append(errs, validation.ValidateResourceRequirements(&strategy.Resources, fldPath.Child("resources"))...)
 
 	return errs
 }
 
 func validateCustomParams(params *deployapi.CustomDeploymentStrategyParams, fldPath *field.Path) field.ErrorList {
 	errs := field.ErrorList{}
 
-	if len(params.Image) == 0 {
-		errs = append(errs, field.Required(fldPath.Child("image"), ""))
-	}
+	errs = append(errs, validateEnv(params.Environment, fldPath.Child("environment"))...)
 
 	return errs
 }
@@ -207,7 +213,7 @@ func validateEnv(vars []kapi.EnvVar, fldPath *field.Path) field.ErrorList {
 
 	for i, ev := range vars {
 		vErrs := field.ErrorList{}
-		idxPath := fldPath.Child("name").Index(i)
+		idxPath := fldPath.Index(i).Child("name")
 		if len(ev.Name) == 0 {
 			vErrs = append(vErrs, field.Required(idxPath, ""))
 		}

pkg/deploy/api/validation/validation_test.go

@@ -240,22 +240,26 @@ func TestValidateDeploymentConfigMissingFields(t *testing.T) {
 			field.ErrorTypeRequired,
 			"spec.strategy.customParams",
 		},
-		"missing spec.strategy.customParams.image": {
+		"invalid spec.strategy.customParams.environment": {
 			api.DeploymentConfig{
 				ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "bar"},
 				Spec: api.DeploymentConfigSpec{
 					Replicas: 1,
 					Triggers: manualTrigger(),
 					Selector: test.OkSelector(),
 					Strategy: api.DeploymentStrategy{
-						Type:         api.DeploymentStrategyTypeCustom,
-						CustomParams: &api.CustomDeploymentStrategyParams{},
+						Type: api.DeploymentStrategyTypeCustom,
+						CustomParams: &api.CustomDeploymentStrategyParams{
+							Environment: []kapi.EnvVar{
+								{Name: "A=B"},
+							},
+						},
 					},
 					Template: test.OkPodTemplate(),
 				},
 			},
-			field.ErrorTypeRequired,
-			"spec.strategy.customParams.image",
+			field.ErrorTypeInvalid,
+			"spec.strategy.customParams.environment[0].name",
 		},
 		"missing spec.strategy.recreateParams.pre.failurePolicy": {
 			api.DeploymentConfig{

pkg/deploy/controller/deployment/controller.go

@@ -265,6 +265,8 @@ func (c *DeploymentController) makeDeployerPod(deployment *kapi.ReplicationContr
 				},
 			},
 			ActiveDeadlineSeconds: &maxDeploymentDurationSeconds,
+			DNSPolicy:             deployment.Spec.Template.Spec.DNSPolicy,
+			ImagePullSecrets:      deployment.Spec.Template.Spec.ImagePullSecrets,
 			// Setting the node selector on the deployer pod so that it is created
 			// on the same set of nodes as the pods.
 			NodeSelector:       deployment.Spec.Template.Spec.NodeSelector,

pkg/deploy/controller/deployment/factory.go

@@ -12,6 +12,7 @@ import (
 	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/util/flowcontrol"
 	utilruntime "k8s.io/kubernetes/pkg/util/runtime"
+	"k8s.io/kubernetes/pkg/util/sets"
 	"k8s.io/kubernetes/pkg/watch"
 
 	controller "github.com/openshift/origin/pkg/controller"
@@ -128,36 +129,50 @@ func (factory *DeploymentControllerFactory) Create() controller.RunnableControll
 //   1. For the Recreate and Rolling strategies, strategy, use the factory's
 //      DeployerImage as the container image, and the factory's Environment
 //      as the container environment.
-//   2. For all Custom strategy, use the strategy's image for the container
-//      image, and use the combination of the factory's Environment and the
-//      strategy's environment as the container environment.
+//   2. For all Custom strategies, or if the CustomParams field is set, use
+//      the strategy's image for the container image, and use the combination
+//      of the factory's Environment and the strategy's environment as the
+//      container environment.
 //
 // An error is returned if the deployment strategy type is not supported.
 func (factory *DeploymentControllerFactory) makeContainer(strategy *deployapi.DeploymentStrategy) (*kapi.Container, error) {
+	image := factory.DeployerImage
+	var environment []kapi.EnvVar
+	var command []string
+
+	set := sets.NewString()
+	// Use user-defined values from the strategy input.
+	if p := strategy.CustomParams; p != nil {
+		if len(p.Image) > 0 {
+			image = p.Image
+		}
+		if len(p.Command) > 0 {
+			command = p.Command
+		}
+		for _, env := range strategy.CustomParams.Environment {
+			set.Insert(env.Name)
+			environment = append(environment, env)
+		}
+	}
+
 	// Set default environment values
-	environment := []kapi.EnvVar{}
 	for _, env := range factory.Environment {
+		if set.Has(env.Name) {
+			continue
+		}
 		environment = append(environment, env)
 	}
 
 	// Every strategy type should be handled here.
 	switch strategy.Type {
-	case deployapi.DeploymentStrategyTypeRecreate, deployapi.DeploymentStrategyTypeRolling:
-		// Use the factory-configured image.
-		return &kapi.Container{
-			Image: factory.DeployerImage,
-			Env:   environment,
-		}, nil
-	case deployapi.DeploymentStrategyTypeCustom:
-		// Use user-defined values from the strategy input.
-		for _, env := range strategy.CustomParams.Environment {
-			environment = append(environment, env)
-		}
-		return &kapi.Container{
-			Image: strategy.CustomParams.Image,
-			Env:   environment,
-		}, nil
+	case deployapi.DeploymentStrategyTypeRecreate, deployapi.DeploymentStrategyTypeRolling, deployapi.DeploymentStrategyTypeCustom:
 	default:
 		return nil, fmt.Errorf("unsupported deployment strategy type: %s", strategy.Type)
 	}
+
+	return &kapi.Container{
+		Image:   image,
+		Command: command,
+		Env:     environment,
+	}, nil
 }

pkg/deploy/strategy/support/lifecycle.go

@@ -160,7 +160,7 @@ func (e *HookExecutor) executeExecNewPod(hook *deployapi.LifecycleHook, deployme
 
 	// Track whether the pod has already run to completion and avoid showing logs
 	// or the Success message twice.
-	completed := false
+	completed, created := false, false
 
 	// Try to create the pod.
 	pod, err := e.podClient.CreatePod(deployment.Namespace, podSpec)
@@ -172,6 +172,7 @@ func (e *HookExecutor) executeExecNewPod(hook *deployapi.LifecycleHook, deployme
 		pod = podSpec
 		pod.Namespace = deployment.Namespace
 	} else {
+		created = true
 		fmt.Fprintf(e.out, "--> %s: Running hook pod ...\n", label)
 	}
 
@@ -200,7 +201,9 @@ waitLoop:
 				wg.Done()
 				break waitLoop
 			}
-			fmt.Fprintf(e.out, "--> %s: Hook pod is already running ...\n", label)
+			if !created {
+				fmt.Fprintf(e.out, "--> %s: Hook pod is already running ...\n", label)
+			}
 			go once.Do(func() { e.readPodLogs(pod, wg) })
 			break waitLoop
 		default:

test/extended/deployments/deployments.go

@@ -28,6 +28,7 @@ var _ = g.Describe("deploymentconfigs", func() {
 	var (
 		deploymentFixture       = exutil.FixturePath("..", "extended", "fixtures", "test-deployment-test.yaml")
 		simpleDeploymentFixture = exutil.FixturePath("..", "extended", "fixtures", "deployment-simple.yaml")
+		customDeploymentFixture = exutil.FixturePath("..", "extended", "fixtures", "custom-deployment.yaml")
 		oc                      = exutil.NewCLI("cli-deployment", exutil.KubeConfigPath())
 	)
 
@@ -185,6 +186,29 @@ var _ = g.Describe("deploymentconfigs", func() {
 			}
 		})
 	})
+
+	g.Describe("with custom deployments", func() {
+		g.It("should run the custom deployment steps [Conformance]", func() {
+			out, err := oc.Run("create").Args("-f", customDeploymentFixture).Output()
+			o.Expect(err).NotTo(o.HaveOccurred())
+
+			o.Expect(waitForLatestCondition(oc, "custom-deployment", deploymentRunTimeout, deploymentRunning)).NotTo(o.HaveOccurred())
+
+			out, err = oc.Run("logs").Args("-f", "dc/custom-deployment").Output()
+			o.Expect(err).NotTo(o.HaveOccurred())
+			g.By(fmt.Sprintf("checking the logs for substrings\n%s", out))
+			o.Expect(out).To(o.ContainSubstring("--> pre: Running hook pod ..."))
+			o.Expect(out).To(o.ContainSubstring("test pre hook executed"))
+			o.Expect(out).To(o.ContainSubstring("--> Scaling custom-deployment-1 to 2"))
+			o.Expect(out).To(o.ContainSubstring("--> Reached 50%"))
+			o.Expect(out).To(o.ContainSubstring("Halfway"))
+			o.Expect(out).To(o.ContainSubstring("Finished"))
+			o.Expect(out).To(o.ContainSubstring("--> Success"))
+
+			g.By("verifying the deployment is marked complete")
+			o.Expect(waitForLatestCondition(oc, "custom-deployment", deploymentRunTimeout, deploymentReachedCompletion)).NotTo(o.HaveOccurred())
+		})
+	})
 })
 
 func deploymentStatuses(rcs []kapi.ReplicationController) []string {

test/extended/fixtures/custom-deployment.yaml

@@ -0,0 +1,43 @@
+apiVersion: v1
+kind: DeploymentConfig
+metadata:
+  name: custom-deployment
+spec:
+  replicas: 2
+  selector:
+    name: custom-deployment
+  strategy:
+    type: Rolling
+    rollingParams:
+      pre:
+        failurePolicy: Abort
+        execNewPod:
+          containerName: myapp
+          command:
+          - /bin/echo
+          - test pre hook executed
+    customParams:
+      command:
+      - /bin/sh
+      - -c
+      - |
+        set -e
+        openshift-deploy --until=50%
+        echo Halfway
+        openshift-deploy
+        echo Finished
+  template:
+    metadata:
+      labels:
+        name: custom-deployment
+    spec:
+      terminationGracePeriodSeconds: 0
+      containers:
+      - image: "docker.io/centos:centos7"
+        imagePullPolicy: IfNotPresent
+        name: myapp
+        command:
+        - /bin/sleep
+        - "100"
+  triggers:
+  - type: ConfigChange

test/extended/fixtures/test-deployment-test.yaml

@@ -21,6 +21,7 @@ spec:
       labels:
         name: deployment-test
     spec:
+      terminationGracePeriodSeconds: 0
       containers:
       - image: "docker.io/centos:centos7"
         imagePullPolicy: IfNotPresent