Introduce readiness check and proper wait in `init --wait` #1629

alenkacz · 2020-08-04T15:19:40Z

What this PR does / why we need it:
This is part of the solution for #1625

Signed-off-by: Alena Varkockova <varkockova.a@gmail.com>

kensipe · 2020-08-04T16:41:49Z

One thing to consider... is while liveness probes and readiness probes are v1.0 stable... startup probes are v1.16 [alpha]. I don't think this is viable for 1.15 or earlier... I also believe konvoy is based on 1.15.
We would have to claim a requirement on kube 1.16+ for a release with this solution.

kensipe · 2020-08-04T16:48:43Z

for reference: https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#when-should-you-use-a-startup-probe

…nt on readiness probe Signed-off-by: Ken Sipe <kensipe@gmail.com>

kensipe

/lgtm

zen-dog · 2020-08-04T19:31:30Z

pkg/kudoctl/kudoinit/manager/manager.go

@@ -32,6 +32,8 @@ type Initializer struct {
 	deployment *appsv1.StatefulSet
 }

+const Name = "manager"


We started to consolidate all the hard-coded names in types.go. Please move this one there too

I much prefer it here... packages communicate good meaning in Go and manager.Name is very clear and concise IMO

just reviewed types.go... not a fan of what is there. for instance ManagerName is not true... that is the manager pod name. IMO it would be better to have manager.PodName than kudoinit.DefaultManagerName which isn't correct... Based on this assertion... perhaps manager.Name could be more accurate as manager.ContainerName but it seems like a reasonable abbrev. It seems to we that all other uses of types.go in the maturer kube eco-system is for values used in the API. For kudoinit, that would be types / options used in Artifacter or InstallVerifier, Step, etc. The constants defined here now, do not adhere to that and thus seem off to me. Without tribal nugging I wouldn't know or to think to put them there. What is the position in having them there?

It's subjective of course but I do like kudoinit.DefaultManagerName more than manager.Name. First clearly indicates that this is for the init while the second could be anything manager related. I agree that some naming can be improved but my main argument is about consistency: we can have these constants one way (all in one place) or another (each in its own package) but please not both.

agree on subjectivity... part of my questioning is the scenario you are bringing up... for the current use case this is init so kudoinit makes sense... but it is likely that we would use these constants in other ways which isn't init... By defining them in the package for which they define state they can be leverage by packages that define behavior (if it is a separate package). this doesn't work well the other way IMO.
It would be great to meet around some standards and conventions as well. It would be good to get more aligned on this... having constants "all in one place" isn't a goal that makes sense to me... at least not in isolation.

zen-dog · 2020-08-04T19:33:42Z

pkg/kudoctl/kudoinit/setup/wait.go

-	if err == nil && image == expectedImage {
+func verifyKudoDeployment(client corev1.PodsGetter, namespace string) (bool, error) {
+	ready, err := isKUDOPodReady(client, namespace)
+	if err == nil && ready {


Not that I object but why was the image comparison removed?

good question... I didn't see any value in it

I think the value was being able to establish whether it's the version of pod we expect. Not useful when the pod is being created initially, but when we are rolling out an update to the manager deployment, we need to guard against declaring victory after just checking that the pre-existing pod is healthy, i.e. the following race scenario:

apply statefulset change

statefulset controller is lazy, doing nothing yet

we check the (old!) pod health, hey! it's healthy, consider the rollout done

only now does the statefulset controller replace the pod with the new one...

in agreement with the need for awareness of this scenario...

currently we do NOT apply a change to statefulset... we delete, then apply

I'm not confident that image version change is sufficient... for instance, is it possible that a user may want to "upgrade" to the latest service-accounts, crds, etc, but use a previous version of kudo manager? The version is a flag controllable thing... Assuming it is the same version of the cli is likely the 80% use case... perhaps more.. but doesn't capture all possibilities.

it would be better IMO to either wait for deletes to complete... or capture the current statefulset prior to upgrade and check revision. I'll create an Issue to capture this.

porridge · 2020-08-05T05:24:01Z

pkg/kudoctl/cmd/testdata/deploy-kudo-ns.yaml.golden

@@ -586,6 +586,9 @@ spec:
        - containerPort: 443
          name: webhook-server
          protocol: TCP
+        readinessProbe:


Looks like it is readiness probe after all? The PR description needs updating, then.

yeah, the startup probe is 1.16+

Haven't we recently introduced something that requires k8s version >= 1.16 anyway?

we did but I removed it again so we're 1.15 :)

porridge · 2020-08-05T05:30:37Z

pkg/kudoctl/kudoinit/setup/wait.go

-	if err == nil && image == expectedImage {
+func verifyKudoDeployment(client corev1.PodsGetter, namespace string) (bool, error) {
+	ready, err := isKUDOPodReady(client, namespace)
+	if err == nil && ready {


I think the value was being able to establish whether it's the version of pod we expect. Not useful when the pod is being created initially, but when we are rolling out an update to the manager deployment, we need to guard against declaring victory after just checking that the pre-existing pod is healthy, i.e. the following race scenario:

apply statefulset change

statefulset controller is lazy, doing nothing yet

we check the (old!) pod health, hey! it's healthy, consider the rollout done

only now does the statefulset controller replace the pod with the new one...

ANeumann82

lgtm

Signed-off-by: Ken Sipe <kensipe@gmail.com>

alenkacz requested review from gerred, kensipe, nfnt and zen-dog as code owners August 4, 2020 15:19

Introduce a TCP readineess check

e811dad

Signed-off-by: Alena Varkockova <varkockova.a@gmail.com>

alenkacz force-pushed the av/readiness-tcp branch from cfb620f to e811dad Compare August 4, 2020 15:20

manager wait now waits for container status of ready which is depende…

d4ee0a1

…nt on readiness probe Signed-off-by: Ken Sipe <kensipe@gmail.com>

kensipe changed the title ~~Introduce a TCP startup check~~ TCP ReadinessProbe added. Init --wait Waits on Ready. Aug 4, 2020

kensipe approved these changes Aug 4, 2020

View reviewed changes

kensipe added this to the 0.16.0 milestone Aug 4, 2020

zen-dog reviewed Aug 4, 2020

View reviewed changes

porridge reviewed Aug 5, 2020

View reviewed changes

alenkacz changed the title ~~TCP ReadinessProbe added. Init --wait Waits on Ready.~~ Introduce readiness check and proper wait in init --wait Aug 5, 2020

ANeumann82 approved these changes Aug 5, 2020

View reviewed changes

kensipe mentioned this pull request Aug 5, 2020

Upgrade (init) to Capture Verify Readiness of the Correct StatefulSet #1631

Open

moved const to colocate with other const per pr feedback

479c9e1

Signed-off-by: Ken Sipe <kensipe@gmail.com>

kensipe merged commit 5c68af2 into main Aug 6, 2020

kensipe deleted the av/readiness-tcp branch August 6, 2020 00:09

alenkacz mentioned this pull request Aug 6, 2020

Looks like --wait on kudo init does not wait for webhook to be ready #1625

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce readiness check and proper wait in `init --wait` #1629

Introduce readiness check and proper wait in `init --wait` #1629

alenkacz commented Aug 4, 2020 •

edited by porridge

Loading

kensipe commented Aug 4, 2020 •

edited

Loading

kensipe commented Aug 4, 2020

kensipe left a comment

zen-dog Aug 4, 2020

kensipe Aug 4, 2020 •

edited

Loading

kensipe Aug 4, 2020

zen-dog Aug 5, 2020

kensipe Aug 5, 2020

zen-dog Aug 4, 2020

kensipe Aug 4, 2020

porridge Aug 5, 2020

kensipe Aug 5, 2020

porridge Aug 5, 2020

porridge Aug 5, 2020

alenkacz Aug 5, 2020

zen-dog Aug 5, 2020

alenkacz Aug 6, 2020

porridge Aug 5, 2020

ANeumann82 left a comment

Introduce readiness check and proper wait in init --wait #1629

Introduce readiness check and proper wait in init --wait #1629

Conversation

alenkacz commented Aug 4, 2020 • edited by porridge Loading

kensipe commented Aug 4, 2020 • edited Loading

kensipe commented Aug 4, 2020

kensipe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kensipe Aug 4, 2020 • 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

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

ANeumann82 left a comment

Choose a reason for hiding this comment

Introduce readiness check and proper wait in `init --wait` #1629

Introduce readiness check and proper wait in `init --wait` #1629

alenkacz commented Aug 4, 2020 •

edited by porridge

Loading

kensipe commented Aug 4, 2020 •

edited

Loading

kensipe Aug 4, 2020 •

edited

Loading