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.

Sign up for GitHub

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

Jump to bottom

Check for all errors in CRI connection validation #115102

Merged
merged 1 commit into from Jan 24, 2023
Merged

Check for all errors in CRI connection validation #115102

merged 1 commit into from Jan 24, 2023

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Jan 16, 2023 •
edited

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

We only have one CRI API (v1) to validate during the initial connection of the kubelet with the container runtime. Therefore we can now verify all kind of GRPC related issues.

Which issue(s) this PR fixes:

Fixes #114956, kubernetes-sigs/cri-tools#1041

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fail CRI connection if service or image endpoint is throwing any error on kubelet startup.

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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. 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. labels Jan 16, 2023
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 16, 2023
bart0sh
bart0sh reviewed Jan 16, 2023
View reviewed changes
pkg/kubelet/cri/remote/remote_image.go Outdated
@@ -98,6 +98,8 @@ func (r *remoteImageService) validateServiceConnection(ctx context.Context, conn

} else if status.Code(err) == codes.Unimplemented {
return fmt.Errorf("CRI v1 image API is not implemented for endpoint %q: %w", endpoint, err)
} else if status.Code(err) == codes.Unavailable {
Copy link
Contributor

Choose a reason for hiding this comment

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

@saschagrunert Thank you for your PR!

Can you explain why do we check only for 2 codes? There are more than 2 defined in the grpc package.
And why this function returns nil on any errors except of these 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a very good point! We now only have one CRI API to test against, means we can check for any error during connection setup.

Changed as suggested.

@saschagrunert
Check for all errors in CRI connection validation
e89547d 
We only have one CRI API (v1) to validate during the initial connection
of the kubelet with the container runtime. Therefore we can now verify
all kind of GRPC related issues.

Fixes: kubernetes#114956

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 17, 2023
@saschagrunert saschagrunert changed the title Check for Unavailable in CRI connection validation Check for all errors in CRI connection validation Jan 17, 2023
@bart0sh
Copy link
Contributor

bart0sh commented Jan 17, 2023

/triage accepted
/priority important-soon
/lgtm
/assign
/assign @SergeyKanzhelev @derekwaynecarr @mrunalp @dchen1107

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jan 17, 2023
@k8s-ci-robot k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 17, 2023
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed 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. labels Jan 17, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6f7b2158ef2b36c0628cbe919841afe4f8d7a28b

@bart0sh bart0sh added this to Needs Approver in SIG Node PR Triage Jan 17, 2023
@bart0sh
Copy link
Contributor

bart0sh commented Jan 17, 2023

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 17, 2023
SergeyKanzhelev
SergeyKanzhelev approved these changes Jan 18, 2023
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

/lgtm
/assign @mrunalp

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrunalp, saschagrunert, 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 Jan 23, 2023
@mrunalp mrunalp moved this from Needs Approver to Done in SIG Node PR Triage Jan 23, 2023
@k8s-triage-robot
Copy link

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

2 similar comments
@k8s-triage-robot
Copy link

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

@k8s-triage-robot
Copy link

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

@saschagrunert
Copy link
Member Author

/retest

@saschagrunert
Copy link
Member Author

/test pull-kubernetes-verify

@k8s-ci-robot k8s-ci-robot merged commit a12dd41 into kubernetes:master Jan 24, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.27 milestone Jan 24, 2023
@saschagrunert saschagrunert deleted the cri-unavailable branch January 24, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bart0sh bart0sh bart0sh left review comments

@mrunalp mrunalp mrunalp approved these changes

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@matthyx matthyx Awaiting requested review from matthyx

Assignees

@mrunalp mrunalp

@derekwaynecarr derekwaynecarr

@dchen1107 dchen1107

@SergeyKanzhelev SergeyKanzhelev

@bart0sh bart0sh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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/node Categorizes an issue or PR as relevant to SIG Node. 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
SIG Node PR Triage
Done
Milestone
v1.27
Development

Successfully merging this pull request may close these issues.

The CRI v1 implementation of validateServiceConnection does not check for unavailable
8 participants
@saschagrunert @bart0sh @k8s-ci-robot @k8s-triage-robot @mrunalp @SergeyKanzhelev @derekwaynecarr @dchen1107