Skip to content

Conversation

@mrniranjan
Copy link
Contributor

This PR addresses issue where we are verifying
if the cpu manager state file is same after kubelet restart while we are verifying the above, we are not checking if Guranteed pod started before kubelet restart is also still running.

Refer: https://issues.redhat.com/browse/OCPBUGS-43280

This PR addresses issue where we are verifying
if the cpu manager state file is same after kubelet restart
while we are verifying the above, we are not checking
if Guranteed pod started before kubelet restart is also
still running.

Refer: https://issues.redhat.com/browse/OCPBUGS-43280

Signed-off-by: Niranjan M.R <[email protected]>
@openshift-ci openshift-ci bot requested review from ffromani and jmencak July 11, 2025 11:38

By("verify test pod comes back after kubelet restart")
Eventually(func() error {
updatedPod := &corev1.Pod{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can reduce code complexity and avoid using references, instead you can use like this:

var updatedPod corev1.Pod
Eventually(func() error{
    err := testclient.DataPlaneClient.Get(ctx, client.ObjectKeyFromObject(testpod), &updatedPod)
...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in latest commit

Comment on lines 388 to 393
for _, condition := range updatedPod.Status.Conditions {
if condition.Type == corev1.PodReady && condition.Status == corev1.ConditionTrue {
testlog.Infof("post kubelet restart pod is ready with UID: %v", updatedPod.UID)
return nil
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can make the function here more consistent with the flow, as in apply negative conditions if none is met then return nil (i.e succeeds); this can be done like this:

for _, condition := range updatedPod.Status.Conditions {
						if condition.Type == corev1.PodReady && condition.Status != corev1.ConditionTrue {
							return fmt.Errorf("pod condition is not ready after restart: conditions=%v", updatedPod.Status.Conditions)
						}
					}
return nil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in latest commit.

Copy link
Contributor

@swatisehgal swatisehgal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Mostly looks good, some minor comments.

// Check pod ready condition
for _, condition := range updatedPod.Status.Conditions {
if condition.Type == corev1.PodReady && condition.Status != corev1.ConditionTrue {
return fmt.Errorf("Pod ondition is not in Ready state after kubelet restart: condition: %v", updatedPod.Status.Conditions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT:

Suggested change
return fmt.Errorf("Pod ondition is not in Ready state after kubelet restart: condition: %v", updatedPod.Status.Conditions)
return fmt.Errorf("Pod condition is not in Ready state after kubelet restart: condition: %v", updatedPod.Status.Conditions)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in latest commit

}
// Check pod ready condition
for _, condition := range updatedPod.Status.Conditions {
if condition.Type == corev1.PodReady && condition.Status != corev1.ConditionTrue {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case pod condition is ready but fails the condition status check would be nice to point out condition Reason and condition Message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in latest commit

Copy link
Contributor

@swatisehgal swatisehgal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2025
@mrniranjan
Copy link
Contributor Author

/test okd-scos-e2e-aws-ovn

@mrniranjan
Copy link
Contributor Author

/test e2e-aws-ovn-techpreview

@mrniranjan
Copy link
Contributor Author

/test e2e-aws-ovn

@mrniranjan mrniranjan changed the title verify Guranteed pod is running after kubelet restart OCPBUGS-59354: E2E add test to verify guaranteed pod is running after kubelet restart Jul 16, 2025
@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Jul 16, 2025
@openshift-ci-robot
Copy link
Contributor

@mrniranjan: This pull request references Jira Issue OCPBUGS-59354, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.20.0) matches configured target version for branch (4.20.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @mrniranjan

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

This PR addresses issue where we are verifying
if the cpu manager state file is same after kubelet restart while we are verifying the above, we are not checking if Guranteed pod started before kubelet restart is also still running.

Refer: https://issues.redhat.com/browse/OCPBUGS-43280

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 16, 2025

@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: mrniranjan.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

@mrniranjan: This pull request references Jira Issue OCPBUGS-59354, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.20.0) matches configured target version for branch (4.20.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @mrniranjan

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This PR addresses issue where we are verifying
if the cpu manager state file is same after kubelet restart while we are verifying the above, we are not checking if Guranteed pod started before kubelet restart is also still running.

Refer: https://issues.redhat.com/browse/OCPBUGS-43280

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 openshift-eng/jira-lifecycle-plugin repository.

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.

@mrniranjan
Copy link
Contributor Author

/test e2e-aws-ovn

@mrniranjan
Copy link
Contributor Author

@yanirq can you approve this PR

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 21, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MarSik, mrniranjan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2025
@mrniranjan
Copy link
Contributor Author

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 22, 2025

@mrniranjan: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit eacfe9f into openshift:main Jul 22, 2025
19 checks passed
@openshift-ci-robot
Copy link
Contributor

@mrniranjan: Jira Issue OCPBUGS-59354: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-59354 has been moved to the MODIFIED state.

Details

In response to this:

This PR addresses issue where we are verifying
if the cpu manager state file is same after kubelet restart while we are verifying the above, we are not checking if Guranteed pod started before kubelet restart is also still running.

Refer: https://issues.redhat.com/browse/OCPBUGS-43280

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 openshift-eng/jira-lifecycle-plugin repository.

@mrniranjan
Copy link
Contributor Author

/cherry-pick release-4.19

@openshift-cherrypick-robot

@mrniranjan: new pull request created: #1365

Details

In response to this:

/cherry-pick release-4.19

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.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: cluster-node-tuning-operator
This PR has been included in build cluster-node-tuning-operator-container-v4.20.0-202507221345.p0.geacfe9f.assembly.stream.el9.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants