Skip to content

Fix winkernel proxy regression failing to query v1 endpoints created by dockershim CNI#110592

Closed
daschott wants to merge 1 commit into
kubernetes:release-1.22from
daschott:daschott/winkernel_fix_dockershim_v1.22
Closed

Fix winkernel proxy regression failing to query v1 endpoints created by dockershim CNI#110592
daschott wants to merge 1 commit into
kubernetes:release-1.22from
daschott:daschott/winkernel_fix_dockershim_v1.22

Conversation

@daschott

@daschott daschott commented Jun 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

What happened?

#109985 has introduced a regression for Kubernetes clusters using dockershim as the runtime that invoke CNIs using the v1 HNS APIs. Winkernel proxier is unable to retrieve endpoints due to this hcsshim call failing to retrieve v1 HNS endpoints: https://pkg.go.dev/github.com/Microsoft/hcsshim@v0.8.22/hcn#ListEndpointsOfNetwork

This causes service proxy rules to not be created, as local endpoints would not be found.

Clusters using the containerD runtime and CNIs that leverage HCN APIs are not impacted.

Source:
We have noticed this is failing on our dockershim related tests: https://testgrid.k8s.io/sig-windows-1.22-release#aks-engine-windows-dockershim-1.22

https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/ci-kubernetes-e2e-aks-engine-azure-1-22-windows/1536521484288135168

What did you expect to happen?

Service proxy rules are created for all expected services

How can we reproduce it (as minimally and precisely as possible)?

Create a Kubernetes cluster with Windows and dockershim runtime

Anything else we need to know?

This issue does not occur on clusters with the containerD runtime (that use v2 CNI workflow creating HCN endpoints instead of HNS endpoints)

Kubernetes version

1.23, 1.22

Cloud provider

Reproduced on Azure, but all are impacted.

OS version

Windows Server 2019

Install tools

Details

Container runtime (CRI) and version (if applicable)

Dockershim

Related plugins (CNI, CSI, ...) and versions (if applicable)

Azure CNI + dockershim. Any CNI creating v1 HNS endpoints will be impacted.

@k8s-ci-robot

k8s-ci-robot commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

@daschott: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

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.

@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jun 15, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. label Jun 15, 2022
@k8s-ci-robot

k8s-ci-robot commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

@daschott: This cherry pick PR is for a release branch and has not yet been approved by Release Managers.
Adding the do-not-merge/cherry-pick-not-approved label.

To merge this cherry pick, it must first be approved (/lgtm + /approve) by the relevant OWNERS.

AFTER it has been approved by code owners, please leave the following comment on a line by itself, with no leading whitespace: /cc kubernetes/release-managers

(This command will request a cherry pick review from Release Managers and should work for all GitHub users, whether they are members of the Kubernetes GitHub organization or not.)

For details on the patch release process and schedule, see the Patch Releases page.

Details

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.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. 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. labels Jun 15, 2022
@k8s-ci-robot

k8s-ci-robot commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

@daschott: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Details

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.

@k8s-ci-robot

k8s-ci-robot commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

Hi @daschott. Thanks for your PR.

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

Details

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 15, 2022
@daschott daschott mentioned this pull request Jun 15, 2022
Merged
@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 15, 2022
@daschott

daschott commented Jun 15, 2022

Copy link
Copy Markdown
Contributor Author

/sig windows

@k8s-ci-robot k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Jun 15, 2022
@daschott

daschott commented Jun 15, 2022

Copy link
Copy Markdown
Contributor Author

/test pull-kubernetes-e2e-aks-engine-windows-containerd-1-22

@k8s-ci-robot

k8s-ci-robot commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

@daschott: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/test pull-kubernetes-e2e-aks-engine-windows-containerd-1-22

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.

@ameukam

ameukam commented Jun 15, 2022

Copy link
Copy Markdown
Member

/ok-to-test

@k8s-ci-robot 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 Jun 15, 2022
@sbangari

sbangari commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2022
@k8s-ci-robot

k8s-ci-robot commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daschott, sbangari

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 Jun 15, 2022
@daschott

daschott commented Jun 15, 2022

Copy link
Copy Markdown
Contributor Author

/test pull-kubernetes-e2e-aks-engine-windows-containerd-1-22

@jsturtevant

jsturtevant commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

/test pull-kubernetes-e2e-aks-engine-windows-dockershim-1-22

@daschott daschott mentioned this pull request Jun 15, 2022
Closed
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2022
@k8s-ci-robot

k8s-ci-robot commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

@daschott: PR needs rebase.

Details

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.

@daschott

daschott commented Jun 21, 2022

Copy link
Copy Markdown
Contributor Author

Due to revert, closing in favor of #110701

@daschott daschott closed this Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ksubrmnn ksubrmnn Awaiting requested review from ksubrmnn

@kumarvin123 kumarvin123 Awaiting requested review from kumarvin123

Assignees

@sbangari sbangari

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

v1.22

Development

Successfully merging this pull request may close these issues.

6 participants

@daschott @k8s-ci-robot @ameukam @sbangari @jsturtevant @marosset