Support multiple events in the lifecycle policy #285
Conversation
volcano-sh-bot
added
the
do-not-merge/work-in-progress
volcano-sh-bot
added
the
size/L
We need cherry-pick CRD when this PR is merged. |
pkg/apis/batch/v1alpha1/job.go
Outdated
| @@ -178,7 +178,7 @@ type LifecyclePolicy struct { | |||
| // The Event recorded by scheduler; the controller takes actions | |||
| // according to this Event. | |||
| // +optional | |||
| Event Event `json:"event,omitempty" protobuf:"bytes,2,opt,name=event"` | |||
| Event []Event `json:"event,omitempty" protobuf:"bytes,2,opt,name=event"` | |||
There was a problem hiding this comment.
Add a new member; we already publish this CRD; we can not change it.
There was a problem hiding this comment.
Added a new member EventList
| break | ||
| } | ||
| if _, found := policyEvents[event]; found { | ||
| err = multierror.Append(err, fmt.Errorf("duplicate event %v", event)) |
There was a problem hiding this comment.
that's ok if duplicated in the same array.
There was a problem hiding this comment.
Code updated to ignore duplicates in the same array
There was a problem hiding this comment.
The duplicated events are already been removed in codes of:
policyEventsList := getEventlist(policy)
?
There was a problem hiding this comment.
If there are duplicate events within one policy it will be removed in the codes of:
policyEventList := getEventlist(policy)
But if there are duplicate events across different policy then
Error message will be thrown ("duplicate events %v",event)
There was a problem hiding this comment.
ok, then we may need more clear error message for that since the first case is being handled automatically now.
There was a problem hiding this comment.
can the error message be updated as ("duplicate events %v across different policy", event) ?
There was a problem hiding this comment.
That would be good thanks.
Rajadeepan
changed the title
Rajadeepan
changed the title
Rajadeepan
changed the title
volcano-sh-bot
removed
the
do-not-merge/work-in-progress
Travis tests have failedHey @Rajadeepan, TravisBuddy Request Identifier: c0c60700-9f0f-11e9-9e24-3df77584075c |
Travis tests have failedHey @Rajadeepan, TravisBuddy Request Identifier: add68ad0-a0ee-11e9-9257-c7ea9dc52f8c |
Travis tests have failedHey @Rajadeepan, TravisBuddy Request Identifier: 4d1bc270-a119-11e9-b4a0-0f4450928f2b |
pkg/apis/batch/v1alpha1/job.go
Outdated
| // The EventList recorded by scheduler; the controller takes actions | ||
| // according to this EventList. | ||
| // +optional | ||
| EventList []Event `json:"eventlist,omitempty" protobuf:"bytes,3,opt,name=eventlist"` |
There was a problem hiding this comment.
Updated to Events
Rajadeepan
force-pushed
the
mulevents
branch
2 times, most recently
from
b042b5b to
aac88fc
Compare
Travis tests have failedHey @Rajadeepan, TravisBuddy Request Identifier: 3c24db80-a143-11e9-b4a0-0f4450928f2b |
|
/lgtm |
|
/approve |
|
cc @k82cn |
|
/lgtm Please also open PR for e2e test. |
volcano-sh-bot
added
the
needs-rebase
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: k82cn, Rajadeepan, TommyLike 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 |
volcano-sh-bot
added
the
approved
volcano-sh-bot
removed
lgtm
ok will raise a new PR for e2e test |
|
/lgtm |
Support multiple events in the lifecycle policy. updating events to list of events.
#266
This PR need to be updated with updated submodule of charts after the PR volcano-retired/charts#5 gets merged.