agent: smarter readiness check #485

ipochi · 2023-04-12T13:44:44Z

This commit introduces a smarter way for the readiness check.

Currently as soon as the agent is up, it signals itself as ready regardless of whether its actually connected to the server or not.

This commit introduces a callback function and a connected field in ClientSet struct to update the status of the agent -> proxy-server connection at regular intervals.

Concern: This makes the assumption that the client is connected to at least one proxy server and therefore the status is ready.

I've thought about updating the readiness of the agent only when all proxy-servers are connected but wasn't sure about the implications from the kubernetes side, ~~since this being a readiness check , kubernetes might treat the pod as not ready and not send any traffic.~~

EDIT: Since the agent pods are not behind a Kubernetes service, the readiness of the pod doesn't really matter.

Please share your reviews,concerns to the approach I've taken.

/cc: @jveski @cheftako @jkh52 @tallclair

k8s-ci-robot · 2023-04-12T13:44:54Z

Hi @ipochi. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jkh52 · 2023-04-14T17:22:30Z

/ok-to-test

cmd/agent/app/server.go

tests/readiness_test.go

pkg/agent/clientset.go

ipochi · 2023-06-01T14:55:51Z

/test pull-apiserver-network-proxy-test

cmd/agent/app/server.go

pkg/agent/readiness.go

tests/readiness_test.go

jkh52 · 2023-06-01T21:20:43Z

This looks pretty close.

@mihivagyok can you verify that this meets the requirements of #491?

tests/readiness_test.go

jkh52 · 2023-07-26T16:48:44Z

/retest

jkh52 · 2023-07-27T18:16:35Z

cmd/agent/app/options/options.go

@@ -117,6 +119,7 @@ func (o *GrpcProxyAgentOptions) Flags() *pflag.FlagSet {
 	flags.StringVar(&o.AgentIdentifiers, "agent-identifiers", o.AgentIdentifiers, "Identifiers of the agent that will be used by the server when choosing agent. N.B. the list of identifiers must be in URL encoded format. e.g.,host=localhost&host=node1.mydomain.com&cidr=127.0.0.1/16&ipv4=1.2.3.4&ipv4=5.6.7.8&ipv6=:::::&default-route=true")
 	flags.BoolVar(&o.WarnOnChannelLimit, "warn-on-channel-limit", o.WarnOnChannelLimit, "Turns on a warning if the system is going to push to a full channel. The check involves an unsafe read.")
 	flags.BoolVar(&o.SyncForever, "sync-forever", o.SyncForever, "If true, the agent continues syncing, in order to support server count changes.")
+	flags.BoolVar(&o.EnableAgentServerReadiness, "agent-server-readiness", o.EnableAgentServerReadiness, "If true, the agent readiness probe checks if its connected to atleast one server.")


flags.BoolVar(&o.EnableAgentServerReadiness, "agent-server-readiness",

Did you add this flag based on a misunderstanding with @cheftako comments (about changing liveness behavior)? I don't remember saying that we need a flag to enable the new readiness behavior (I am ok with not flag guarding the readiness change).

If we keep the flag: since this is the agent binary, "agent" in name seems somewhat redundant. Maybe "--enable-connected-readiness" or similar?

Yes, I think there was a confusion on that. I too don't think a flag is necessary.
I'm happy to remove the commit related to addition of the flag.

removed the commit.

jkh52 · 2023-07-27T18:32:56Z

cmd/agent/app/server.go

 	readinessHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		fmt.Fprintf(w, "ok")
+		var failedChecks []string


This section is very close to fully generic. It would be nice to organize it somewhere different than cmd/agent to keep that minimal. It would also be good to add some unit test coverage (for example: exercise the verbose codepath).

I'm ok with deferring these suggestions, to unblock making the feature available.

IMO, I'd like to get this merged as is.

The addition of unit tests, while useful would require more thoughts and time and the result would be a very trivial check and not worth the effort and time.

This commit introduces a smarter way for the readiness check. Currently as soon as the agent is up, it signals itself as ready regardless of whether its actually connected to the server or not. This commit introduces a readiness check in the Agent which reports true if its connected to atleast one proxy server. Signed-off-by: Imran Pochi <imranpochi@microsoft.com>

ipochi · 2023-08-08T17:00:00Z

@jkh52 @cheftako

friendly ping. Shall we merge this ?

jkh52 · 2023-08-08T17:22:24Z

@jkh52 @cheftako

friendly ping. Shall we merge this ?

Did you see the feedback: But can you make small adjustments to make it more obvious to future readers? For example, only introduce readiness.go (not healthz.go), and avoid code naming like "HealthZ" (which some readers may think implies liveness health).

I'm actually OK with merging as-is, and we can always address the above separately. Adding a hold just so that @cheftako can also look.

/hold
/lgtm
/approve

k8s-ci-robot · 2023-08-08T17:22:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ipochi, jkh52

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~~ [jkh52]

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

Signed-off-by: Imran Pochi <imranpochi@microsoft.com>

jkh52 · 2023-08-09T16:31:49Z

/lgtm

Feel free to remove the hold in a day or two, if no other reviewers have feedback.

ipochi · 2023-08-09T19:35:35Z

/lgtm

Feel free to remove the hold in a day or two, if no other reviewers have feedback.

@jkh52 How does one remove hold ?

jkh52 · 2023-08-09T20:34:37Z

/lgtm
Feel free to remove the hold in a day or two, if no other reviewers have feedback.

@jkh52 How does one remove hold ?

I believe it is "/remove-hold"

ipochi · 2023-08-10T14:19:36Z

/remove-hold

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 12, 2023

k8s-ci-robot requested review from dberkov and Jefftree April 12, 2023 13:44

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 12, 2023

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 12, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 14, 2023

jkh52 reviewed Apr 14, 2023

View reviewed changes

cmd/agent/app/server.go Outdated Show resolved Hide resolved

jkh52 reviewed Apr 14, 2023

View reviewed changes

cmd/agent/app/server.go Outdated Show resolved Hide resolved

jkh52 reviewed Apr 14, 2023

View reviewed changes

tests/readiness_test.go Show resolved Hide resolved

jkh52 reviewed Apr 14, 2023

View reviewed changes

pkg/agent/clientset.go Outdated Show resolved Hide resolved

jkh52 mentioned this pull request May 12, 2023

konnectivity-agent - improve readiness probe #491

Closed

ipochi force-pushed the imran/smarter-readiness-check branch from 135bc49 to a61723a Compare June 1, 2023 13:01

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 1, 2023