[Backend] Use pod namespace as default when not in multi user mode #3009
Conversation
|
/retest |
|
/retest |
1 similar comment
|
/retest |
| @@ -599,6 +607,9 @@ func (r *ResourceManager) ReportWorkflowResource(workflow *util.Workflow) error | |||
| } | |||
| runId := workflow.ObjectMeta.Labels[util.LabelKeyWorkflowRunId] | |||
| jobId := workflow.ScheduledWorkflowUUIDAsStringOrEmpty() | |||
| if len(workflow.Namespace) == 0 { | |||
| return util.NewInvalidInputError("Workflow missing namespace") | |||
There was a problem hiding this comment.
Why is this needed?
Are we sure this would work for "default" Kubernetes namespace?
There was a problem hiding this comment.
This is not needed. But I think it is clearer, because I think persistence agent is the only instance calling this. And it gets data using workflow client, so it will always carry namespace.
There was a problem hiding this comment.
I verified retry works. That includes coverage for persistence agent. Also, e2e tests pass.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: IronPan 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 |
2 similar comments
|
/retest |
|
/retest |
|
/retest |
|
strange, a lot of samples failed |
…ubeflow#3009) * Use pod namespace as default when not in multi user mode * Report error when POD_NAMESPACE not set * Change report workflow endpoint to always require namespace * Fix integration tests
Fixes #2908
The fix is the same as comment: #2908 (comment)
Problem of the bug is that, pod client need to default to pod namespace when it is not provided.
My fix: I think it's better to be explicit about which namespace we want to use, so I added getting namespace logic in handlers and removed default namespace logic in workflow client.
In addition to that, now I did a little more sanity checking depending on multi user mode or not:
Verification: I built an image and patched to kfp 0.2.2 standalone deployment, verified now pods can be retried successfully.
/assign @jingzhang36
/assign @IronPan
This change is