Skip to content

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

Closed
daschott wants to merge 1 commit into
kubernetes:release-1.23from
daschott:daschott/winkernel_fix_dockershim
Closed

Fix winkernel proxy regression failing to query v1 endpoints created by dockershim CNI#110590
daschott wants to merge 1 commit into
kubernetes:release-1.23from
daschott:daschott/winkernel_fix_dockershim

Conversation

@daschott

@daschott daschott commented Jun 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

#109981 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. This PR removes the failing call so that dockershim clusters using CNIs that create v1 HNS endpoints are no longer broken.

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

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

What type of PR is this?

/kind bug
/kind regression

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #110591

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Jun 15, 2022
@k8s-ci-robot k8s-ci-robot added do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels 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 kind/regression Categorizes issue or PR as related to a regression from a prior release. 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. 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 added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 15, 2022
@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 the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jun 15, 2022
@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
This was referenced Jun 15, 2022
Merged
Merged
@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-23

@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-23

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 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 lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 15, 2022
@daschott

daschott commented Jun 15, 2022

Copy link
Copy Markdown
Contributor Author

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

@jsturtevant

jsturtevant commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

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

@jsturtevant

jsturtevant commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

dockershim can not run the failed tests kubernetes/test-infra#26586 fixes this
/test pull-kubernetes-e2e-aks-engine-windows-dockershim-1-23

@daschott daschott mentioned this pull request Jun 15, 2022
Closed
@jsturtevant jsturtevant mentioned this pull request Jun 15, 2022
Closed
@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.

@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
@daschott

daschott commented Jun 21, 2022

Copy link
Copy Markdown
Contributor Author

Due to revert, closing in favor of #110702

@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

@feiskyer feiskyer Awaiting requested review from feiskyer

@sbangari sbangari Awaiting requested review from sbangari

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. kind/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. 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. release-note-none Denotes a PR that doesn't merit a release note. 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.23

Development

Successfully merging this pull request may close these issues.

6 participants

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