Skip to content

Commit

Permalink
feat(sdk): Enable setting OwnerReference on ResourceOps. Fixes kubefl…
Browse files Browse the repository at this point in the history
…ow#1779

Argo supports a field in the ResourceTemplate that makes the controller
add an owner reference of the workflow to the created resource since
v2.4.0 [1].
With the upgrade of Argo client [2] and deployment [3] we are now able
to exploit it.

We set it to 'false' by default on all ResourceOps (actually, leave it
empty).

Setting the field to 'true' for VolumeOps allows the garbage collection
of PVCs upon workflow cleanup [4].

[1] https://github.com/argoproj/argo/blob/v2.4.0/pkg/apis/workflow/v1alpha1/workflow_types.go#L1044-L1045
[2] kubeflow#4498
[3] kubeflow#3537
[4] kubeflow#1779

Signed-off-by: Ilias Katsakioris <elikatsis@arrikto.com>
  • Loading branch information
elikatsis committed Jan 14, 2021
1 parent c2a8bd0 commit 2e096aa
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 3 deletions.
9 changes: 8 additions & 1 deletion sdk/python/kfp/dsl/_resource_op.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class Resource(object):
"merge_strategy": "str",
"success_condition": "str",
"failure_condition": "str",
"set_owner_reference": "bool",
"manifest": "str",
"flags": "list[str]"
}
Expand All @@ -40,6 +41,7 @@ class Resource(object):
"merge_strategy": "str",
"success_condition": "str",
"failure_condition": "str",
"set_owner_reference": "bool",
"manifest": "str",
"flags": "list[str]"
}
Expand All @@ -48,6 +50,7 @@ class Resource(object):
"merge_strategy": "mergeStrategy",
"success_condition": "successCondition",
"failure_condition": "failureCondition",
"set_owner_reference": "setOwnerReference",
"manifest": "manifest",
"flags": "flags"
}
Expand All @@ -57,13 +60,15 @@ def __init__(self,
merge_strategy: str = None,
success_condition: str = None,
failure_condition: str = None,
set_owner_reference: bool = None,
manifest: str = None,
flags: Optional[List[str]] = None):
"""Create a new instance of Resource"""
self.action = action
self.merge_strategy = merge_strategy
self.success_condition = success_condition
self.failure_condition = failure_condition
self.set_owner_reference = set_owner_reference
self.manifest = manifest
self.flags = flags

Expand Down Expand Up @@ -100,6 +105,7 @@ def __init__(self,
merge_strategy: str = None,
success_condition: str = None,
failure_condition: str = None,
set_owner_reference: bool = None,
attribute_outputs: Optional[Dict[str, str]] = None,
flags: Optional[List[str]] = None,
**kwargs):
Expand All @@ -118,7 +124,7 @@ def __init__(self,

if merge_strategy and action != "apply":
raise ValueError("You can't set merge_strategy when action != 'apply'")

# if action is delete, there should not be any outputs, success_condition, and failure_condition
if action == "delete" and (success_condition or failure_condition or attribute_outputs):
raise ValueError("You can't set success_condition, failure_condition, or attribute_outputs when action == 'delete'")
Expand All @@ -130,6 +136,7 @@ def __init__(self,
"merge_strategy": merge_strategy,
"success_condition": success_condition,
"failure_condition": failure_condition,
"set_owner_reference": set_owner_reference,
"flags": flags
}
# `resource` prop in `io.argoproj.workflow.v1alpha1.Template`
Expand Down
6 changes: 4 additions & 2 deletions sdk/python/tests/compiler/compiler_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ def my_pipeline(msg1, json, kind, msg2='value2'):
name="resource"
)
),
attribute_outputs={"out": json}
attribute_outputs={"out": json},
set_owner_reference=True
)
golden_output = {
'container': {
Expand Down Expand Up @@ -145,7 +146,8 @@ def my_pipeline(msg1, json, kind, msg2='value2'):
"kind: '{{inputs.parameters.kind}}'\n"
"metadata:\n"
" name: resource\n"
)
),
'setOwnerReference': True
}
}

Expand Down

0 comments on commit 2e096aa

Please sign in to comment.