feat: anyOf readiness condition requirements - #315
Conversation
kubernetes-prow
Bot
added
kind/feature
|
@vishnukothakapu: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for node-readiness-controller canceled.
|
kubernetes-prow
Bot
added
cncf-cla: yes
|
Hi @vishnukothakapu. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubernetes-prow
Bot
added
the
size/L
|
/title feat: anyOf readiness condition requirements |
|
/ok-to-test |
kubernetes-prow
Bot
added
ok-to-test
|
/retitle feat: anyOf readiness condition requirements |
kubernetes-prow
Bot
changed the title
4f246fb to
61ffd9d
Compare
|
@vishnukothakapu is this ready for review? @AnuragThePathak The code-changes are close to what you already worked on for |
|
/ok-to-test /cc @Karthik-K-N for API review. |
Sure, I'll review within next 24 hours. |
61ffd9d to
127dbbf
Compare
AnuragThePathak
left a comment
AnuragThePathak
left a comment
There was a problem hiding this comment.
Haven't reviewed tests yet but I think some of these changes are necessary.
81dd061 to
7473b90
Compare
There was a problem hiding this comment.
@vishnukothakapu I noticed that you deleted your previous commit and created a completely new one. From next time, prefer not removing the already pushed commits unless you know it won't have massive impact. Retaining what you already did helps review most of the time.
In any case, for my new comments, I won't suggest you to hurry. Instead I would encourage you to take some time and think (feel free to leverage AI either for ideas), then come up with something that allows you achieve the goal with minimal changes in what's already there following the existing conventions.
With that being said, also give others as well some time to reply, I am also curious to see if others also agree with my opinion of not introducing a completely new method for calculating is condition is satisfied or not.
P.S. if anything is not clear or you need more help, feel free to ask questions than applying some changes randomly.
|
@vishnukothakapu great job. You have perfectly nailed the direction, I was referring to. I have some minor feedbacks which I'll address today if possible or tomorrow at worst. In the meantime you can proceed updating the test cases probably as they are failing now and also docs if applicable. I don't anticipate any further functional changes. I am excluding, the point Karthik mentioned here by the way, I'll leave that up to you. |
AnuragThePathak
left a comment
AnuragThePathak
left a comment
There was a problem hiding this comment.
Didn't review the test cases themselves as not passing as of now. Apply the changes I have suggested, fix the failing test cases, and add docs. We should be good.
|
/assign |
2998d12 to
2461f2c
Compare
kubernetes-prow
Bot
removed
the
needs-rebase
a17dbbf to
692c1cd
Compare
| > | ||
| > `bootstrap-only` mode exists to verify that specific components have finished initializing. Using `anyOf` with `bootstrap-only` could lead to the node bootstrapping prematurely if just one component is ready, completely ignoring the initialization status of the others. The admission webhook enforces this restriction. | ||
|
|
||
|
|
There was a problem hiding this comment.
Just noting the misconfiguration risk, I'm not sure if this is worth adding in a warning or adding a webhook validation (may be in a separate PR if we feel the same way, @AnuragThePathak) --
in anyOf rules, if defaultStatus and requiredStatus are both set as true, this could short-circuit the rule, without anyone realizing it. eg: HardwareDriverReady is missing but default status set as True (matching requiredStatus=True) would never taint the node.
There was a problem hiding this comment.
@ajaysundark Thanks for pointing that out!, You're totally right. If a condition defaults to its requiredStatus in an anyOf rule, the whole thing would silently short-circuit and never taint the node.
I'd be happy to add a CEL validation for this, making sure defaultStatus doesn't equal requiredStatus when the policy is anyOf.
@AnuragThePathak Do you want me to add this validation to this PR, or would you prefer we track it as a new issue and handle it in a follow-up PR so we can get this one merged first? Happy to do either!
There was a problem hiding this comment.
@vishnukothakapu I would be in favor of updating the issue with this information-> adding webhook validation (CEL also there but not straight-forward at all + we have failurePolicy = fail, hence webhook is guaranteed to guard) -> updating the docs.
Better to keep things atomic imo
cc @ajaysundark
by the way regarding the lgtm label, apparently I do not have permission to use it so far. probably need to join the org, idk who they allow. Feel free to enlighten me if the process is not as complex as I am imagining.
There was a problem hiding this comment.
@AnuragThePathak Makes total sense! I agree that keeping this atomic is the best approach. I'll update the original issue with @ajaysundark 's edge case and we can tackle the Go webhook validation and docs update in a follow-up PR.
Assuming everything else looks good, is this PR good to go?
There was a problem hiding this comment.
Oh about this PR. Personally I was in favor of adding the validation and doc updates in this PR only so that if this feature goes live, people use it the right way since day 1. Were you two in favor of decoupling both changes?
There was a problem hiding this comment.
I have read your latest message on the issue. Looks good, wording can be improved (slightly lacks the precision) but rest ok. With that being said, also updating the issue adding a post script or something like that might be a good idea, just in case people decide to read just the top text.
692c1cd to
59b5fc1
Compare
AnuragThePathak
left a comment
AnuragThePathak
left a comment
There was a problem hiding this comment.
Seems like I ended up reviewing earlier by mistake. But fix the doc issue and one extra line mentioned (go through all comments once)
53c83d7 to
ad6ab84
Compare
…olicy immutability test
ad6ab84 to
fef71b5
Compare
AnuragThePathak
left a comment
AnuragThePathak
left a comment
There was a problem hiding this comment.
LGTM. I would suggest creating a new issue for defaultStatus + conditionPolicy follow-up when you see this message considering the existing issue will automatically be closed once we merge this PR.
@AnuragThePathak Thank you for the review and approval! I will open the new issue for the |
|
@vishnukothakapu ideally speaking there's no rule like having to wait for this PR to get merged to actually create the new issue. But anyways it's your preference, since the details are logged properly under current issue, I won't push further on creating the issue right away. For your information @ajaysundark maybe you can take a look whenever possible. I am still figuring out on accessing the prow commands by the way. @vishnukothakapu is in favor of creating a separate PR for the restriction on using defaultStatus and conditionPolicy at the same time. I think that will be convenient for us to review as well. Hopefully that's not an issue for release? |
| DefaultStatus: condReq.GetDefaultStatus(), | ||
| }) | ||
|
|
||
| log.V(1).Info("Condition evaluation", "node", node.Name, "rule", rule.Name, |
There was a problem hiding this comment.
@vishnukothakapu is it possible to capture the rule is satisfied with anyOf requirement here in this log (or a separate log?)
There was a problem hiding this comment.
@ajaysundark , The Evaluation result log that prints immediately after this loop actually captures exactly that (it explicitly logs both "conditionsSatisfied": true and "conditionPolicy": "anyOf").
However, if you think it would be helpful to also have conditionPolicy printed inside this per-condition log for easier debugging, I can definitely add that in the next follow-up PR! Let me know what you prefer.
ajaysundark
left a comment
ajaysundark
left a comment
There was a problem hiding this comment.
@vishnukothakapu Thanks for your contribution!
/lgtm
/approve
kubernetes-prow
Bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ajaysundark, AnuragThePathak, vishnukothakapu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
approved
|
@AnuragThePathak @ajaysundark Thank you both for your thorough reviews and guidance throughout this PR! I really appreciate the time you took to help shape this feature. I've opened the follow-up issue for the |
4 participants
Description
Introduces a conditionPolicy field (allOf | anyOf) to NodeReadinessRuleSpec that controls how the list of node conditions is aggregated when evaluating a rule.
Related Issue #313
Type of Change
/kind feature /kind api-change
Testing
Checklist
make testpassesmake lintpassesDoes this PR introduce a user-facing change?
Added
conditionPolicyfield toNodeReadinessRuleSpec(enum:allOf|anyOf).allOf(default) preserves existing behavior;anyOfremoves the taint when at least one condition is satisfied.anyOfcannot be combined withenforcementMode: bootstrap-only.