Skip to content

Update Template Resolution Logic #1744

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.

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
merged 12 commits into from
Dec 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 6 additions & 2 deletions api/openapi-spec/swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -636,7 +636,7 @@
"$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Time"
},
"storedTemplateID": {
"description": "StoredTemplateID is the ID of stored template.",
"description": "StoredTemplateID is the ID of stored template. DEPRECATED: This value is not used anymore.",
"type": "string"
},
"templateName": {
Expand All @@ -647,12 +647,16 @@
"description": "TemplateRef is the reference to the template resource which this node corresponds to. Not applicable to virtual nodes (e.g. Retry, StepGroup)",
"$ref": "#/definitions/io.argoproj.workflow.v1alpha1.TemplateRef"
},
"templateScope": {
"description": "TemplateScope is the template scope in which the template of this node was retrieved.",
"type": "string"
},
"type": {
"description": "Type indicates type of node",
"type": "string"
},
"workflowTemplateName": {
"description": "WorkflowTemplateName is the WorkflowTemplate resource name on which the resolved template of this node is retrieved.",
"description": "WorkflowTemplateName is the WorkflowTemplate resource name on which the resolved template of this node is retrieved. DEPRECATED: This value is not used anymore.",
"type": "string"
}
}
Expand Down
2 changes: 1 addition & 1 deletion examples/workflow-template/hello-world.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ spec:
templateRef:
name: workflow-template-whalesay-template
template: whalesay-template
arguments:
inputs:
parameters:
- name: message
value: "hello world"
4 changes: 2 additions & 2 deletions examples/workflow-template/local-ref.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ spec:
templates:
- name: whalesay
template: whalesay-template
arguments:
inputs:
parameters:
- name: message
value: "hello world"
value: "hello from local ref"
- name: whalesay-template
inputs:
parameters:
Expand Down
8 changes: 4 additions & 4 deletions examples/workflow-template/nested.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ spec:
entrypoint: whalesay
templates:
- name: whalesay
inputs:
parameters:
- name: message
value: hello from nested
templateRef:
name: workflow-template-nested-template
template: whalesay-template
arguments:
parameters:
- name: message
value: "hello"
8 changes: 0 additions & 8 deletions examples/workflow-template/templates.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,11 @@ spec:
inputs:
parameters:
- name: message
arguments:
parameters:
- name: message
value: "{{inputs.parameters.message}} hello"
- name: whalesay-template
template: whalesay-inner-template
inputs:
parameters:
- name: message
arguments:
parameters:
- name: message
value: "{{inputs.parameters.message}} hello"
---
apiVersion: argoproj.io/v1alpha1
kind: WorkflowTemplate
Expand Down
42 changes: 42 additions & 0 deletions pkg/apis/workflow/v1alpha1/generated.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions pkg/apis/workflow/v1alpha1/generated.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 9 additions & 2 deletions pkg/apis/workflow/v1alpha1/openapi_generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions pkg/apis/workflow/v1alpha1/workflow_template_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,8 @@ func (wftmpl *WorkflowTemplate) GetTemplateByName(name string) *Template {
}
return nil
}

// GetTemplateScope returns the template scope of workflow template.
func (wftmpl *WorkflowTemplate) GetTemplateScope() string {
return wftmpl.Name
}
85 changes: 65 additions & 20 deletions pkg/apis/workflow/v1alpha1/workflow_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ type TemplateGetter interface {
GetName() string
GroupVersionKind() schema.GroupVersionKind
GetTemplateByName(name string) *Template
GetTemplateScope() string
}

// TemplateHolder is an interface for holders of templates.
Expand All @@ -78,6 +79,12 @@ type TemplateHolder interface {
IsResolvable() bool
}

// TemplateStorage is an interface of template storage getter and setter.
type TemplateStorage interface {
Copy link
Member

Choose a reason for hiding this comment

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

I'm sure I'm missing something obvious, but could you help me understand why this is implemented in both Workflow and wfOperationCtx? It seems like all the implementation for wfOperationCtx does is call the implementation of the Workflow:

// GetStoredTemplate retrieves a template from stored templates of the workflow.
func (woc *wfOperationCtx) GetStoredTemplate(templateScope string, holder wfv1.TemplateHolder) *wfv1.Template {
	return woc.wf.GetStoredTemplate(templateScope, holder)
}

Why do you not pass the Workflow as the TemplateStorage instead of the wfOperationCtx in operator.go:

	woc.tmplCtx = templateresolution.NewContext(wfc.wftmplInformer.Lister().WorkflowTemplates(wf.Namespace), wf, &woc)

Like you do in validate.go:

tmplCtx := templateresolution.NewContextFromClientset(wfClientset.ArgoprojV1alpha1().WorkflowTemplates(namespace), wf, wf)

Sorry if this is an obvious question.

Copy link
Member Author

@dtaniwaki dtaniwaki Nov 13, 2019

Choose a reason for hiding this comment

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

That's because I wanted to let the operator save the workflow.
https://github.com/argoproj/argo/blob/fae7382686d917d78e3909d1f6db79c272a1aa11/workflow/controller/operator.go#L381-L384

Need to flag woc.updated if new stored templates are saved.

GetStoredTemplate(templateScope string, holder TemplateHolder) *Template
SetStoredTemplate(templateScope string, holder TemplateHolder, tmpl *Template) (bool, error)
}

// Workflow is the definition of a workflow resource
// +genclient
// +genclient:noStatus
Expand All @@ -98,6 +105,7 @@ type WorkflowList struct {
}

var _ TemplateGetter = &Workflow{}
var _ TemplateStorage = &Workflow{}

// WorkflowSpec is the specification of a Workflow.
type WorkflowSpec struct {
Expand Down Expand Up @@ -762,11 +770,16 @@ type NodeStatus struct {
TemplateRef *TemplateRef `json:"templateRef,omitempty" protobuf:"bytes,6,opt,name=templateRef"`

// StoredTemplateID is the ID of stored template.
// DEPRECATED: This value is not used anymore.
StoredTemplateID string `json:"storedTemplateID,omitempty" protobuf:"bytes,18,opt,name=storedTemplateID"`

// WorkflowTemplateName is the WorkflowTemplate resource name on which the resolved template of this node is retrieved.
// DEPRECATED: This value is not used anymore.
WorkflowTemplateName string `json:"workflowTemplateName,omitempty" protobuf:"bytes,19,opt,name=workflowTemplateName"`

// TemplateScope is the template scope in which the template of this node was retrieved.
TemplateScope string `json:"templateScope,omitempty" protobuf:"bytes,20,opt,name=templateScope"`
Copy link
Member

@sarabala1979 sarabala1979 Nov 25, 2019

Choose a reason for hiding this comment

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

What is the use of this field? I can see failed node status has templateref which has the same information

    workflow-template-retry-with-steps-8ddk4-3945074098:
      boundaryID: workflow-template-retry-with-steps-8ddk4
      displayName: hello2a(0)
      finishedAt: "2019-11-25T21:38:27Z"
      id: workflow-template-retry-with-steps-8ddk4-3945074098
      message: failed with exit code 1
      name: workflow-template-retry-with-steps-8ddk4[1].hello2a(0)
      phase: Failed
      startedAt: "2019-11-25T21:38:23Z"
      templateRef:
        name: workflow-template-random-fail-template
        template: random-fail-template
      templateScope: workflow-template-random-fail-template
      type: Pod

Copy link
Member Author

Choose a reason for hiding this comment

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

TemplateScope is used to resolve workflow-template-local templates. Because we use 2 types of references: Template and TemplateRef, this field is necessary. If we can remove Template and all the references are made with TemplateRef, we don't need TemplateScope and simplify the code. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I still not fully understand the need for this.
We can just have resolution logic where .TemplateRef is checked only when .Template is empty.


// Phase a simple, high-level summary of where the node is in its lifecycle.
// Can be used as a state machine.
Phase NodePhase `json:"phase,omitempty" protobuf:"bytes,7,opt,name=phase,casttype=NodePhase"`
Expand Down Expand Up @@ -858,6 +871,20 @@ func (n NodeStatus) CanRetry() bool {
return n.Completed() && !n.Successful()
}

var _ TemplateHolder = &NodeStatus{}

func (n *NodeStatus) GetTemplateName() string {
return n.TemplateName
}

func (n *NodeStatus) GetTemplateRef() *TemplateRef {
return n.TemplateRef
}

func (n *NodeStatus) IsResolvable() bool {
return true
}

// S3Bucket contains the access information required for interfacing with an S3 bucket
type S3Bucket struct {
// Endpoint is the hostname of the bucket endpoint
Expand Down Expand Up @@ -1271,6 +1298,11 @@ func (wf *Workflow) GetTemplateByName(name string) *Template {
return nil
}

// GetTemplateScope returns the template scope of workflow.
func (wf *Workflow) GetTemplateScope() string {
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this function?

return ""
}

// NodeID creates a deterministic node ID based on a node name
func (wf *Workflow) NodeID(name string) string {
if name == wf.ObjectMeta.Name {
Expand All @@ -1281,34 +1313,47 @@ func (wf *Workflow) NodeID(name string) string {
return fmt.Sprintf("%s-%v", wf.ObjectMeta.Name, h.Sum32())
}

// GetStoredTemplate gets a resolved template from stored data.
func (wf *Workflow) GetStoredTemplate(node *NodeStatus) *Template {
id := node.StoredTemplateID
if id == "" {
// GetStoredTemplate retrieves a template from stored templates of the workflow.
func (wf *Workflow) GetStoredTemplate(templateScope string, holder TemplateHolder) *Template {
tmplID := wf.getStoredTemplateName(templateScope, holder)
if tmplID == "" {
return nil
}
tmpl, ok := wf.Status.StoredTemplates[id]
if ok {
return &tmpl
tmpl, ok := wf.Status.StoredTemplates[tmplID]
if !ok {
return nil
}
return nil
return &tmpl
}

// GetStoredOrLocalTemplate gets a resolved template from stored data or local template.
func (wf *Workflow) GetStoredOrLocalTemplate(node *NodeStatus) *Template {
// Try to find a template from stored data.
tmpl := wf.GetStoredTemplate(node)
if tmpl != nil {
return tmpl
// SetStoredTemplate stores a new template in stored templates of the workflow.
func (wf *Workflow) SetStoredTemplate(templateScope string, holder TemplateHolder, tmpl *Template) (bool, error) {
tmplID := wf.getStoredTemplateName(templateScope, holder)
if tmplID == "" {
return false, nil
}
// Try to get template from Workflow.
if node.WorkflowTemplateName == "" && node.TemplateName != "" {
tmpl := wf.GetTemplateByName(node.TemplateName)
if tmpl != nil {
return tmpl
_, ok := wf.Status.StoredTemplates[tmplID]
if !ok {
if wf.Status.StoredTemplates == nil {
wf.Status.StoredTemplates = map[string]Template{}
}
wf.Status.StoredTemplates[tmplID] = *tmpl
return true, nil
}
return false, nil
}

// getStoredTemplateName returns the stored template name of a given template holder on the template scope.
func (wf *Workflow) getStoredTemplateName(templateScope string, holder TemplateHolder) string {
tmplRef := holder.GetTemplateRef()
if tmplRef != nil {
return fmt.Sprintf("%s/%s", tmplRef.Name, tmplRef.Template)
} else if templateScope != "" {
Copy link
Member

Choose a reason for hiding this comment

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

same as above comments

return fmt.Sprintf("%s/%s", templateScope, holder.GetTemplateName())
} else {
// Do not store workflow-local templates.
return ""
}
return nil
}

// ContinueOn defines if a workflow should continue even if a task or step fails/errors.
Expand Down
34 changes: 33 additions & 1 deletion workflow/common/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,39 @@ func MergeReferredTemplate(tmpl *wfv1.Template, referred *wfv1.Template) (*wfv1.
newTmpl := referred.DeepCopy()

newTmpl.Name = tmpl.Name
newTmpl.Outputs = *tmpl.Outputs.DeepCopy()

if len(tmpl.Inputs.Parameters) > 0 {
parameters := make([]wfv1.Parameter, len(tmpl.Inputs.Parameters))
copy(parameters, tmpl.Inputs.Parameters)
newTmpl.Inputs.Parameters = parameters
}
if len(tmpl.Inputs.Artifacts) > 0 {
artifacts := make([]wfv1.Artifact, len(tmpl.Inputs.Artifacts))
copy(artifacts, tmpl.Inputs.Artifacts)
newTmpl.Inputs.Artifacts = artifacts
}

if len(tmpl.Outputs.Parameters) > 0 {
parameters := make([]wfv1.Parameter, len(tmpl.Outputs.Parameters))
copy(parameters, tmpl.Outputs.Parameters)
newTmpl.Outputs.Parameters = parameters
}
if len(tmpl.Outputs.Artifacts) > 0 {
artifacts := make([]wfv1.Artifact, len(tmpl.Outputs.Artifacts))
copy(artifacts, tmpl.Outputs.Artifacts)
newTmpl.Outputs.Artifacts = artifacts
}

if len(tmpl.Arguments.Parameters) > 0 {
parameters := make([]wfv1.Parameter, len(tmpl.Arguments.Parameters))
copy(parameters, tmpl.Arguments.Parameters)
newTmpl.Arguments.Parameters = parameters
}
if len(tmpl.Arguments.Artifacts) > 0 {
artifacts := make([]wfv1.Artifact, len(tmpl.Arguments.Artifacts))
copy(artifacts, tmpl.Arguments.Artifacts)
newTmpl.Arguments.Artifacts = artifacts
}

if len(tmpl.NodeSelector) > 0 {
m := make(map[string]string, len(tmpl.NodeSelector))
Expand Down
Loading