Fix JobSet webhook rejection on TrainJob suspend by using merge patch#28
Fix JobSet webhook rejection on TrainJob suspend by using merge patch#28abhijeet-dhumal wants to merge 2 commits intoopendatahub-io:mainfrom
Conversation
…patch Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@abhijeet-dhumal Can you please provide unit tests for your change? |
|
@abhijeet-dhumal does this need fixing upstream too? I'll let @astefanutti review the approach. I don't feel I understand this well enough. |
Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>
|
looks ok to me with very limited knowledge of context |
|
I haven't been able to reproduce with Trainer 2.1.0, neither on OpenShift nor KinD. |
|
Thanks a lot @astefanutti for testing this.. Since we couldn't reproduce this issue reliably, I'm closing this issue, Thanks! |
When attempting to suspend or resume a running TrainJob, the JobSet webhook rejects the update with:
TrainJob suspend/resume blocked by JobSet webhook validation
Root cause:
Solution:
When only suspend changes, use strategic merge patch instead of SSA:
This sends only the suspend field, bypassing immutable field validation.
Tested with:
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes #
Checklist: