Return non-0 exit code from copilot upload failure #5444
Conversation
ddl-ebrown
force-pushed
the
return-copilot-error-from-file-upload-failure
branch
from
40a7724
to
49ddaa8
Compare
|
Opening this one up for discussion as I think the current behavior is confusing. Not sure if there should be richer exit codes returned / handled elsewhere -- but I don't think returning 0 right now is correct. If a file was supposed to be present and it wasn't OR if the file could not upload, those should be non-0 exit codes. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5444 +/- ##
=======================================
Coverage 60.99% 60.99%
=======================================
Files 793 793
Lines 51325 51325
=======================================
Hits 31305 31305
Misses 17136 17136
Partials 2884 2884
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
- The current copilot behavior is to:
* try to upload the specified file
* if an error occurs, try to write an error file instead
* if the error file write succeeds, return success -- otherwise error
This behavior is not ideal, because it suppresses real issues like
the inability to find the expected output file. While a case can be
made that the sidecar succeeded in handling its responsibility,
important information is lost in the process that is useful to other
containers in the job.
Generallyk, it's better to consider the inability to
find the original file a failure of the job to produce the expected
file in the expected location (useful especially for container tasks).
That means that a failure to upload the file should also return a
non-0 exit code.
- The new behavior is therefore:
* try to upload the specified file
* if an error occurs, try to write an error file instead
* if the error file write succeeds, return the original error
* if the error file write fails, return a joined version of the
errors
Signed-off-by: ddl-ebrown <[email protected]>
ddl-ebrown
force-pushed
the
return-copilot-error-from-file-upload-failure
branch
from
49ddaa8
to
69a83fe
Compare
| @@ -91,7 +91,7 @@ func TestUploadOptions_Upload(t *testing.T) { | |||
| ok, err := containerwatcher.FileExists(success) | |||
| assert.NoError(t, err) | |||
| assert.True(t, ok, "sucessfile not created") | |||
| assert.NoError(t, uopts.Sidecar(ctx)) | |||
| assert.Error(t, uopts.Sidecar(ctx)) | |||
There was a problem hiding this comment.
I think this actually should be an error
|
I like the joining of the errors but personally I think the original behavior of "if the error file write succeeds, return success -- otherwise error" more if only because this is the pattern that flytekit follows for the vast majority of its tasks. I think to change this we'd also have to understand how the plugin would handle the case. An error file implies a clean exit, implying that a user error occurred rather than a system error. The case where copilot is not able to find the expected output file is an okay scenario for making it a non-0 exit code, since that's kind of like a system level error, but in the normal case I think it should still be 0. If you really need it, maybe you can opt in via an environment variable or something... interpret all errors as exit code 1. However I'm not sure if propeller checks for an errors.pb file if that happens though, so if there's information there then it might get lost. This is kinda of an unspoken API, which I don't like. There's a contract between task and engine that's not formally spelled out. I think this is why i'm averse to changing it. |
CI is green, but not sure if all your concerns are covered in the tests. Could you help me better understand the informal contract? I'm not sure which plugin you're referring to here / the semantics involved. IMHO, exit codes are the right way to handle the differentiation between system error and user error. While I was at Puppet, we added a I think it would be pretty straightforward to add a switch and follow that sort of model if you think what's here breaks a contract somewhere? |
Tracking issue
Why are the changes needed?
The current copilot behavior is to:
This behavior is not ideal, because it suppresses real issues like
the inability to find the expected output file. While a case can be
made that the sidecar succeeded in handling its responsibility,
important information is lost in the process that is useful to other
containers in the job.
Generallyk, it's better to consider the inability to
find the original file a failure of the job to produce the expected
file in the expected location (useful especially for container tasks).
That means that a failure to upload the file should also return a
non-0 exit code.
The new behavior is therefore:
What changes were proposed in this pull request?
How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link