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?

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]>

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

openshift-ci · 2025-01-23T10:46:09Z

@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.

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

/approve

openshift-ci · 2025-02-03T14:00:06Z

[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

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

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 added 4 commits January 22, 2025 16:51

e2e: add context

34dffcf

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

e2e: remove redundant parentheses

a40392b

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

e2e:daemonset: increase interval time

0d04b03

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 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e:daemonset: pod phase and events #1289

e2e:daemonset: pod phase and events #1289

Tal-or commented Jan 22, 2025

ffromani Jan 22, 2025

Tal-or Jan 22, 2025 •

edited

Loading

Tal-or Jan 22, 2025

ffromani commented Jan 22, 2025

Tal-or commented Jan 22, 2025

ffromani commented Jan 22, 2025

Tal-or commented Jan 22, 2025

Tal-or commented Jan 23, 2025

openshift-ci bot commented Jan 23, 2025

ffromani commented Feb 3, 2025

openshift-ci bot commented Feb 3, 2025

ffromani commented Feb 3, 2025

e2e:daemonset: pod phase and events #1289

Are you sure you want to change the base?

e2e:daemonset: pod phase and events #1289

Conversation

Tal-or commented Jan 22, 2025

ffromani Jan 22, 2025

Choose a reason for hiding this comment

Tal-or Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Tal-or Jan 22, 2025

Choose a reason for hiding this comment

ffromani commented Jan 22, 2025

Tal-or commented Jan 22, 2025

ffromani commented Jan 22, 2025

Tal-or commented Jan 22, 2025

Tal-or commented Jan 23, 2025

openshift-ci bot commented Jan 23, 2025

ffromani commented Feb 3, 2025

openshift-ci bot commented Feb 3, 2025

ffromani commented Feb 3, 2025

Tal-or Jan 22, 2025 •

edited

Loading