Fixes for topology aware scheduling validation webhook#324
Merged
Conversation
renormalize
previously approved these changes
Jan 15, 2026
renormalize
left a comment
Contributor
There was a problem hiding this comment.
thanks for addressing #317 (comment).
783f271 to
23274ff
Compare
23274ff to
8584f42
Compare
gflarity
reviewed
Jan 15, 2026
gflarity
reviewed
Jan 15, 2026
gflarity
reviewed
Jan 15, 2026
Contributor
|
Didn't request changes to keep things unblocked, but looks like there's a couple bugs in the validation code. |
* Expose `ErrorMatcher` in `testutils`. * Adapted PCS validation unit tests to use the new error matcher. * Added count=1 for test-e2e make target to ensure that no tests are cached Signed-off-by: Madhav Bhargava <madhav.bhargava@sap.com>
7bfe9ac to
b8d9bfd
Compare
sanjaychatterjee
approved these changes
Jan 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
What this PR does / why we need it:
When TAS is disabled then it validates the PCS to ensure that it does not have any TopologyConstraint set. This should only be done when a new PCS is created. Consider an existing PCS with TopologyConstraint set at one or more levels and it has been deployed when TAS was enabled. Now if the admin disabled TAS and restarts Grove, and if the PCS has some changes to its spec then validating webhook disallows it because it runs the validations to ensure that TopologyConstraints should not be defined for a PCS when TAS is disabled. While this is fine for newly create PCS it should not be skipped for PCS that is already running.
This PR fixes this behavior.
Additionally we also expose
ErrorMatcherthat was introduced in #317 and use it inwebhook/admission/pcs/validation/podcliqueset_test.gosince the return type was changed forpcsValidator.validateto returnfield.ErrorList.Which issue(s) this PR fixes:
Fixes #325
Special notes for your reviewer:
Does this PR introduce a API change?
Additional documentation e.g., enhancement proposals, usage docs, etc.: