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

Add support for AWS EBS on windows #79552

Merged
merged 2 commits into from
Jul 24, 2019
Merged

Add support for AWS EBS on windows #79552

merged 2 commits into from
Jul 24, 2019

Conversation

wongma7
Copy link
Contributor

@wongma7 wongma7 commented Jun 29, 2019 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it: Currently, EBS WaitForAttach on windows nodes fails because the EBS driver looks for the device at some Linux-specific /dev/xxxxx path which doesn't exist on Windows. Nothing prevents the scheduling of windows pods using EBS to windows nodes, so the pods end up stuck pending. Windows nodes are already GA, so fix it.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
TODO: add Get-Disk fallback or maybe not.
TODO: e2e test?

Does this PR introduce a user-facing change?:

Add support for AWS EBS on windows

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 29, 2019
@wongma7
Copy link
Contributor Author

wongma7 commented Jun 29, 2019

/sig aws
/sig storage

@k8s-ci-robot k8s-ci-robot added sig/aws sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 29, 2019
@k8s-ci-robot k8s-ci-robot requested review from justinsb and rootfs June 29, 2019 00:57
@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jun 29, 2019
@wongma7
Copy link
Contributor Author

wongma7 commented Jun 29, 2019

/sig windows

@k8s-ci-robot k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Jun 29, 2019
leakingtapan
leakingtapan reviewed Jul 1, 2019
View reviewed changes
pkg/volume/awsebs/attacher.go
dir := deviceMountPath
if runtime.GOOS == "windows" {
// FormatAndMount will mklink here, avoid "Cannot create a file when that file already exists"
dir = filepath.Dir(deviceMountPath)
Copy link

Choose a reason for hiding this comment

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

Im still a bit confused why this is needed, could you add some more details? The same to the later change at SetUpAt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, lemme know if it makes sense now.

Copy link
Member

Choose a reason for hiding this comment

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

that's the correct behavior, I did the same implementation on other place for windows too.

@wongma7 wongma7 force-pushed the windows-ebs branch 2 times, most recently from 84fbf82 to 623f7fd Compare July 2, 2019 21:26
@wongma7 wongma7 changed the title WIP: Add support for AWS EBS on windows Add support for AWS EBS on windows Jul 2, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 2, 2019
@wongma7
Copy link
Contributor Author

wongma7 commented Jul 2, 2019

This is ready for review. I turned on e2e tests in the 2nd commit, I ran DynamicPV ones locally and they passed. Also tested the patch manually.

@wongma7
Copy link
Contributor Author

wongma7 commented Jul 2, 2019

/test pull-kubernetes-e2e-gce-100-performance

1 similar comment
@wongma7
Copy link
Contributor Author

wongma7 commented Jul 8, 2019

/test pull-kubernetes-e2e-gce-100-performance

@PatrickLang
Copy link
Contributor

also related kubernetes/enhancements#1122 https://github.com/kubernetes/enhancements/pulls/1141

Can you take a look at that? It would be good to make sure this can feasibly be migrated out of tree later

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 17, 2019
@wongma7 wongma7 force-pushed the windows-ebs branch 2 times, most recently from fb57e4c to c397e1d Compare July 17, 2019 20:24
@leakingtapan
Copy link

/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 Jul 22, 2019
@wongma7
Copy link
Contributor Author

wongma7 commented Jul 22, 2019

/assign @gnufied
Please approve, TY

gnufied
gnufied reviewed Jul 23, 2019
View reviewed changes
Copy link
Member

@gnufied gnufied left a comment

Choose a reason for hiding this comment

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

Mostly looks good. some tests would be nice.

pkg/volume/awsebs/attacher_windows.go Outdated Show resolved Hide resolved
pkg/volume/awsebs/attacher_windows.go
return "", fmt.Errorf("disk not found in ebsnvme-id.exe output: %q", string(output))
}
return matches[1], nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need some tests for this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't think it would be too useful, I would basically be testing the regex expression, & the only way to verify it is correct is to test against actual (not copy/pasted) ebsnvme-id.exe output. if e.g. the format of ebsnvme-id.exe changes someday, e2e will catch it & error will propagate.

pkg/volume/awsebs/attacher_windows.go
diskRe := regexp.MustCompile(
`Disk Number: (\d+)\s*` +
`Volume ID: ` + volumeID + `\s*`)
matches := diskRe.FindStringSubmatch(string(output))
Copy link
Member

Choose a reason for hiding this comment

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

Is the output a list with all devices present? i.e:

Disk Number : 1
Volume ID: ...
Device Name: ....
Disk Number: 2
Volume ID: ...
Device Name: ....

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 23, 2019
@gnufied
Copy link
Member

gnufied commented Jul 23, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 23, 2019
@wongma7
Copy link
Contributor Author

wongma7 commented Jul 23, 2019

/retest

@wongma7
Copy link
Contributor Author

wongma7 commented Jul 23, 2019

@gnufied require your /approve as well pls . Thanks. 👍

@gnufied
Copy link
Member

gnufied commented Jul 23, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, leakingtapan, wongma7

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 Jul 23, 2019
@k8s-ci-robot k8s-ci-robot merged commit 60c2d44 into kubernetes:master Jul 24, 2019
This was referenced Jul 24, 2019
Closed
Closed
@gab-satchi gab-satchi mentioned this pull request Aug 2, 2019
Merged
@leakingtapan leakingtapan mentioned this pull request Sep 11, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PatrickLang PatrickLang PatrickLang left review comments

@leakingtapan leakingtapan leakingtapan left review comments

@andyzhangx andyzhangx andyzhangx left review comments

@gnufied gnufied gnufied left review comments

@justinsb justinsb Awaiting requested review from justinsb

@rootfs rootfs Awaiting requested review from rootfs

Assignees

@gnufied gnufied

@leakingtapan leakingtapan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wongma7 @PatrickLang @leakingtapan @gnufied @k8s-ci-robot @andyzhangx