feat: IgnoreNotFound prevents a workflow error when the resource does not exist #1834

dmayle · 2019-12-11T19:34:09Z

This implements the solution proposed in:
#1823

Adds an 'ignoreNotFound' value to the resource object in the workflow manifest so that kubectl won't throw an error when the resource does not exist.

While this was implemented in order to allow checking for the existence of a resource, it is also useful in other cases, such as an idempotent delete step.

dmayle · 2019-12-12T08:48:00Z

I've checked, and the test is broken in trunk, and the error does not come from this code. Also, I have already signed the CLA

pkg/apis/workflow/v1alpha1/workflow_types.go

simster7 · 2019-12-13T18:00:56Z

workflow/executor/resource.go

@@ -265,6 +272,12 @@ func (we *WorkflowExecutor) SaveResourceParameters(resourceNamespace string, res
 		if param.ValueFrom == nil {
 			continue
 		}
+		if we.Template.Resource.IgnoreNotFound && resourceName == "" {
+			// Resource wasn't found, so we populate result with sentinel value
+			output := "<NOT_FOUND>"


Is there a reason you provided a sentinel value? Using kubectl get ... -ignore-not-found=true directly doesn't provide one and I don't think any user would expect a value to be filled in if a resource doesn't exist.

Considering your use case from your proposal:

... - name: check-for-existence resource: action: get ignoreNotFound: true manifest: | apiVersion: v1 kind: Pod metadata: name: does-not-exist namespace: argo-workflows outputs: parameters: - name: not-found valueFrom: jsonPath: '{.metadata.name}'

A user would have to know what the sentinel value is before checking it against the result of '{.metadata.name}' to determine if a resource actually exists or not, or would have to check if the result of '{.metadata.name}' and name match. It would be more logical to simply check if '{.metadata.name}' is empty or not.

The reason that there is a sentinel value is because we have three separate possibilities from any jsonpath expression: 1) non-empty value like "foo", 2) empty value like "{.metadata.annotations.imageregistry}" == "", and 3) non-existent entity.

If we don't provide a sentinel value, it's still possible to detect the difference between cases (2) and (3), but it requires the developer to either use two steps (one with a known existing jsonpath like .metadata.name followed by the desired jsonpath) or by including a marker in the jsonpath expression (e.g. '<{.metadata.annotations.imageregistry}>' which can than be compared with "<>" for empty vs "" for non-existent). Both of these require considerable extra costs to development, either extra runtime costs or string parsing with every result.

Using a sentinel value provides a nice balance that makes usage simple and efficient for most use cases. In the case where the user happens to be using the exact sentinel value chosen here, they then have to choose a solution like adding markers to the jsonpath.

Adding a custom sentinel value with an additional flag would also solve that problem, but that would be adding more code and configuration for a use case that is unlikely to occur in actual usage.

I see your point, thanks for explaining.

@alexec What do you think about this? I am leaning towards not including it. Even though it will force developers to have to do an extra check, I think it is more intuitive for users.

My first thought wast that it should probably be a sentinel error. They are more clearly defined. However, as I understand it, you want to:

Ignore "not found" errors.

Allow subsequent steps to understand that the resource was not found and act on it.

Am I correct? If that's the case, could we say that .metadata.creationTImestamp is null?

@alexec The main point of this feature is to disassociate the kubectl not-found error from the workflow error domain.
Right now, the "resource" template subsumes all kubectl errors, which would normally be a useful behavior, but in the case of kubectl, they made a decision to return an error code when looking for a resource that doesn't exist (or for deleting one).

While this can be useful in the guise of a shell script, where you can add per-command error handling, in argo-workflows there is no per-step handling of errors, so gracefully handling this case is problematic.

We could switch the behavior over wholesale, but that might be surprising to existing users. This PR just adds the option to the user to set the --ignore-not-found flag on get or delete operations. With this set, deleting a resource becomes idempotent in the case of a race condition (delete if found, ignore if someone else deleted it first), and getting a resource can now also be used to check for the existence of a resource (something that I need enough that I'm already maintaining a local fork to do so).

The added behavior that we're discussing here is specifically about how to return this additional piece of information to the developer in a way that is useful to consume. We only return a single string, so we have to choose how to encode the new information.

As you've indicated, we could force all developers who are trying to retrieve some field value to use two steps in their workflow (One to check a value like.metadata.creationTimestamp and one to check the desired value). This performs an additional API RPC call, adds argo executor overhead, and requires additional workflow complexity.

We could also suggest that the user add their own markers, but that requires them to split out the marker on every step to get a useful value.

The code here returns a fixed sentinel value that is unlikely to appear in the wild. I've chosen the characters to be outside of the set used in kubernetes identifiers, and unlikely to occur in a natural language sentence. This way, the check is as simple as a string comparison for the sentinel.

pkg/apis/workflow/v1alpha1/workflow_types.go

alexec · 2019-12-19T00:15:46Z

@simster7 I've assigned you an owner as you've already looked into this.

workflow/executor/resource.go

simster7 · 2020-01-02T20:03:02Z

@dmayle Any plans on finishing this up? We would like to get this merged

dmayle · 2020-01-03T12:42:17Z

Yes, absolutely, but I'm out of the office until next week. BTW - would you rather I stack changes when updating, or rebase and flatten?

…

On Thu, Jan 2, 2020, 20:03 Simon Behar ***@***.***> wrote: @dmayle <https://github.com/dmayle> Any plans on finishing this up? We would like to get this merged — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1834>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAQLWPLERQXFYBVNJR3C4TQ3ZB7PANCNFSM4JZUCP4A> .

simster7 · 2020-01-03T16:31:22Z

No preference! Commits get squashed when merging to master anyway

pkg/apis/workflow/v1alpha1/generated.pb.go

workflow/executor/resource.go

codecov · 2020-01-07T08:25:56Z

Codecov Report

Merging #1834 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1834   +/-   ##
=======================================
  Coverage   10.63%   10.63%           
=======================================
  Files          35       35           
  Lines       24992    24992           
=======================================
  Hits         2657     2657           
  Misses      21996    21996           
  Partials      339      339

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87c497f...d03aa8d. Read the comment docs.

dmayle · 2020-01-07T11:24:05Z

The argo CI check failed because of a transient network error connecting to Github. Is there anyway to retrigger just that check?

simster7 · 2020-01-07T14:04:38Z

Don't worry about the Argo CI check, we treat that one as flakey

dmayle · 2020-01-07T15:34:33Z

I've had to rebase again onto master because of the generated workflow file.

…xist

…ds which support it

dmayle · 2020-01-09T23:04:45Z

Any thoughts?

simster7 · 2020-01-13T23:45:51Z

(@dmayle Just FYI, we're a bit busy with the upcoming API Server release and will get to this a couple of days after the release)

dmayle · 2020-01-15T12:13:15Z

@simster7 thanks for the heads up!

alexec · 2020-01-31T17:58:14Z

@simster7 are you happy to be the assigned reviewer of this PR?
@dmayle you'll probably need to do a git merge master on the PR before review.

alexec

Please can you do a git merge master?

CLAassistant · 2020-03-17T12:53:35Z

All committers have signed the CLA.

simster7 · 2020-05-12T14:57:48Z

@dmayle Seems like #1779 solves the problem in this issue. Can you confirm? Closing for now

dmayle · 2020-05-14T14:52:08Z

@simster7 #1779 allows me to pass the --ignore-not-found flag, but ExecResource() expects JSON output (doesn't allow empty output). I'll try to get around to a new patch based on this new version

simster7 · 2020-05-14T14:54:24Z

Thanks @dmayle. Sorry for the absolute crazy delay on this

simster7 reviewed Dec 12, 2019

View reviewed changes

pkg/apis/workflow/v1alpha1/workflow_types.go Show resolved Hide resolved

simster7 reviewed Dec 13, 2019

View reviewed changes

This was referenced Dec 16, 2019

feat: Allow to pass optional flags to resource template #1779

Merged

Allow resources to pass arbitrary flags to kubectl #1857

Open

alexec assigned simster7 Dec 19, 2019

alexec added this to the v2.5 milestone Dec 19, 2019

simster7 reviewed Dec 19, 2019

View reviewed changes

workflow/executor/resource.go Outdated Show resolved Hide resolved

dmayle force-pushed the ignore_not_found branch from accd328 to 6363107 Compare January 6, 2020 15:13

simster7 reviewed Jan 6, 2020

View reviewed changes

pkg/apis/workflow/v1alpha1/generated.pb.go Outdated Show resolved Hide resolved

simster7 changed the title ~~IgnoreNotFound prevents a workflow error when the resource does not exist~~ feat: IgnoreNotFound prevents a workflow error when the resource does not exist Jan 6, 2020

simster7 reviewed Jan 6, 2020

View reviewed changes

workflow/executor/resource.go Outdated Show resolved Hide resolved

dmayle force-pushed the ignore_not_found branch from c0f5ce9 to 4ad58f0 Compare January 7, 2020 13:08

dmayle force-pushed the ignore_not_found branch from 4ad58f0 to b36a1da Compare January 7, 2020 15:33

dmayle added 5 commits January 8, 2020 08:27

IgnoreNotFound prevents a workflow error when the resource does not e…

ad45c39

…xist

Limit the ignoreNotFound parameter to get and delete, the only comman…

87de693

…ds which support it

Ensure all branches limit parameter to working commands

89fb3c8

feat(ignoreNotFound): Fix generated files

7b1a92a

Fix reference

87c497f

dmayle force-pushed the ignore_not_found branch from 2c3d7f2 to 87c497f Compare January 8, 2020 07:31

simster7 assigned alexec Jan 9, 2020

alexec modified the milestones: v2.5, v2.6 Jan 27, 2020

Merge branch 'master' into HEAD

d03aa8d

alexec removed this from the v2.6 milestone Feb 6, 2020

alexec requested changes Feb 7, 2020

View reviewed changes

simster7 added this to the v2.6 milestone Feb 19, 2020

alexec removed this from the v2.6 milestone Feb 19, 2020

simster7 closed this May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: IgnoreNotFound prevents a workflow error when the resource does not exist #1834

feat: IgnoreNotFound prevents a workflow error when the resource does not exist #1834

dmayle commented Dec 11, 2019

dmayle commented Dec 12, 2019

simster7 Dec 13, 2019 •

edited

Loading

dmayle Jan 6, 2020

simster7 Jan 6, 2020

alexec Jan 7, 2020

dmayle Jan 7, 2020

alexec commented Dec 19, 2019

simster7 commented Jan 2, 2020

dmayle commented Jan 3, 2020 via email

simster7 commented Jan 3, 2020

codecov bot commented Jan 7, 2020 •

edited

Loading

dmayle commented Jan 7, 2020

simster7 commented Jan 7, 2020

dmayle commented Jan 7, 2020

dmayle commented Jan 9, 2020

simster7 commented Jan 13, 2020

dmayle commented Jan 15, 2020

alexec commented Jan 31, 2020

alexec left a comment •

edited

Loading

CLAassistant commented Mar 17, 2020

simster7 commented May 12, 2020

dmayle commented May 14, 2020

simster7 commented May 14, 2020

feat: IgnoreNotFound prevents a workflow error when the resource does not exist #1834

feat: IgnoreNotFound prevents a workflow error when the resource does not exist #1834

Conversation

dmayle commented Dec 11, 2019

dmayle commented Dec 12, 2019

simster7 Dec 13, 2019 • edited Loading

Choose a reason for hiding this comment

dmayle Jan 6, 2020

Choose a reason for hiding this comment

simster7 Jan 6, 2020

Choose a reason for hiding this comment

alexec Jan 7, 2020

Choose a reason for hiding this comment

dmayle Jan 7, 2020

Choose a reason for hiding this comment

alexec commented Dec 19, 2019

simster7 commented Jan 2, 2020

dmayle commented Jan 3, 2020 via email

simster7 commented Jan 3, 2020

codecov bot commented Jan 7, 2020 • edited Loading

Codecov Report

dmayle commented Jan 7, 2020

simster7 commented Jan 7, 2020

dmayle commented Jan 7, 2020

dmayle commented Jan 9, 2020

simster7 commented Jan 13, 2020

dmayle commented Jan 15, 2020

alexec commented Jan 31, 2020

alexec left a comment • edited Loading

Choose a reason for hiding this comment

CLAassistant commented Mar 17, 2020

simster7 commented May 12, 2020

dmayle commented May 14, 2020

simster7 commented May 14, 2020

simster7 Dec 13, 2019 •

edited

Loading

codecov bot commented Jan 7, 2020 •

edited

Loading

alexec left a comment •

edited

Loading