feat(sdk): Enable setting OwnerReference on ResourceOps. Fixes #1779 #4831
Conversation
|
/assign @Ark-kun |
|
cc @PatrickXYS Also, @Ark-kun friendly ping |
|
Thank @elikatsis it lgtm to me /cc @Bobgy To see if we can move forward |
|
/assign @chensun |
|
This seems like a breaking change for volumeOp though, are you sure users always expect their volumes GCed after workflow finishes? |
|
@Bobgy after giving it some thought, you are right. Let's not change the current behavior, and since this is something easily configurable we can just instruct users to use it. Thanks! I'm changing this, rebasing on top of current master, and pushing. |
elikatsis
force-pushed
the
feature-sdk-resource-ownerref
branch
from
7119f59
to
b3382cc
Compare
elikatsis
force-pushed
the
feature-sdk-resource-ownerref
branch
from
b3382cc
to
f1f73d0
Compare
|
@Bobgy I rebased on top of master and resolved conflicts |
|
/assign @chensun @numerology |
|
Thanks! Do you mind adding a testcase or modifying one of the existing cases to cover this? |
elikatsis
force-pushed
the
feature-sdk-resource-ownerref
branch
from
f1f73d0
to
2e096aa
Compare
You are right. With the first iteration the Thanks! |
|
@numerology friendly ping |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elikatsis, numerology The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elikatsis, numerology The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
It seems like the test is failing because of the new pip dependency resolver. This means that some we have a version range for a dependency X which is incompatible with the range of versions another dependency Y requires for X. This is triggered by |
|
/retest |
chensun
force-pushed
the
master
branch
2 times, most recently
from
7542f0e
to
44d22a6
Compare
…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>
elikatsis
force-pushed
the
feature-sdk-resource-ownerref
branch
from
2e096aa
to
9ba27af
Compare
|
@Bobgy or @numerology could you Tests were failing for irrelevant reasons and then there were conflicts :/ I've rebased and everything seems ok |
|
/lgtm |
|
/retest |
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) and 'true' for VolumeOps. This allows the garbage collection of
PVCs upon workflow cleanup without any further change [4].
[1] https://github.com/argoproj/argo/blob/v2.4.0/pkg/apis/workflow/v1alpha1/workflow_types.go#L1044-L1045
[2] #4498
[3] #3537
[4] #1779
Signed-off-by: Ilias Katsakioris elikatsis@arrikto.com
Closes #1779
Checklist:
Do you want this pull request (PR) cherry-picked into the current release branch?
Learn more about cherry-picking updates into the release branch.