[WIP] test(kubelet): add future-proof test for PodStatus equality #136543
Conversation
k8s-ci-robot
added
size/L
|
@herb-duan: 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. |
k8s-ci-robot
added
do-not-merge/needs-sig
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
|
Hi @herb-duan. Thanks for your PR. I'm waiting for a kubernetes 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
k8s-ci-robot
removed
the
do-not-merge/needs-sig
github-project-automation
bot
moved this to Triage
in SIG Node: code and documentation PRs
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: herb-duan The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This commit introduces a new test, TestIsPodStatusByKubeletEqualFutureProof, to make the verification of the pod status equality logic more robust and prevent future regressions. The previous test was brittle as it only checked a few specific fields. It would not have detected issues if new fields were added to the v1.PodStatus struct, potentially re-introducing bugs where the kubelet performs unnecessary status updates. Following reviewer feedback, the new test uses reflection to ensure every field in v1.PodStatus is explicitly categorized as either kubelet-owned or ignored. The test will now fail if it encounters an unknown field, forcing any developer who modifies v1.PodStatus to consciously update the equality logic and its corresponding tests. The test includes sub-tests to verify that: - Changes to ignored fields (e.g., ResourceClaimStatuses) are correctly ignored. - Changes to owned fields (e.g., Phase, PodIPs) are correctly detected. ResourceClaimStatuses/ExtendedResourceClaimStatus test cases are temporarily commented out (depend on PR kubernetes#136238 merge). A TODO has been added for NominatedNodeName, as it is identified as a field that should be ignored, but the current equality function does not handle it. This is left for further discussion. Signed-off-by: Herb Duan <herbertduan@qq.com>
herb-duan
force-pushed
the
test-kubelet-podstatus-future-proof
branch
from
3b7535c to
ac8f02c
Compare
herb-duan
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
Hi @pohly, This is the follow-up PR for the future-proof test we discussed in #136238. NOTE: This PR is WIP and depends on PR #136238 being merged first. Key notes for this PR:
Could you please review this PR when you have time? Also, could you help with Thanks! /sig node |
k8s-ci-robot
added
release-note-none
|
@herb-duan: The label 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. |
k8s-ci-robot
removed
the
needs-priority
| t.Fatalf("New fields found in v1.PodStatus: %v. Please add them to either "+ | ||
| "kubeletOwnedFields or kubeletIgnoredFields in TestIsPodStatusByKubeletEqualFutureProof "+ | ||
| "and update isPodStatusByKubeletEqual logic if necessary.", unknownFields.List()) | ||
| } |
There was a problem hiding this comment.
This is a good start for the test.
| } | ||
|
|
||
| // Verify that changes to fields that are not owned by kubelet are ignored. | ||
| t.Run("ignored fields are ignored", func(t *testing.T) { |
There was a problem hiding this comment.
The two sub-tests rely on manually writing test data. If someone adds a new field to the lists above and then forgets to extend the sub-tests, nothing fails and in a diff it's not obvious that something is missing, so it might also not get caught in review.
My idea was to generate different statuses with https://github.com/kubernetes-sigs/randfill, then determine based on the lists above whether a status should be considered equal or different.
Fuzzing is not guaranteed to hit all fields, although in practice it probably will very quickly. If we want to be more systematic, we can also manually set one field at a time. That's better because then we are sure that this one field is correctly handled by isPodStatusByKubeletEqual.
Fuzzing is not generating valid statuses, but this might be good enough for isPodStatusByKubeletEqual?
3 participants
What type of PR is this?
/kind cleanup
/kind test
What this PR does / why we need it:
This PR introduces a new test
TestIsPodStatusByKubeletEqualFutureProofto make the pod status equality logic more robust and prevent future regressions.Key improvements:
v1.PodStatusfields are explicitly categorized as kubelet-owned or ignored (fails fast if new fields are added).NominatedNodeName(marked as ignored but not handled by current logic) for further discussion.This addresses reviewer feedback from #136238 to avoid reintroduction of unnecessary kubelet reconciliation loops for new non-kubelet-owned PodStatus fields.
Which issue(s) this PR is related to:
Follow-up to #136238
Indirectly relates to #136239
Special notes for your reviewer:
NominatedNodeNameis marked as ignored but not handled byisPodStatusByKubeletEqual(TODO: discuss whether to update the equality logic).Does this PR introduce a user-facing change?
NONE