Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

promote grpcProbes to beta #108522

Merged
merged 1 commit into from
Mar 24, 2022
Merged

Conversation

SergeyKanzhelev
Copy link
Member

/kind feature
/sig node

KEP: kubernetes/enhancements#2727

What this PR does / why we need it:

KEP promotion to beta.

Which issue(s) this PR fixes:

KEP: kubernetes/enhancements#2727

Special notes for your reviewer:

The feature is tested wtth the e2e tests and can be tested in "real life" cluster on GKE:

gcloud config set project $PROJECT
gcloud config set compute/zone asia-east1-c

gcloud container clusters create test-grpc \
    --enable-kubernetes-alpha \
    --no-enable-autorepair \
    --no-enable-autoupgrade \
    --release-channel=rapid \
    --cluster-version=1.23

gcloud container clusters get-credentials test-grpc --zone asia-east1-c --project $PROJECT

Try something like this pod:

apiVersion: v1
kind: Pod
metadata:
  name: etcd-with-grpc
spec:
  containers:
  - name: etcd
    image: k8s.gcr.io/etcd:3.5.1-0
    command: [ "/usr/local/bin/etcd", "--data-dir",  "/var/lib/etcd", "--listen-client-urls", "http://0.0.0.0:2379", "--advertise-client-urls", "http://127.0.0.1:2379", "--log-level", "debug"]
    ports:
    - containerPort: 2379
    livenessProbe:
      grpc:
        port: 2379
      initialDelaySeconds: 10

More end to end tests will be added with the new agnhost command: #108500 This is not blocking

Does this PR introduce a user-facing change?

Support for gRPC probes is now in beta. GRPCContainerProbe feature gate is enabled by default.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. sig/node Categorizes an issue or PR as relevant to SIG Node. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 4, 2022
@k8s-ci-robot k8s-ci-robot added area/code-generation area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Mar 4, 2022
@rphillips
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2022
@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@bowei
Copy link
Member

bowei commented Mar 7, 2022

/lgtm

@SergeyKanzhelev
Copy link
Member Author

/label api-review

@k8s-ci-robot k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Mar 7, 2022
@liggitt liggitt self-assigned this Mar 7, 2022
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KEP included this bit in beta graduation:

Depending on skew strategy:

kubelet version skew ensures all (kubelet ver, cluster ver) support the feature.

only 1.23+ kubelets support the feature, so against a 1.24 server, what would a 1.22 kubelet do with a pod that specified a grpc probe?

pkg/features/kube_features.go Show resolved Hide resolved
test/e2e/common/node/container_probe.go Show resolved Hide resolved
@liggitt
Copy link
Member

liggitt commented Mar 8, 2022

only 1.23+ kubelets support the feature, so against a 1.24 server, what would a 1.22 kubelet do with a pod that specified a grpc probe?

answering my own question, kubelets < 1.24 would return a missing probe handler for ... error by default:

if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.GRPCContainerProbe) && p.GRPC != nil {
host := &(status.PodIP)
service := p.GRPC.Service
klog.V(4).InfoS("GRPC-Probe", "host", host, "service", service, "port", p.GRPC.Port, "timeout", timeout)
return pb.grpc.Probe(*host, *service, int(p.GRPC.Port), timeout)
}

@liggitt
Copy link
Member

liggitt commented Mar 8, 2022

API mechanics lgtm, but the e2e test and kubelet support bit from the KEP need answering before merge

@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Mar 14, 2022
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2022
@SergeyKanzhelev
Copy link
Member Author

/test pull-kubernetes-node-e2e-containerd-features

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 14, 2022
@SergeyKanzhelev
Copy link
Member Author

/retest

@ehashman
Copy link
Member

No code changes, just updating API and feature gates.

/assign @derekwaynecarr
where did we land on TLS support for beta? kubernetes/enhancements#3177 (comment)

@SergeyKanzhelev
Copy link
Member Author

where did we land on TLS support for beta? kubernetes/enhancements#3177 (comment)

I replied on the comment. We do support TLS, but do not check the certificate. Same as HTTP.

@wgahnagl
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2022
@SergeyKanzhelev
Copy link
Member Author

@liggitt can you please approve for API

@liggitt
Copy link
Member

liggitt commented Mar 23, 2022

/approve
for API changes

node approval at #108522 (review)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mrunalp, SergeyKanzhelev

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2022
@ehashman
Copy link
Member

/milestone v1.24

@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Mar 23, 2022
@k8s-ci-robot k8s-ci-robot merged commit 8168c68 into kubernetes:master Mar 24, 2022
@SergeyKanzhelev SergeyKanzhelev deleted the grpcToBeta branch January 23, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-generation area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Status: API review completed, 1.24
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.