Support gRPC probe #14134

seongpyoHong · 2023-06-29T02:43:49Z

Proposed Changes

Exec probe by grpc-health-probe is the only way to check gRPC service healthy.

Add gRPC probe that kubernetes support from version 1.24 to make probing for gRPC service more convenient.

Release Note

Support gRPC probe.

knative-prow · 2023-06-29T02:43:58Z

Welcome @seongpyoHong! It looks like this is your first PR to knative/serving 🎉

knative-prow · 2023-06-29T02:43:58Z

Hi @seongpyoHong. Thanks for your PR.

I'm waiting for a knative 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.

config/core/300-resources/configuration.yaml

seongpyoHong · 2023-07-04T15:36:19Z

@evankanderson @mgencur @psschwei (I tagged you because you were designated as a reviewer.)

Could you please review this PR?

psschwei · 2023-07-05T16:38:46Z

cc @dprotaso @skonto

dprotaso

We should also add some unit tests and e2e tests

dprotaso · 2023-07-07T19:07:31Z

pkg/queue/health/probe.go

+	client := grpchealth.NewHealthClient(conn)
+
+	resp, err := client.Check(metadata.NewOutgoingContext(ctx, make(metadata.MD)), &grpchealth.HealthCheckRequest{
+		Service: ptr.ToString(config.Service),


Don't need to pull in a new pkg - we have https://github.com/knative/pkg/blob/main/ptr/ptr.go#L53

dprotaso · 2023-07-07T19:24:46Z

pkg/queue/health/probe.go

+		},
+	}
+
+	v := version.Get()


This doesn't work - this package is from the k8s code and assumes the binary is built with certain linker flags.

I'd look what we're doing with the HTTPProbe

dprotaso · 2023-07-07T19:26:49Z

pkg/queue/health/probe.go

+		if err == context.DeadlineExceeded {
+			return fmt.Errorf("timeout: failed to connect service %q within %v", addr, config.Timeout)
+		} else {
+			return fmt.Errorf("error: failed to connect service at %q", addr)


we should wrap the error in this message - eg. use %w

dprotaso · 2023-07-07T19:27:57Z

pkg/queue/health/probe.go

+				return fmt.Errorf("timeout: health rpc did not complete within %v", config.Timeout)
+			}
+		}
+		return fmt.Errorf("error: health rpc probe failed: %+v", err)


likewise wrap the error using %w

dprotaso · 2023-07-07T19:29:45Z

pkg/queue/health/probe.go

+	dialer := &net.Dialer{
+		Control: func(network, address string, c syscall.RawConn) error {
+			return c.Control(func(fd uintptr) {
+				unix.SetsockoptLinger(int(fd), syscall.SOL_SOCKET, syscall.SO_LINGER, &unix.Linger{Onoff: 1, Linger: 1})
+			})
+		},
+	}


Can you reference where you got this from

https://github.com/kubernetes/kubernetes/blob/master/pkg/probe/dialer_others.go#L33

and mention why we are using these options

I have a question about it.

Is it necessary to handle the Windows container? 🤔

No - we currently don't have windows support

seongpyoHong · 2023-07-09T15:55:32Z

@dprotaso
I've rebased and apply your feedback except unit tests & e2e tests.

I will request a review again when the test-related work is completed. Thank you. 😄

seongpyoHong · 2023-07-10T13:30:11Z

/retest

seongpyoHong · 2023-09-14T14:32:43Z

@dprotaso

Sorry for late response. I added 2 commits.

Ignore ReadinessProbe in user-container when it's gRPC probe(4245075)
Add unit test in pkg/reconciler/revision/resources/deploy_test.go & Modify test to use ephemeral port (dd1b4f5)

codecov · 2023-09-15T16:07:34Z

Codecov Report

Patch coverage: 77.52% and project coverage change: -0.06% ⚠️

Comparison is base (4cb442c) 86.10% compared to head (d3e8464) 86.04%.
Report is 27 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14134      +/-   ##
==========================================
- Coverage   86.10%   86.04%   -0.06%     
==========================================
  Files         196      196              
  Lines       14787    14874      +87     
==========================================
+ Hits        12732    12799      +67     
- Misses       1749     1764      +15     
- Partials      306      311       +5

Files Changed	Coverage Δ
pkg/apis/serving/v1/revision_defaults.go	`95.70% <40.00%> (-1.74%)`	⬇️
pkg/queue/health/probe.go	`53.84% <65.30%> (+4.67%)`	⬆️
pkg/apis/serving/fieldmask.go	`94.66% <100.00%> (+0.12%)`	⬆️
pkg/apis/serving/k8s_validation.go	`94.36% <100.00%> (+0.03%)`	⬆️
pkg/queue/readiness/probe.go	`90.62% <100.00%> (+0.88%)`	⬆️
pkg/reconciler/revision/resources/deploy.go	`90.13% <100.00%> (ø)`
pkg/reconciler/revision/resources/queue.go	`98.44% <100.00%> (+0.02%)`	⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dprotaso · 2023-09-19T18:09:47Z

hey @seongpyoHong thanks for the changes - looks like you have just one small lint error left

https://github.com/knative/serving/actions/runs/6236884761/job/16936938875?pr=14134#step:5:28916

seongpyoHong · 2023-09-21T06:03:36Z

Hi @dprotaso, I resolved lint error.

How can I run GitHub Actions for lint(code check)?

dprotaso · 2023-09-21T19:55:50Z

How can I run GitHub Actions for lint(code check)?

Do you mean locally? We don't have an easy way to do it

What we do is here: https://github.com/knative/actions/blob/main/.github/workflows/reusable-helper-go-style.yaml

dprotaso · 2023-09-21T19:56:24Z

/lgtm
/approve

thanks for all the help 🎉 getting this over the line

knative-prow · 2023-09-21T19:56:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, seongpyoHong

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~~ [dprotaso]
~~pkg/apis/OWNERS~~ [dprotaso]

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

* Add gRPC probe * Modify unit test * Modify unit test * Set default to grpcprobe's service field * Use knative pkg for ptr operation * Use config's KubeMajor & KubeMinor instead of k8s native version pkg * Wrap error in GRPCProbe * Add comment to explain why use dialer_others.go * Run update scripts * Add probe test * Add test in readiness/probe_test.go * update deps * Ignore readinessProbe when it's gRPC * Fix test to use empemeral port * Resolve govet lint error * Use errors.Is to compare * Also use ephemeral port for handler test * drop unneccesary else block (cherry picked from commit d02702e)

* Support gRPC probe (knative#14134) * Add gRPC probe * Modify unit test * Modify unit test * Set default to grpcprobe's service field * Use knative pkg for ptr operation * Use config's KubeMajor & KubeMinor instead of k8s native version pkg * Wrap error in GRPCProbe * Add comment to explain why use dialer_others.go * Run update scripts * Add probe test * Add test in readiness/probe_test.go * update deps * Ignore readinessProbe when it's gRPC * Fix test to use empemeral port * Resolve govet lint error * Use errors.Is to compare * Also use ephemeral port for handler test * drop unneccesary else block (cherry picked from commit d02702e) * Regenerate OCP release artifacts --------- Co-authored-by: Seongpyo Hong <sphong0417@gmail.com>

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/API API objects and controllers labels Jun 29, 2023

knative-prow bot requested review from evankanderson, mgencur and psschwei June 29, 2023 02:44

knative-prow bot added area/autoscale area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Jun 29, 2023

seongpyoHong commented Jun 29, 2023

View reviewed changes

config/core/300-resources/configuration.yaml Show resolved Hide resolved

nak3 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 Jun 29, 2023

dprotaso requested changes Jul 7, 2023

View reviewed changes

knative-prow bot assigned dprotaso Jul 7, 2023

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2023

seongpyoHong force-pushed the add-grpc-probe branch from 34eee90 to f23b828 Compare July 9, 2023 15:53

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2023

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 9, 2023

knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 10, 2023

seongpyoHong force-pushed the add-grpc-probe branch from 63da60d to 3c9307c Compare July 10, 2023 13:52

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 10, 2023

seongpyoHong added 10 commits September 14, 2023 21:05

Use knative pkg for ptr operation

3f9109b

Use config's KubeMajor & KubeMinor instead of k8s native version pkg

9649a83

Wrap error in GRPCProbe

1030861

Add comment to explain why use dialer_others.go

c5e0241

Run update scripts

7550632

Add probe test

27dce74

Add test in readiness/probe_test.go

b376488

update deps

a9e99ef

Ignore readinessProbe when it's gRPC

4245075

Fix test to use empemeral port

dd1b4f5

seongpyoHong force-pushed the add-grpc-probe branch from 4673587 to dd1b4f5 Compare September 14, 2023 12:51

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2023

seongpyoHong requested a review from dprotaso September 14, 2023 14:32

seongpyoHong added 3 commits September 19, 2023 23:15

Resolve govet lint error

20be25d

Use errors.Is to compare

65fffb3

Also use ephemeral port for handler test

c3d9361

drop unneccesary else block

d3e8464

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2023

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2023

knative-prow bot merged commit d02702e into knative:main Sep 21, 2023
70 checks passed

ReToCode mentioned this pull request Jan 9, 2024

[RELEASE-1.11] Backport of Support GRPC probes openshift-knative/serving#582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support gRPC probe #14134

Support gRPC probe #14134

seongpyoHong commented Jun 29, 2023

knative-prow bot commented Jun 29, 2023

knative-prow bot commented Jun 29, 2023

seongpyoHong commented Jul 4, 2023 •

edited

Loading

psschwei commented Jul 5, 2023

dprotaso left a comment

dprotaso Jul 7, 2023

dprotaso Jul 7, 2023

dprotaso Jul 7, 2023

dprotaso Jul 7, 2023

dprotaso Jul 7, 2023

dprotaso Jul 7, 2023

seongpyoHong Jul 9, 2023 •

edited

Loading

dprotaso Jul 10, 2023

seongpyoHong commented Jul 9, 2023

seongpyoHong commented Jul 10, 2023

seongpyoHong commented Sep 14, 2023

codecov bot commented Sep 15, 2023 •

edited

Loading

dprotaso commented Sep 19, 2023

seongpyoHong commented Sep 21, 2023

dprotaso commented Sep 21, 2023

dprotaso commented Sep 21, 2023

knative-prow bot commented Sep 21, 2023

Support gRPC probe #14134

Support gRPC probe #14134

Conversation

seongpyoHong commented Jun 29, 2023

Proposed Changes

knative-prow bot commented Jun 29, 2023

knative-prow bot commented Jun 29, 2023

seongpyoHong commented Jul 4, 2023 • edited Loading

psschwei commented Jul 5, 2023

dprotaso left a comment

Choose a reason for hiding this comment

dprotaso Jul 7, 2023

Choose a reason for hiding this comment

dprotaso Jul 7, 2023

Choose a reason for hiding this comment

dprotaso Jul 7, 2023

Choose a reason for hiding this comment

dprotaso Jul 7, 2023

Choose a reason for hiding this comment

dprotaso Jul 7, 2023

Choose a reason for hiding this comment

dprotaso Jul 7, 2023

Choose a reason for hiding this comment

seongpyoHong Jul 9, 2023 • edited Loading

Choose a reason for hiding this comment

dprotaso Jul 10, 2023

Choose a reason for hiding this comment

seongpyoHong commented Jul 9, 2023

seongpyoHong commented Jul 10, 2023

seongpyoHong commented Sep 14, 2023

codecov bot commented Sep 15, 2023 • edited Loading

Codecov Report

dprotaso commented Sep 19, 2023

seongpyoHong commented Sep 21, 2023

dprotaso commented Sep 21, 2023

dprotaso commented Sep 21, 2023

knative-prow bot commented Sep 21, 2023

seongpyoHong commented Jul 4, 2023 •

edited

Loading

seongpyoHong Jul 9, 2023 •

edited

Loading

codecov bot commented Sep 15, 2023 •

edited

Loading