Recreate dead solver pods during self-check #1388
Conversation
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
jetstack-bot
added
release-note
jetstack-bot
added
area/acme
pkg/issuer/acme/http/pod.go
Outdated
| err := s.cleanupPods(ch) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return nil, fmt.Errorf(errMsg) | ||
| return s.createPod(ch) |
There was a problem hiding this comment.
We can't just change this to call createPod without also making the pod's name determinate else there's a risk we could enter a loop of:
- create a pod/present successfully
- resync occurs and we've not observed the new pod in the lister
- a new pod will be created - because we use GenerateName, the call to the apiservers Create endpoint will succeed, next time we loop around here, we'll delete both pods because multiple exist, and the loop re-runs.
We can avoid this by not using GenerateName in buildPod, and instead using Name (where 'name' is predictable). That way, we'll get an AlreadyExists error if we hit this race, causing the controller to resync after a back-off (which allows for the cache to become consistent)
| @@ -222,4 +226,49 @@ var _ = framework.CertManagerDescribe("ACME Certificate (HTTP01)", func() { | |||
| err = h.WaitCertificateIssuedValid(f.Namespace.Name, certificateName, time.Minute*5) | |||
| Expect(err).NotTo(HaveOccurred()) | |||
| }) | |||
|
|
|||
| It("should obtain a signed certificate with a single CN from the ACME server after solver pod killed", func() { | |||
There was a problem hiding this comment.
maybe change to "should automatically recreate challenge pod and still obtain a certificate if it is manually deleted"? (just thinking about how we'll read these test failure messages in a few months time when we start breaking things 😅)
| for _, p := range podlist.Items { | ||
| log.Logf("solver pod %s", p.Name) | ||
| // TODO(dmo): make this cleaner instead of just going by name | ||
| if strings.Contains(p.Name, "http-solver") { |
There was a problem hiding this comment.
👍 not important for this PR really, but we can use a label selector in the List call above to ensure we get the correct pods 😄 (similar to how we do it in ensurePod)
| By("killing the solver pod") | ||
| podClient := f.KubeClientSet.CoreV1().Pods(f.Namespace.Name) | ||
| var pod corev1.Pod | ||
| err = wait.PollImmediate(500*time.Millisecond, time.Minute, |
There was a problem hiding this comment.
nit: maybe change Poll to 1s? 500ms might put systems under strain if we're running 20 tests concurrently
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
This reverts commit 6b81093. Signed-off-by: Daniel Morsing <dmo@jetstack.io>
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
|
/retest |
|
😄 /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DanielMorsing, munnerz 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:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
While solver pods are fairly simple and shouldn't die, they can be terminated for a variety of reasons, like draining nodes or overzealous sysadmins. Recreate the solver pod if it goes away while we're waiting for the self-check to pass.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged):fixes #1311
Release note: