Dataplane: Add gRPC health check

[Shunpoco]

Currently our dataplane server doesn't have health check, so we can't confirm whether the dataplane daemonset runs properly or not.

This PR introduce health check for gRPC to the dataplane server, and add liveness, readiness, and startup probe for the daemonset.

Fixes: #51

[shaneutt]

Awesome, thanks for adding this @Shunpoco! 🥳

One thing we'll need to add is somewhere in the test a simple verification. I think right after deploying the dataplane just a requires.Eventually() that ensures we reached the expected state would be suffiient for now.

[Shunpoco]

@shaneutt
Let me ask about the verification for readiness of the dataplane container.
WaitForBlixtReadiness already checks if the dataplane pod is ready at least in one node here.
I think before adding the readiness probe, k8s decided that the dataplane pod is ready if a container in the pod is not down (it ignores whether a container shows error or not).
After adding the readiness probe, k8s decides if the dataplane pod is ready using the probe.
So the correctness of the result of WaitForBlixtReadiness will be improved by the readiness probe, but I'm wondering we should add more verification or current implementation is enough.

[shaneutt]

@shaneutt Let me ask about the verification for readiness of the dataplane container. WaitForBlixtReadiness already checks if the dataplane pod is ready at least in one node here. I think before adding the readiness probe, k8s decided that the dataplane pod is ready if a container in the pod is not down (it ignores whether a container shows error or not). After adding the readiness probe, k8s decides if the dataplane pod is ready using the probe. So the correctness of the result of WaitForBlixtReadiness will be improved by the readiness probe, but I'm wondering we should add more verification or current implementation is enough.

To me this seems like enough of an iterative improvement for now that we can move forward with this, see how it works out, and maybe down the road considering if we want to do more.

[shaneutt]

Hey @Shunpoco just checking in, everything going OK? Need any assistance?

[Shunpoco]

@shaneutt
Thanks for the ping!
I think once @aryan9600 reviews this change and approves it, this PR is ready for merge 😄

[shaneutt]

@Shunpoco, did you see my comment above about adding a test? Are you comfortable with adding a test.Eventually() somewhere to the existing integration tests that verifies that the health checks are present and that the expected state is eventually reached? Let us know if you have any questions or confusion on that, the test suite is mostly pretty straightforward, but as I understand it, it has some trouble running on non-Linux systems.

[Shunpoco]

@shaneutt
OK, let me clarify, I think I misunderstood.
In my comment, I wanted to say that WaitForBlixtReadiness already checks whether pod is ready, so we don't need to put requires.Eventually() for the readiness probe.
But do you want me to add an additional codes for check the pod's status?

[shaneutt]

@shaneutt OK, let me clarify, I think I misunderstood. In my comment, I wanted to say that WaitForBlixtReadiness already checks whether pod is ready, so we don't need to put requires.Eventually() for the readiness probe. But do you want me to add an additional codes for check the pod's status?

Yeah, my thinking was this: you're right that the current tests effectively exercise the health checks, but nothing is verifying their presence or configuration. A simple couple line check could help us to avoid regressions (though we would hope they would never happen!) or mistakes in test configuration later on and just double check that the health checks are present, and are configured as intended. Let me know what you think?

[Shunpoco]

@shaneutt

I got it. I will add the verification.

I pushed da1be5f as a draft of the verification (I will blush up after I confirm we have the same view).

• I put waitForDataplaneReadiness function in suite_test.go. The function does: 1) check if the dataplane pod has readinessprobe (if not, return an error), and then 2) check if the all dataplane pods are ready.
• Call the function in TestMain after the dataplane is deployed.

Note: Because TestMain doesn't take testing.T, I don't use require.Eventually.

Does this change suit for your intention?

[shaneutt]

Yep, this looks good. Might seem a little superfluous but I appreciate you adding it as a documentation of intent, and on the off chance the health checks fail, now we can fail at a very specific place with specific logging which will be easier for debugging.

I'll mark this draft temporarily since you said you wanted to do a couple more things, please mark as ready and call me in for a final review whenever you're ready. Very much appreciate your contribution!

[Shunpoco]

@shaneutt
Now this PR is ready for review! please check c7fc302.

[shaneutt]

Looking good!

Couple minor comments to sort out then I think we're ready to 🚢

[aryan9600]

looks pretty good, just have a couple of nits. also it'd be great if you could squash all your commits into 1-2 concise commits, thanks!

[aryan9600]

LGTM!
Thanks @Shunpoco 🎖️

[shaneutt]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aryan9600, shaneutt, Shunpoco

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:

• OWNERS [aryan9600,shaneutt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment