Ignore reconcile errors that occur because a pod is being terminated #1233

kevinearls · 2022-11-08T15:00:14Z

Signed-off-by: Kevin Earls [email protected]

This fixes #1219 by not logging errors that occur because we are trying to reconcile something in a namespace that is being terminated.

Signed-off-by: Kevin Earls <[email protected]>

frzifus · 2022-11-09T10:17:44Z

controllers/opentelemetrycollector_controller.go

@@ -168,9 +169,12 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct
 func (r *OpenTelemetryCollectorReconciler) RunTasks(ctx context.Context, params reconcile.Params) error {
 	for _, task := range r.tasks {
 		if err := task.Do(ctx, params); err != nil {
-			r.log.Error(err, fmt.Sprintf("failed to reconcile %s", task.Name))


How often does this logline occur when a pod is terminated?

Many. :-). Like for everything the reconciler touches. To be honest, it's not that big a deal, and probably something that will only be seen when running kuttl tests. But I found it very annoying, so I submitted the PR.

Would it make sense to end the reconciling with by returing nil if the reason is a terminating pod? That way the user gets informed that something did not work and we would get rid of all the unnecessary log lines?

Signed-off-by: Kevin Earls <[email protected]>

frzifus · 2022-11-14T18:36:35Z

controllers/opentelemetrycollector_controller.go

@@ -168,6 +169,10 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct
 func (r *OpenTelemetryCollectorReconciler) RunTasks(ctx context.Context, params reconcile.Params) error {
 	for _, task := range r.tasks {
 		if err := task.Do(ctx, params); err != nil {
+			// If we get an error that occur because a pod is being terminated then exit this loop
+			if apierrors.IsForbidden(err) && strings.Contains(err.Error(), "because it is being terminated") {


one last thing, should we print the termination logline before? And maybe an extra hint, that we cancel the reconciliation?

@frzifus what log level should I print that at?

i thought maybe:

r.log.Error(err, fmt.Sprintf("failed to reconcile %s", task.Name))

And maybe an additional line that says that the reconciliation will be canceled?

I am a bit skeptical about checking the string because it is being terminated". This could easily change in the future

You might be able to check for v1.NamespaceTerminatingCause in the error: https://github.com/kubernetes/apiserver/blob/v0.26.0-rc.0/pkg/admission/plugin/namespace/lifecycle/admission.go#L177

@damemi Thanks!

Signed-off-by: Kevin Earls <[email protected]>

frzifus

up to the others

kevinearls · 2022-11-16T12:48:06Z

@pavolloffay Can you merge this?

Signed-off-by: Kevin Earls <[email protected]>

damemi · 2022-11-22T19:15:39Z

controllers/opentelemetrycollector_controller.go

@@ -168,6 +168,11 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct
 func (r *OpenTelemetryCollectorReconciler) RunTasks(ctx context.Context, params reconcile.Params) error {
 	for _, task := range r.tasks {
 		if err := task.Do(ctx, params); err != nil {
+			// If we get an error that occurs because a pod is being terminated, then exit this loop
+			if apierrors.IsForbidden(err) && apierrors.HasStatusCause(err, corev1.NamespaceTerminatingCause) {
+				r.log.V(2).Info("Exiting reconcile loop because namespace", params.Instance.Namespace, "is being terminated")


Suggested change

r.log.V(2).Info("Exiting reconcile loop because namespace", params.Instance.Namespace, "is being terminated")

r.log.V(2).Info("Exiting reconcile loop because namespace is being terminated", "namespace", params.Instance.Namespace)

(see https://pkg.go.dev/github.com/go-logr/logr#Logger.Info)

Signed-off-by: Kevin Earls <[email protected]>

secustor

Linking issue #1291

…pen-telemetry#1233) * Ignore reconcile errors that occur because a pod is being terminated Signed-off-by: Kevin Earls <[email protected]> * Appease the all powerfull linter Signed-off-by: Kevin Earls <[email protected]> * Change behavior to end reconcile loop if pod has been terminated Signed-off-by: Kevin Earls <[email protected]> * Print a log message if we exit the reconciler loop Signed-off-by: Kevin Earls <[email protected]> * Look for NamespaceTerminatingCause Signed-off-by: Kevin Earls <[email protected]> * Appease the almighty linter Signed-off-by: Kevin Earls <[email protected]> * Fix log message Signed-off-by: Kevin Earls <[email protected]> * Skip flaky test Signed-off-by: Kevin Earls <[email protected]> Signed-off-by: Kevin Earls <[email protected]> Co-authored-by: Ben B <[email protected]>

kevinearls added 2 commits November 8, 2022 15:57

Ignore reconcile errors that occur because a pod is being terminated

aa9ab2d

Signed-off-by: Kevin Earls <[email protected]>

Appease the all powerfull linter

77fc4a9

Signed-off-by: Kevin Earls <[email protected]>

kevinearls marked this pull request as ready for review November 8, 2022 15:39

kevinearls requested a review from a team November 8, 2022 15:39

frzifus reviewed Nov 9, 2022

View reviewed changes

Change behavior to end reconcile loop if pod has been terminated

0896ae9

Signed-off-by: Kevin Earls <[email protected]>

frzifus reviewed Nov 14, 2022

View reviewed changes

Print a log message if we exit the reconciler loop

7b0267e

Signed-off-by: Kevin Earls <[email protected]>

frzifus approved these changes Nov 14, 2022

View reviewed changes

kevinearls and others added 2 commits November 15, 2022 15:27

Merge branch 'main' into ignore-being-terminated-errors

189dcd6

Merge branch 'main' into ignore-being-terminated-errors

c43dd28

kevinearls added 3 commits November 22, 2022 09:28

Merge branch 'main' into ignore-being-terminated-errors

ff5b12c

Look for NamespaceTerminatingCause

0af9e90

Signed-off-by: Kevin Earls <[email protected]>

Appease the almighty linter

cd7d18e

Signed-off-by: Kevin Earls <[email protected]>

damemi reviewed Nov 22, 2022

View reviewed changes

kevinearls added 4 commits November 30, 2022 04:20

Merge branch 'main' into ignore-being-terminated-errors

cd56efd

Merge branch 'main' into ignore-being-terminated-errors

81c2657

Fix log message

e9743b6

Signed-off-by: Kevin Earls <[email protected]>

Skip flaky test

1213f7b

Signed-off-by: Kevin Earls <[email protected]>

kevinearls requested a review from a team November 30, 2022 14:00

secustor approved these changes Nov 30, 2022

View reviewed changes

secustor mentioned this pull request Nov 30, 2022

TestSmartCollectorReassignment is flaky #1291

Closed

Merge branch 'main' into ignore-being-terminated-errors

63f0ecf

kevinearls mentioned this pull request Nov 30, 2022

[chore] Change the E2E automation to run the tests in OpenShift #1254

Merged

pavolloffay merged commit ce30a35 into open-telemetry:main Dec 1, 2022

kevinearls deleted the ignore-being-terminated-errors branch December 1, 2022 09:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore reconcile errors that occur because a pod is being terminated #1233

Ignore reconcile errors that occur because a pod is being terminated #1233

kevinearls commented Nov 8, 2022

frzifus Nov 9, 2022

kevinearls Nov 9, 2022 •

edited

Loading

frzifus Nov 9, 2022

frzifus Nov 14, 2022

kevinearls Nov 14, 2022

frzifus Nov 14, 2022

pavolloffay Nov 21, 2022

damemi Nov 21, 2022

kevinearls Nov 22, 2022

frzifus left a comment

kevinearls commented Nov 16, 2022

damemi Nov 22, 2022

secustor left a comment

	r.log.V(2).Info("Exiting reconcile loop because namespace", params.Instance.Namespace, "is being terminated")
	r.log.V(2).Info("Exiting reconcile loop because namespace is being terminated", "namespace", params.Instance.Namespace)

Ignore reconcile errors that occur because a pod is being terminated #1233

Ignore reconcile errors that occur because a pod is being terminated #1233

Conversation

kevinearls commented Nov 8, 2022

Choose a reason for hiding this comment

kevinearls Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frzifus left a comment

Choose a reason for hiding this comment

kevinearls commented Nov 16, 2022

Choose a reason for hiding this comment

secustor left a comment

Choose a reason for hiding this comment

kevinearls Nov 9, 2022 •

edited

Loading