feat: Extend workflowDefaults to full Workflow and clean up docs and …

…code (argoproj#2508)

api/openapi-spec/swagger.json

@@ -2954,7 +2954,7 @@
           "format": "int32"
         },
         "ttlStrategy": {
-          "title": "TTLStrategy limits the lifetime of a Workflow that has finished execution depending on if it\nSucceeded or Failed. If this struct is set, once the Workflow finishes, it will be\ndeleted after the time to live expires. If this field is unset,\nthe controller config map will hold the default values\nUpdate",
+          "description": "TTLStrategy limits the lifetime of a Workflow that has finished execution depending on if it\nSucceeded or Failed. If this struct is set, once the Workflow finishes, it will be\ndeleted after the time to live expires. If this field is unset,\nthe controller config map will hold the default values.",
           "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.TTLStrategy"
         },
         "volumeClaimTemplates": {

config/config.go

@@ -51,11 +51,13 @@ type Config struct {
 	// controller watches workflows and pods that *are not* labeled with an instance id.
 	InstanceID string `json:"instanceID,omitempty"`
 
+	// MetricsConfig specifies configuration for metrics emission
 	MetricsConfig PrometheusConfig `json:"metricsConfig,omitempty"`
 
 	// FeatureFlags for general/experimental features
 	FeatureFlags FeatureFlags `json:"featureFlags,omitempty"`
 
+	// TelemetryConfig specifies configuration for telemetry emission
 	TelemetryConfig PrometheusConfig `json:"telemetryConfig,omitempty"`
 
 	// Parallelism limits the max total parallel workflows that can execute at the same time
@@ -70,8 +72,8 @@ type Config struct {
 	// Config customized Docker Sock path
 	DockerSockPath string `json:"dockerSockPath,omitempty"`
 
-	// Default workflow spec, will be adde to workflow if the parameters are not set in the workflow
-	DefautWorkflowSpec *wfv1.WorkflowSpec `json:"workflowDefaults,omitempty"`
+	// WorkflowDefaults are values that will apply to all Workflows from this controller, unless overridden on the Workflow-level
+	WorkflowDefaults *wfv1.Workflow `json:"workflowDefaults,omitempty"`
 
 	// PodSpecLogStrategy enable the logging of podspec on controller log.
 	PodSpecLogStrategy PodSpecLogStrategy `json:"podSpecLogStrategy,omitempty"`

docs/default-workflow-specs.md

@@ -4,34 +4,54 @@
 
 > v2.7 and after
 
-It's possible to set default workflow specs which will be written to all workflows if the spec of interest is not set. This can be configurated through the 
-workflow controller config [map](../workflow/config/config.go#L11) and the field [DefaultWorkflowSpec](../workflow/config/config.go#L69). 
+## Introduction
 
+Default Workflow spec values can be set at the controller config map that will apply to all Workflows executed from said controller.
+If a Workflow has a value that also has a default value set in the config map, thw Workflow's value will take precedence.
 
-In order to edit the Default workflow spec for a controller, edit the workflow config map: 
+## Setting Default Workflow Values
 
+Default Workflow values can be specified by adding them under the `workflowDefaults` key in the [`workflow-controller-configmap`](./workflow-controller-configmap.yaml).
+Values can be added as the would under the `Workflow.spec` tag.
 
-```bash 
-kubectl edit cm/workflow-controller-configmap
-```
-
-
-As an example the time for a argo workflow to live after finish can be set, in the spec this field is known as ```secondsAfterCompletion``` in the ```ttlStrategy```. Example of how the config map could look with this filed can be found [here](./workflow-controller-configmap.yaml).
-
-In order to test it a example workflow can be submited, in this case the [coinflip example](../examples/coinflip.yaml), the following can be run:
+For example, to specify default values that would partially produce the following `Workflow`:
 
-```bash 
-argo submit ./examples/coinflip.yaml
+```yaml
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: gc-ttl-
+  annotations:
+    argo: workflows
+  labels:
+    foo: bar
+spec:
+  ttlStrategy:
+    secondsAfterSuccess: 5     # Time to live after workflow is successful
+  parallelism: 3
 ```
 
-to verify that the the defaultd are set run 
-
-```bash
-argo get [YOUR_ARGO_WORKFLOW_NAME]
-```
+The following would be specified in the Config Map:
 
-You should then see the field, Ttl Strategy populated
 ```yaml
-Ttl Strategy:
-  Seconds After Completion:  10
-```
+# This file describes the config settings available in the workflow controller configmap
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: workflow-controller-configmap
+data:
+  config: |
+
+    # Default values that will apply to all Workflows from this controller, unless overridden on the Workflow-level
+    workflowDefaults:
+      metadata:
+        annotations:
+          argo: workflows
+        labels:
+          foo: bar
+      spec:
+        ttlStrategy:
+          secondsAfterSuccess: 5
+        parallelism: 3
+
+```

docs/workflow-controller-configmap.yaml

@@ -171,7 +171,15 @@ data:
       #     name: argo-mysql-config
       #     key: password
 
-    # default workflow spec, follow the workflow spec, more info: https://github.com/argoproj/argo/blob/master/examples/README.md#the-structure-of-workflow-specs
+    # Default values that will apply to all Workflows from this controller, unless overridden on the Workflow-level
+    # See more: docs/default-workflow-specs.yaml
     workflowDefaults:
-      ttlStrategy:
-        secondsAfterCompletion: 10
+      metadata:
+        annotations:
+          argo: workflows
+        labels:
+          foo: bar
+      spec:
+        ttlStrategy:
+          secondsAfterSuccess: 5
+        parallelism: 3

go.mod

@@ -76,7 +76,7 @@ require (
 	golang.org/x/crypto v0.0.0-20200128174031-69ecbb4d6d5d
 	golang.org/x/net v0.0.0-20200301022130-244492dfa37a
 	golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d
-	golang.org/x/tools v0.0.0-20200323210725-ef1313dc6d0a // indirect
+	golang.org/x/tools v0.0.0-20200325010219-a49f79bcc224 // indirect
 	google.golang.org/api v0.20.0
 	google.golang.org/genproto v0.0.0-20200317114155-1f3552e48f24
 	google.golang.org/grpc v1.28.0

go.sum

@@ -607,8 +607,8 @@ golang.org/x/tools v0.0.0-20200207183749-b753a1ba74fa/go.mod h1:TB2adYChydJhpapK
 golang.org/x/tools v0.0.0-20200212150539-ea181f53ac56/go.mod h1:TB2adYChydJhpapKDTa4BR/hXlZSLoq2Wpct/0txZ28=
 golang.org/x/tools v0.0.0-20200224181240-023911ca70b2/go.mod h1:TB2adYChydJhpapKDTa4BR/hXlZSLoq2Wpct/0txZ28=
 golang.org/x/tools v0.0.0-20200317043434-63da46f3035e/go.mod h1:Sl4aGygMT6LrqrWclx+PTx3U+LnKx/seiNR+3G19Ar8=
-golang.org/x/tools v0.0.0-20200323210725-ef1313dc6d0a h1:QpsrlnR31DN2tCItRzamUXN4oAe3U00ru8f0ik9HSYI=
-golang.org/x/tools v0.0.0-20200323210725-ef1313dc6d0a/go.mod h1:Sl4aGygMT6LrqrWclx+PTx3U+LnKx/seiNR+3G19Ar8=
+golang.org/x/tools v0.0.0-20200325010219-a49f79bcc224 h1:azwY/v0y0K4mFHVsg5+UrTgchqALYWpqVo6vL5OmkmI=
+golang.org/x/tools v0.0.0-20200325010219-a49f79bcc224/go.mod h1:Sl4aGygMT6LrqrWclx+PTx3U+LnKx/seiNR+3G19Ar8=
 golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898 h1:/atklqdjdhuosWIl6AIbOeHJjicWYPqR9bpxqxYG2pA=
 golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=

pkg/apiclient/cronworkflow/cron-workflow.swagger.json

@@ -1699,7 +1699,7 @@
         },
         "ttlStrategy": {
           "$ref": "#/definitions/github.com.argoproj.argo.pkg.apis.workflow.v1alpha1.TTLStrategy",
-          "title": "TTLStrategy limits the lifetime of a Workflow that has finished execution depending on if it\nSucceeded or Failed. If this struct is set, once the Workflow finishes, it will be\ndeleted after the time to live expires. If this field is unset,\nthe controller config map will hold the default values\nUpdate"
+          "description": "TTLStrategy limits the lifetime of a Workflow that has finished execution depending on if it\nSucceeded or Failed. If this struct is set, once the Workflow finishes, it will be\ndeleted after the time to live expires. If this field is unset,\nthe controller config map will hold the default values."
         },
         "activeDeadlineSeconds": {
           "type": "string",

pkg/apiclient/workflow/workflow.swagger.json

@@ -2106,7 +2106,7 @@
         },
         "ttlStrategy": {
           "$ref": "#/definitions/github.com.argoproj.argo.pkg.apis.workflow.v1alpha1.TTLStrategy",
-          "title": "TTLStrategy limits the lifetime of a Workflow that has finished execution depending on if it\nSucceeded or Failed. If this struct is set, once the Workflow finishes, it will be\ndeleted after the time to live expires. If this field is unset,\nthe controller config map will hold the default values\nUpdate"
+          "description": "TTLStrategy limits the lifetime of a Workflow that has finished execution depending on if it\nSucceeded or Failed. If this struct is set, once the Workflow finishes, it will be\ndeleted after the time to live expires. If this field is unset,\nthe controller config map will hold the default values."
         },
         "activeDeadlineSeconds": {
           "type": "string",

pkg/apiclient/workflowarchive/workflow-archive.swagger.json

@@ -1537,7 +1537,7 @@
         },
         "ttlStrategy": {
           "$ref": "#/definitions/github.com.argoproj.argo.pkg.apis.workflow.v1alpha1.TTLStrategy",
-          "title": "TTLStrategy limits the lifetime of a Workflow that has finished execution depending on if it\nSucceeded or Failed. If this struct is set, once the Workflow finishes, it will be\ndeleted after the time to live expires. If this field is unset,\nthe controller config map will hold the default values\nUpdate"
+          "description": "TTLStrategy limits the lifetime of a Workflow that has finished execution depending on if it\nSucceeded or Failed. If this struct is set, once the Workflow finishes, it will be\ndeleted after the time to live expires. If this field is unset,\nthe controller config map will hold the default values."
         },
         "activeDeadlineSeconds": {
           "type": "string",

pkg/apiclient/workflowtemplate/workflow-template.swagger.json

@@ -1567,7 +1567,7 @@
         },
         "ttlStrategy": {
           "$ref": "#/definitions/github.com.argoproj.argo.pkg.apis.workflow.v1alpha1.TTLStrategy",
-          "title": "TTLStrategy limits the lifetime of a Workflow that has finished execution depending on if it\nSucceeded or Failed. If this struct is set, once the Workflow finishes, it will be\ndeleted after the time to live expires. If this field is unset,\nthe controller config map will hold the default values\nUpdate"
+          "description": "TTLStrategy limits the lifetime of a Workflow that has finished execution depending on if it\nSucceeded or Failed. If this struct is set, once the Workflow finishes, it will be\ndeleted after the time to live expires. If this field is unset,\nthe controller config map will hold the default values."
         },
         "activeDeadlineSeconds": {
           "type": "string",

pkg/apis/workflow/v1alpha1/workflow_types.go

@@ -239,8 +239,7 @@ type WorkflowSpec struct {
 	// TTLStrategy limits the lifetime of a Workflow that has finished execution depending on if it
 	// Succeeded or Failed. If this struct is set, once the Workflow finishes, it will be
 	// deleted after the time to live expires. If this field is unset,
-	// the controller config map will hold the default values
-	// Update
+	// the controller config map will hold the default values.
 	TTLStrategy *TTLStrategy `json:"ttlStrategy,omitempty" protobuf:"bytes,30,opt,name=ttlStrategy"`
 
 	// Optional duration in seconds relative to the workflow start time which the workflow is

workflow/controller/controller.go

@@ -366,13 +366,15 @@ func (wfc *WorkflowController) processNextItem() bool {
 		wfc.throttler.Remove(key)
 		return true
 	}
-	err = wfc.addingWorkflowDefaultValueIfValueNotExist(wf)
+
+	err = wfc.setWorkflowDefaults(wf)
 	if err != nil {
-		log.Warnf("Failed to unmarshal key '%s' to workflow object: %v", key, err)
+		log.Warnf("Failed to apply default workflow values to '%s': %v", wf.Name, err)
 		woc := newWorkflowOperationCtx(wf, wfc)
 		woc.markWorkflowFailed(fmt.Sprintf("invalid spec: %s", err.Error()))
 		woc.persistUpdates()
 		wfc.throttler.Remove(key)
+		return true
 	}
 
 	if wf.ObjectMeta.Labels[common.LabelKeyCompleted] == "true" {
@@ -381,7 +383,9 @@ func (wfc *WorkflowController) processNextItem() bool {
 		// but we are still draining the controller's workflow workqueue
 		return true
 	}
+
 	woc := newWorkflowOperationCtx(wf, wfc)
+
 	// Loading running workflow from persistence storage if nodeStatusOffload enabled
 	if wf.Status.IsOffloadNodeStatus() {
 		nodes, err := wfc.offloadNodeStatusRepo.Get(string(wf.UID), wf.GetOffloadNodeStatusVersion())
@@ -434,32 +438,6 @@ func (wfc *WorkflowController) processNextItem() bool {
 	return true
 }
 
-// addingWorkflowDefaultValueIfValueNotExist sets values in the workflow.Spec with defaults from the
-// workflowController. Values in the workflow will be given the upper hand over the defaults.
-// The defaults for the workflow controller is set in the WorkflowController.Config.DefautWorkflowSpec
-func (wfc *WorkflowController) addingWorkflowDefaultValueIfValueNotExist(wf *wfv1.Workflow) error {
-	if wfc.Config.DefautWorkflowSpec != nil {
-		defaultsSpec, err := json.Marshal(*wfc.Config.DefautWorkflowSpec)
-		if err != nil {
-			return err
-		}
-		workflowSpec, err := json.Marshal(wf.Spec)
-		if err != nil {
-			return err
-		}
-		// https: //godoc.org/k8s.io/apimachinery/pkg/util/strategicpatch#StrategicMergePatch
-		new, err := strategicpatch.StrategicMergePatch(defaultsSpec, workflowSpec, wfv1.WorkflowSpec{})
-		if err != nil {
-			return err
-		}
-		err = json.Unmarshal(new, &wf.Spec)
-		if err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
 func (wfc *WorkflowController) podWorker() {
 	for wfc.processNextPodItem() {
 	}
@@ -642,6 +620,31 @@ func (wfc *WorkflowController) newPodInformer() cache.SharedIndexInformer {
 	return informer
 }
 
+// setWorkflowDefaults sets values in the workflow.Spec with defaults from the
+// workflowController. Values in the workflow will be given the upper hand over the defaults.
+// The defaults for the workflow controller are set in the workflow-controller config map
+func (wfc *WorkflowController) setWorkflowDefaults(wf *wfv1.Workflow) error {
+	if wfc.Config.WorkflowDefaults != nil {
+		defaultsSpec, err := json.Marshal(*wfc.Config.WorkflowDefaults)
+		if err != nil {
+			return err
+		}
+		workflowBytes, err := json.Marshal(wf)
+		if err != nil {
+			return err
+		}
+		mergedWf, err := strategicpatch.StrategicMergePatch(defaultsSpec, workflowBytes, wfv1.Workflow{})
+		if err != nil {
+			return err
+		}
+		err = json.Unmarshal(mergedWf, &wf)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 func (wfc *WorkflowController) newWorkflowTemplateInformer() wfextvv1alpha1.WorkflowTemplateInformer {
 	return wfextv.NewSharedInformerFactoryWithOptions(wfc.wfclientset, workflowTemplateResyncPeriod, wfextv.WithNamespace(wfc.GetManagedNamespace())).Argoproj().V1alpha1().WorkflowTemplates()
 }