NO-JIRA:e2e:daemonset: pod phase and events #1289

Tal-or · 2025-01-22T13:49:04Z

In case the DaemonSet's pods are not running for some reason,
we want to log the pod's phase and event for
better observability.

This could help us with finding the root cause
for the reason the pod didn't start.

Signed-off-by: Talor Itzhak [email protected]

ffromani · 2025-01-22T13:51:43Z

test/e2e/performanceprofile/functests/utils/daemonset/daemonset.go

-	testlog.Infof("daemonset %q %q desired %d scheduled %d ready %d", namespace, name, ds.Status.DesiredNumberScheduled, ds.Status.CurrentNumberScheduled, ds.Status.NumberReady)
-	return ds.Status.DesiredNumberScheduled > 0 && ds.Status.DesiredNumberScheduled == ds.Status.NumberReady, nil
+	isRunning := ds.Status.DesiredNumberScheduled > 0 && ds.Status.DesiredNumberScheduled == ds.Status.NumberReady
+	if !isRunning {


style: everything else being equal, please align the happy path to the left: https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88

The happy path is isRunning == true which is the opposite of !isRunning

I moved the logic into a separate function to make it more readable. I hope it's ok now

test/e2e/performanceprofile/functests/utils/daemonset/daemonset.go

ffromani · 2025-01-22T14:24:46Z

thanks, we can go a step further (ofc not blocking)

func IsRunning(ctx context.Context, cli client.Client, namespace, name string) (bool, error) {
	ds, err := GetByName(ctx, cli, namespace, name)
	if err != nil {
		if k8serrors.IsNotFound(err) {
			testlog.Warningf("daemonset %q %q not found - retrying", namespace, name)
			return false, nil
		}
		return false, err
	}
	if isRunning := ds.Status.DesiredNumberScheduled > 0 && ds.Status.DesiredNumberScheduled == ds.Status.NumberReady; !isRunning {
		return false, logEventsAndPhase(ctx, cli, ds)
	}
	return true, nil
}

func logEventsAndPhase(ctx context.Context, cli client.Client, ds *appsv1.DaemonSet) error {
	podList := &corev1.PodList{}
	err := cli.List(ctx, podList, &client.ListOptions{
		Namespace:     ds.Namespace,
		LabelSelector: labels.SelectorFromSet(ds.Spec.Selector.MatchLabels),
	})
	if err != nil {
		return fmt.Errorf("failed to list pods for DaemonSet %s; %w", client.ObjectKeyFromObject(ds).String(), err)
	}
	for _, pod := range podList.Items {
		if pod.Status.Phase != corev1.PodRunning {
			podKey := client.ObjectKeyFromObject(&pod).String()
			testlog.Warningf("daemonset %s pod %s is not running, expected status %s got %s", ds.Name, podKey, corev1.PodRunning, pod.Status.Phase)
			podEvents, err := events.GetEventsForObject(cli, pod.Namespace, pod.Name, string(pod.UID))
			if err != nil {
				return fmt.Errorf("failed to list events for Pod %s; %w", podKey, err)
			}
			for _, event := range podEvents.Items {
				testlog.Warningf("-> %s %s %s", event.Action, event.Reason, event.Message)
			}
		}
	}
	return nil
}

Tal-or · 2025-01-22T14:47:38Z

thanks, we can go a step further (ofc not blocking)

func IsRunning(ctx context.Context, cli client.Client, namespace, name string) (bool, error) {
	ds, err := GetByName(ctx, cli, namespace, name)
	if err != nil {
		if k8serrors.IsNotFound(err) {
			testlog.Warningf("daemonset %q %q not found - retrying", namespace, name)
			return false, nil
		}
		return false, err
	}
	if isRunning := ds.Status.DesiredNumberScheduled > 0 && ds.Status.DesiredNumberScheduled == ds.Status.NumberReady; !isRunning {
		return false, logEventsAndPhase(ctx, cli, ds)
	}
	return true, nil
}

func logEventsAndPhase(ctx context.Context, cli client.Client, ds *appsv1.DaemonSet) error {
	podList := &corev1.PodList{}
	err := cli.List(ctx, podList, &client.ListOptions{
		Namespace:     ds.Namespace,
		LabelSelector: labels.SelectorFromSet(ds.Spec.Selector.MatchLabels),
	})
	if err != nil {
		return fmt.Errorf("failed to list pods for DaemonSet %s; %w", client.ObjectKeyFromObject(ds).String(), err)
	}
	for _, pod := range podList.Items {
		if pod.Status.Phase != corev1.PodRunning {
			podKey := client.ObjectKeyFromObject(&pod).String()
			testlog.Warningf("daemonset %s pod %s is not running, expected status %s got %s", ds.Name, podKey, corev1.PodRunning, pod.Status.Phase)
			podEvents, err := events.GetEventsForObject(cli, pod.Namespace, pod.Name, string(pod.UID))
			if err != nil {
				return fmt.Errorf("failed to list events for Pod %s; %w", podKey, err)
			}
			for _, event := range podEvents.Items {
				testlog.Warningf("-> %s %s %s", event.Action, event.Reason, event.Message)
			}
		}
	}
	return nil
}

Could you please highlight the diff from the existing implementation?

ffromani · 2025-01-22T14:53:00Z

thanks, we can go a step further (ofc not blocking)

func IsRunning(ctx context.Context, cli client.Client, namespace, name string) (bool, error) {
	ds, err := GetByName(ctx, cli, namespace, name)
	if err != nil {
		if k8serrors.IsNotFound(err) {
			testlog.Warningf("daemonset %q %q not found - retrying", namespace, name)
			return false, nil
		}
		return false, err
	}
	if isRunning := ds.Status.DesiredNumberScheduled > 0 && ds.Status.DesiredNumberScheduled == ds.Status.NumberReady; !isRunning {
		return false, logEventsAndPhase(ctx, cli, ds)
	}
	return true, nil
}

func logEventsAndPhase(ctx context.Context, cli client.Client, ds *appsv1.DaemonSet) error {
	podList := &corev1.PodList{}
	err := cli.List(ctx, podList, &client.ListOptions{
		Namespace:     ds.Namespace,
		LabelSelector: labels.SelectorFromSet(ds.Spec.Selector.MatchLabels),
	})
	if err != nil {
		return fmt.Errorf("failed to list pods for DaemonSet %s; %w", client.ObjectKeyFromObject(ds).String(), err)
	}
	for _, pod := range podList.Items {
		if pod.Status.Phase != corev1.PodRunning {
			podKey := client.ObjectKeyFromObject(&pod).String()
			testlog.Warningf("daemonset %s pod %s is not running, expected status %s got %s", ds.Name, podKey, corev1.PodRunning, pod.Status.Phase)
			podEvents, err := events.GetEventsForObject(cli, pod.Namespace, pod.Name, string(pod.UID))
			if err != nil {
				return fmt.Errorf("failed to list events for Pod %s; %w", podKey, err)
			}
			for _, event := range podEvents.Items {
				testlog.Warningf("-> %s %s %s", event.Action, event.Reason, event.Message)
			}
		}
	}
	return nil
}

Could you please highlight the diff from the existing implementation?

basically same logic, but keep pushing the happy path leftwards

Tal-or · 2025-01-22T15:59:00Z

thanks, we can go a step further (ofc not blocking)

func IsRunning(ctx context.Context, cli client.Client, namespace, name string) (bool, error) {
	ds, err := GetByName(ctx, cli, namespace, name)
	if err != nil {
		if k8serrors.IsNotFound(err) {
			testlog.Warningf("daemonset %q %q not found - retrying", namespace, name)
			return false, nil
		}
		return false, err
	}
	if isRunning := ds.Status.DesiredNumberScheduled > 0 && ds.Status.DesiredNumberScheduled == ds.Status.NumberReady; !isRunning {
		return false, logEventsAndPhase(ctx, cli, ds)
	}
	return true, nil
}

func logEventsAndPhase(ctx context.Context, cli client.Client, ds *appsv1.DaemonSet) error {
	podList := &corev1.PodList{}
	err := cli.List(ctx, podList, &client.ListOptions{
		Namespace:     ds.Namespace,
		LabelSelector: labels.SelectorFromSet(ds.Spec.Selector.MatchLabels),
	})
	if err != nil {
		return fmt.Errorf("failed to list pods for DaemonSet %s; %w", client.ObjectKeyFromObject(ds).String(), err)
	}
	for _, pod := range podList.Items {
		if pod.Status.Phase != corev1.PodRunning {
			podKey := client.ObjectKeyFromObject(&pod).String()
			testlog.Warningf("daemonset %s pod %s is not running, expected status %s got %s", ds.Name, podKey, corev1.PodRunning, pod.Status.Phase)
			podEvents, err := events.GetEventsForObject(cli, pod.Namespace, pod.Name, string(pod.UID))
			if err != nil {
				return fmt.Errorf("failed to list events for Pod %s; %w", podKey, err)
			}
			for _, event := range podEvents.Items {
				testlog.Warningf("-> %s %s %s", event.Action, event.Reason, event.Message)
			}
		}
	}
	return nil
}

Could you please highlight the diff from the existing implementation?

basically same logic, but keep pushing the happy path leftwards

it does. unless I'm missing something

Tal-or · 2025-01-23T08:15:04Z

/retest
infra issues

ffromani · 2025-02-03T13:59:39Z

/approve

ffromani · 2025-02-03T14:08:58Z

/label acknowledge-critical-fixes-only

test code can't break the product and actually helps improving the product quality

yanirq · 2025-03-20T13:56:01Z

test/e2e/performanceprofile/functests/utils/daemonset/daemonset.go

 	testlog.Infof("wait for the daemonset %q %q to be running", namespace, name)
-	return wait.PollUntilContextTimeout(context.TODO(), 10*time.Second, timeout, true, func(ctx context.Context) (bool, error) {
-		return IsRunning(cli, namespace, name)
+	return wait.PollUntilContextTimeout(ctx, 30*time.Second, timeout, true, func(ctx2 context.Context) (bool, error) {


nit : I would use a more meaningful name than ctx2 (e.g. timeoutCtx or derivedCtx)

Add context from the caller instead of local `context.TODO()`. Signed-off-by: Talor Itzhak <[email protected]>

Signed-off-by: Talor Itzhak <[email protected]>

less iterations means less spamming especially within the nodeinspector which log some data for every iteration. Signed-off-by: Talor Itzhak <[email protected]>

In case the DaemonSet's pods are not running for some reason, we want to log the pod's phase and event for better observability. This could help us with finding the root cause for the reason the pod didn't start. Signed-off-by: Talor Itzhak <[email protected]>

openshift-ci · 2025-03-20T14:03:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, Tal-or

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:

~~OWNERS~~ [ffromani]

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

Tal-or · 2025-04-10T09:17:43Z

/retest

Tal-or · 2025-05-05T11:27:26Z

/retest

Tal-or · 2025-05-05T11:27:53Z

/cc @ffromani @yanirq

ffromani · 2025-05-05T13:27:34Z

/lgtm

openshift-ci-robot · 2025-05-06T13:09:05Z

@Tal-or: This pull request explicitly references no jira issue.

In response to this:

In case the DaemonSet's pods are not running for some reason,
we want to log the pod's phase and event for
better observability.

This could help us with finding the root cause
for the reason the pod didn't start.

Signed-off-by: Talor Itzhak [email protected]

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-robot · 2025-05-06T15:22:05Z

/retest-required

Remaining retests: 0 against base HEAD 7172669 and 2 for PR HEAD 1899f7e in total

openshift-ci · 2025-05-06T17:51:24Z

@Tal-or: all tests passed!

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

openshift-bot · 2025-05-06T21:27:33Z

[ART PR BUILD NOTIFIER]

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

openshift-ci bot requested review from rbaturov and yanirq January 22, 2025 13:50

ffromani reviewed Jan 22, 2025

View reviewed changes

swatisehgal reviewed Jan 22, 2025

View reviewed changes

test/e2e/performanceprofile/functests/utils/daemonset/daemonset.go Outdated Show resolved Hide resolved

Tal-or force-pushed the node_inspector_events branch from 3e0bea5 to 963cc89 Compare January 22, 2025 14:10

Tal-or force-pushed the node_inspector_events branch from 963cc89 to 7de0434 Compare January 22, 2025 14:51

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2025

openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Feb 3, 2025

yanirq reviewed Mar 20, 2025

View reviewed changes

Tal-or added 4 commits March 20, 2025 16:00

e2e: add context

dfb9ca1

Add context from the caller instead of local `context.TODO()`. Signed-off-by: Talor Itzhak <[email protected]>

e2e: remove redundant parentheses

59dcfd1

Signed-off-by: Talor Itzhak <[email protected]>

e2e:daemonset: increase interval time

f85bc3f

less iterations means less spamming especially within the nodeinspector which log some data for every iteration. Signed-off-by: Talor Itzhak <[email protected]>

Tal-or force-pushed the node_inspector_events branch from 7de0434 to 1899f7e Compare March 20, 2025 14:03

openshift-ci bot requested review from ffromani and yanirq May 5, 2025 11:27

openshift-ci bot assigned ffromani May 5, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2025

Tal-or changed the title ~~e2e:daemonset: pod phase and events~~ NO-JIRA:e2e:daemonset: pod phase and events May 6, 2025

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 6, 2025

openshift-merge-bot bot merged commit a3371e1 into openshift:main May 6, 2025
19 checks passed

NO-JIRA:e2e:daemonset: pod phase and events #1289

NO-JIRA:e2e:daemonset: pod phase and events #1289

Uh oh!

Conversation

Tal-or commented Jan 22, 2025

Uh oh!

ffromani Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

Tal-or Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tal-or Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ffromani commented Jan 22, 2025

Uh oh!

Tal-or commented Jan 22, 2025

Uh oh!

ffromani commented Jan 22, 2025

Uh oh!

Tal-or commented Jan 22, 2025

Uh oh!

Tal-or commented Jan 23, 2025

Uh oh!

ffromani commented Feb 3, 2025

Uh oh!

ffromani commented Feb 3, 2025

Uh oh!

yanirq Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Mar 20, 2025

Uh oh!

Tal-or commented Apr 10, 2025

Uh oh!

Tal-or commented May 5, 2025

Uh oh!

Tal-or commented May 5, 2025

Uh oh!

ffromani commented May 5, 2025

Uh oh!

openshift-ci-robot commented May 6, 2025

Uh oh!

openshift-ci-robot commented May 6, 2025

Uh oh!

openshift-ci bot commented May 6, 2025

Uh oh!

Uh oh!

openshift-bot commented May 6, 2025

Uh oh!

Uh oh!

Tal-or Jan 22, 2025 •

edited

Loading