Include first failed job name in event emitted when JobSet fails, as well as the JobSet failure condition #477
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielvegamyhre 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 |
k8s-ci-robot
added
approved
✅ Deploy Preview for kubernetes-sigs-jobset canceled.
|
| @@ -916,3 +914,42 @@ func findReplicatedStatus(replicatedJobStatus []jobset.ReplicatedJobStatus, repl | |||
| } | |||
| return jobset.ReplicatedJobStatus{} | |||
| } | |||
|
|
|||
| // messageWithFirstFailedJob appends the first failed job to the original event message in human readable way. | |||
There was a problem hiding this comment.
Could we move these out of this file?
They don't require the controller so it may be useful to move them to a separate file.
There was a problem hiding this comment.
Yeah jobset_controller.go is overdue for refactoring. To start I think we can refactor some of the many helper functions into separate files based on the feature (similar to what you did with startup policy).
I did some refactoring in this PR (e.g. moving some functions into success_policy.go, adding a constants pkg, etc.)
However, for these particular functions, I'm not sure of the best place to put them yet. They are about finding the first failed job for a Jobset and generating an event message for it, which doesn't fit into any existing (or new) logical grouping.
I think for now we should leave these 3 functions here and maybe in a separate PR we can refactor some more, I don't want to go overboard splitting things up.
danielvegamyhre
force-pushed
the
jobevent
branch
from
0381ab5 to
4d1bc06
Compare
| "sigs.k8s.io/jobset/pkg/util/collections" | ||
| ) | ||
|
|
||
| // TODO: add unit tests for the functions in this file. |
There was a problem hiding this comment.
Do you want this done as part of this PR? or a TODO for others?
There was a problem hiding this comment.
I'll create a ticket to do it in follow up PR once this is merged so the file exists in main and the issue can link to it
|
/hold I have a minor nit around constants naming but otherwise this looks good to me. |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
|
/retest |
|
All tests are passing locally but the build fails in e2e CI? odd... |
Not sure why we did this but it bit us again... https://github.com/kubernetes-sigs/jobset/blob/main/Dockerfile Docker file is copying elements one by one in pkg. |
Ahhhh not again... this dockerfile will be the death of me. Fixing. Let's just change it to pkg/, the only unnecessary part of pkg/ is the pkg/testing and it's just one file. |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
@kannon92 I fixed the dockerfile but lgtm was removed because of this, can you take another look please? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Fixes #466