fix: reflect pod failed creates on the chaperon status

[marwanad]

This fixes a bug where pod chaperons in a target cluster can delay the scheduling loop if the pods fail to create and no status is set on the chaperon.

This changes the behavior to set PodScheduled condition with reason PodFailedCreate and checking for that in the proxy filter step. The pod creation gets requeued and retried. Upon success, the chaperon status will inherit the pod one as before.

[adrienjt]

This could use an e2e test.

[adrienjt]

I don't understand. If the candidate pod creation failed, why would podChaperon.Status.Phase not be empty?

[marwanad]

So prior to this change, the following will happen:

1. Attempt to create the pod.
   

   admiralty/pkg/controllers/chaperon/controller.go

   Line 120 in f98c59d

   pod, err = c.kubeclientset.CoreV1().Pods(podChaperon.Namespace).Create(ctx, newPod(podChaperon), metav1.CreateOptions{})

2. Pod creation fails.
   

   admiralty/pkg/controllers/chaperon/controller.go

   Line 122 in f98c59d

   return nil, fmt.Errorf("cannot create pod for pod chaperon %v", err)

   and method returns
3. The status setting code on the chaperon never gets to run because of the early exit above so the chaperon status is never updated.
   

   admiralty/pkg/controllers/chaperon/controller.go

   Line 158 in f98c59d

   if needStatusUpdate {

[adrienjt]

After this change, only the condition is updated, so the phase should still be empty, right? So line 235 (return true) is never executed. Checking the condition here appears to be unnecessary.

[marwanad]

woops I misunderstood your earlier question, good catch! I forgot to also set the phase here. I think it would be reasonable to set the phase to Pending in that failed pod create case since Failed generally implies container termination.

[adrienjt]

1. The e2e test tests the implementation, not that the delay issue is fixed.
2. This breaks the invariant that so far the pod chaperon status was simply the candidate pod status. Using the phase and condition to store a candidate pod creation error feels hacky. Could you store the pod creation error as a chaperon annotation instead? Sorry to mention that only now.