Record the error when the creation pod fails

[utam0k]

When a pod creation fails, EphemeralContainer will tell you why.

Follow up: #3074

[nikola-jokic]

Hey @utam0k,

Thank you for your contributions! Can you please help me understand why would you like to update statuses per failure? To me, it seems like the EphemeralRunner resource should act in a similar way as the job. It should keep the state until something is either successful or not. It shouldn't be inspected after each failure but try its best to actually spin up the pod and only when it assumes it won't be able to start it (with the limit of 5 retries), it should set its state to signal the failure.

We also don't do back-off periods so it would be harder to inspect it anyway. But I would love to hear your opinion ☺️

[utam0k]

Thank you for your contributions! Can you please help me understand why would you like to update statuses per failure?

Sure!

To me, it seems like the EphemeralRunner resource should act in a similar way as the job. It should keep the state until something is either successful or not. It shouldn't be inspected after each failure but try its best to actually spin up the pod and only when it assumes it won't be able to start it (with the limit of 5 retries), it should set its state to signal the failure.

I agree with you. However, errors in pod creation are different from this. In this case, the object(Pod) is not created in the first place because creation requests are denied by kube-api. Therefore, EphemeralRunner statuses will remain empty unless an error is made in Failure at the time of pod creation. If you actually run the test with the following patch, you will see that the status is not updated, and the test will be timed out.

$ git diff controllers/actions.github.com/ephemeralrunner_controller.go
diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go
index 1c010af..920316e 100644
--- a/controllers/actions.github.com/ephemeralrunner_controller.go
+++ b/controllers/actions.github.com/ephemeralrunner_controller.go
@@ -201,15 +201,16 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
                default:
                        // Pod was not found. Create if the pod has never been created
                        log.Info("Creating new EphemeralRunner pod.")
-                       result, err := r.createPod(ctx, ephemeralRunner, secret, log)
-                       if err != nil {
-                               errMessage := fmt.Sprintf("Pod has failed to create: %v", err)
-                               if err := r.markAsFailed(ctx, ephemeralRunner, errMessage, "CreationPodFailure", log); err != nil {
-                                       log.Error(err, "Failed to set ephemeral runner to phase Failed")
-                                       return ctrl.Result{}, err
-                               }
-                       }
-                       return result, nil
+                       return r.createPod(ctx, ephemeralRunner, secret, log)
+                       // result, err := r.createPod(ctx, ephemeralRunner, secret, log)
+                       // if err != nil {
+                       //      errMessage := fmt.Sprintf("Pod has failed to create: %v", err)
+                       //      if err := r.markAsFailed(ctx, ephemeralRunner, errMessage, "CreationPodFailure", log); err != nil {
+                       //              log.Error(err, "Failed to set ephemeral runner to phase Failed")
+                       //              return ctrl.Result{}, err
+                       //      }
+                       // }
+                       // return result, nil
                }
        }

I agree about the 5 retries. That means more ephemeralRunner.Status.Failures) if the pod creation fails, right?

[nikola-jokic]

Oh I misunderstood the original intention of the PR, sorry.

I have another suggestion. I would suggest the following:

1. In constants.go, create two constants

// EphemeralRunner pod creation failure reasons
const (
  ReasonTooManyPodFailures = "TooManyPodFailures"
  ReasonInvalidPodFailure = "InvalidPod"
)

2. Revert back to your original change.
3. In your original change, on the caller side, inspect the error:

result, err := r.createPod(ctx, ephemeralRunner, secret, log)
switch {
  case err == nil:
    return result, nil
  case kerrors.IsInvalid(err):
    // mark ephemeral runner as failed, apply ReasonInvalidPodFailure and add an appropriate message
  default:
    // log the error and return the result, err. So for example, if there is a temporary failure while communicating with the API, we shouldn't count that as a creation failure
}

My reasoning is that we don't want to count on network failures for example. Also, this is a great change because we don't need to retry if the pod is invalid. It will never be successful so let's avoid unnecessary reconciliations.

And thank you for including the test! Does this suggestion make sense? 😄

[utam0k]

@nikola-jokic Thanks for your feedback. They make sense 👍 I have already addressed them. Please take a look at this PR again🙏

[nikola-jokic]

LGTM! Thank you!

[utam0k]

@nikola-jokic Thanks for your quick review 🙏