Remove non KMM related tolerations from the NMC status #1091
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @TomerNewman. 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 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. |
k8s-ci-robot
added
needs-ok-to-test
✅ Deploy Preview for kubernetes-sigs-kmm ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Can we please rename the commit/PR title to something like |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1091 +/- ##
==========================================
- Coverage 79.09% 74.95% -4.15%
==========================================
Files 51 76 +25
Lines 5109 6903 +1794
==========================================
+ Hits 4041 5174 +1133
- Misses 882 1515 +633
- Partials 186 214 +28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
TomerNewman
changed the title
TomerNewman
force-pushed
the
bugfix/nmc-status-holds-kmm-only-tolerations
branch
from
906aab4 to
2632f48
Compare
| if err = setWorkerConfigAnnotation(pod, nms.Config); err != nil { | ||
| return nil, fmt.Errorf("could not set worker config: %v", err) | ||
| } | ||
| if err = setWorkerTolerationsAnnotation(pod, nms.Tolerations); err != nil { |
There was a problem hiding this comment.
Something doesn't sums up for me. If we always set the worker's annotation based on the NMC spec, then why do we need the annotation at all? We can take it from the NMC spec and it will be the same.
Neither one of the option doesn't reflect the "real" pod's tolerations.
There was a problem hiding this comment.
If the tolerations in nmc spec changes for some reason while the worker pod runs from A to B, then later the status will be based on tolerations B (because we are doing status=spec), even though the worker pod ran with tolerations A.
| continue | ||
| } | ||
| tolerationsAnnotation := h.podManager.GetTolerationsAnnotation(&p) | ||
| if tolerationsAnnotation != "" { |
There was a problem hiding this comment.
do we need to check if tolerationsAnnotation != "" ? won't Unmarshal knows to handle it by default?
There was a problem hiding this comment.
@yevgeny-shnaidman you are right, when I applied Module without tolerations it made null on the pod's annotation.
This should have be the fix but it is not correct, so now I changed setWorkerTolerationsAnnotation.
Until now, the nmc status reflected all pod tolerations, including those unrelated to KMM. This commit updates the behavior to source tolerations from a dedicated annotation instead, ensuring only KMM-specific tolerations appear in the status.
TomerNewman
force-pushed
the
bugfix/nmc-status-holds-kmm-only-tolerations
branch
from
2632f48 to
c85d519
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TomerNewman, yevgeny-shnaidman 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 |
k8s-ci-robot
added
the
approved
|
/lgtm |
k8s-ci-robot
added
the
lgtm
5 participants
Until now, the nmc status reflected all the worker-pod tolerations, including those unrelated to KMM.
This commit updates the behavior to get tolerations from a dedicated annotation instead, ensuring only KMM-specific tolerations appear in the status.
/cc @ybettan @yevgeny-shnaidman