Port Node and Pod State to Structured Intervals #28299
Conversation
dgoodwin
changed the title
dgoodwin
force-pushed
the
node-pod-monitor-intervals
branch
from
d31d52b
to
f65da88
Compare
openshift-ci
bot
added
the
approved
|
@dgoodwin: The following tests failed, say
Full PR test history. Your PR dashboard. 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/test-infra repository. I understand the commands that are listed here. |
|
Job Failure Risk Analysis for sha: cba88ea
|
|
I think this is ready for review. The missing intervals for nodes reaching desired config is fixed, and the pod lifecycle events are now sorting together. For comparison, here is a job WITHOUT these changes, the node-state or pods sections should be identical: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/28299/pull-ci-openshift-origin-master-e2e-aws-ovn-upgrade/1712155920533819392/artifacts/e2e-aws-ovn-upgrade/openshift-e2e-test/artifacts/junit/e2e-timelines_kube-apiserver_20231011-180818.html |
|
|
||
| case isContainer && isContainerReadyTransition: | ||
| containerToReadinessTransitions[container.ToLocator()] = append(containerToReadinessTransitions[container.ToLocator()], event) | ||
| containerToReadinessTransitions[cls] = append(containerToReadinessTransitions[cls], event) | ||
|
|
||
| case isKubeletReadinessCheck: |
There was a problem hiding this comment.
do we need isContainer && here like the ones above that also use cls? or maybe isKubeletReadinessCheck imples that isContainer == true?
There was a problem hiding this comment.
Edit, good eye, looks like it will have a container in the locator, but checking for it in addition to the isKubeletreadinessCheck is likely redundant. I'd be hesitant to change the logic that was there, probably more likely to cause problems than improve anything.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DennisPeriquet, dgoodwin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Ports the code that generates the node and pod state intervals we show in spyglass to use, and generate new structured intervals. These components interpret other "source" intervals to determine what intervals they should generate.
This was incredibly complex and time consuming to get working between the complexity and the use of raw json files as test inputs and outputs.
I discovered that Intervals were not serialized via the same logic as EventIntervals (a dupe type we use just for serialization for some reason), and this is now addressed. As a result you'll see in some of the expected.json files, a few intervals are reordered to where they should be by the new logic. (primarily consideration of the To timestamp)
There is an unfortunate hack involved where Locators cannot be used as map keys because they contain a map themselves, go does not allow this. You'll notice a couple spots where I am maintaining a map of locatorStr -> locator, for lookup later when we need the real locators. This was done because I do not want us parsing locator strings, as this was one of the problems that led to this entire effort.
Apologies for the size but it was largely unavoidable, these parts of the code are linked, one goes and they all need to.