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

fixing issue where SMB share paths cannot resolve with CRI-containerD on Windows #96396

Merged

Conversation

marosset
Copy link
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Partially Fixes #94213

Special notes for your reviewer:

Here is the equivalent PR that went into csi-proxy: kubernetes-csi/csi-proxy#98

As mentioned in the comments the underlying issue here is the behavior of the EvalSymlinks call in golang on Windows.
This change is intended as a work-around until a version of containerD for Windows is built with a versin of golang containing the fix (golang/go#42096)

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 release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. 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 Nov 10, 2020
@marosset
Copy link
Contributor Author

/sig windows
/sig storage
/milestone v1.20
/cc @andyzhangx @jingxu97

@k8s-ci-robot k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Nov 10, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Nov 10, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Nov 10, 2020
@marosset
Copy link
Contributor Author

/test pull-kubernetes-e2e-aks-engine-azure-windows

@jingxu97
Copy link
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 Nov 10, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jingxu97, marosset

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 Nov 10, 2020
@marosset
Copy link
Contributor Author

/hold
I want to do a little bit more validation before this merges.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2020
@andyzhangx
Copy link
Member

/priority important-soon
/triage accepted

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 10, 2020
@marosset
Copy link
Contributor Author

/hold cancel

I tested these changes with a private kubelet running on nodes configured with both docker and containerd.
In both cases I was able to create PVCs using the kubernetes.io/azure-file storage class and successfully schedule containers that used those PVCs.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2020
@k8s-ci-robot k8s-ci-robot merged commit 1d4c0ad into kubernetes:master Nov 10, 2020
@marosset marosset deleted the windows-smb-mount-symlink-fix branch November 11, 2020 00:59
@andyzhangx
Copy link
Member

@marosset should we cherry-pick this PR to old releases? what's the minimum supported k8s version for containerd on Windows?

@marosset
Copy link
Contributor Author

@andyzhangx I was planning on backporting this to v1.19 this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. kind/bug Categorizes issue or PR as related to a bug. 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-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/windows Categorizes an issue or PR as relevant to SIG Windows. 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
None yet
Development

Successfully merging this pull request may close these issues.

SMB mount does not work on containerd on Windows
4 participants