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

Fix the bug by passing fstype correctly #335

Merged
merged 1 commit into from
Aug 15, 2019
Merged

Fix the bug by passing fstype correctly #335

merged 1 commit into from
Aug 15, 2019

Conversation

leakingtapan
Copy link
Contributor

@leakingtapan leakingtapan commented Aug 11, 2019 •
edited
Loading

Is this a bug fix or adding new feature?
Fixes: #324 #326

/cc @wongma7 @jsafrane @shanesiebken

What is this PR about? / Why do we need it?
This fixes a bug to use fstype inside volume capacity for NodeStageVolume and NodePublishVolume call. So that pre-created persistence volume's fstype is used when mounting the volume correctly.

This change also removed the usage of fstype in driver's storage class parameter in favor of reserved csi.storage.k8s.io/fstype parameter.

What testing is done?
Tested on Kops cluster with a pre-created xfs EBS volume and consume it with fstype set in the PV spec.

@k8s-ci-robot k8s-ci-robot requested a review from wongma7 August 11, 2019 18:42
@k8s-ci-robot
Copy link
Contributor

@leakingtapan: GitHub didn't allow me to request PR reviews from the following users: shanesiebken.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Is this a bug fix or adding new feature?
Fixes: #324 #326

/cc @wongma7 @jsafrane @shanesiebken

What is this PR about? / Why do we need it?

What testing is done?
Tested on Kops cluster with a pre-created xfs EBS volume and consume it with fstype set in the PV spec.

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 requested a review from jsafrane August 11, 2019 18:42
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 11, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leakingtapan

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 11, 2019
@leakingtapan leakingtapan changed the title Fstype Fix the bug to pass fstype correctly Aug 11, 2019
@leakingtapan leakingtapan changed the title Fix the bug to pass fstype correctly Fix the bug by passing fstype correctly Aug 11, 2019
@coveralls
Copy link

coveralls commented Aug 11, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 750

  • 14 of 18 (77.78%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 71.1%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/driver/controller.go 1 3 33.33%
pkg/driver/node.go 13 15 86.67%
Totals Coverage Status
Change from base Build 744: -0.2%
Covered Lines: 1176
Relevant Lines: 1654
💛 - Coveralls

@leakingtapan leakingtapan force-pushed the fstype branch 2 times, most recently from 90ec455 to a4e37c0 Compare August 11, 2019 22:32
@leakingtapan
Copy link
Contributor Author

/retest

jsafrane
jsafrane reviewed Aug 12, 2019
View reviewed changes
pkg/driver/node.go Outdated Show resolved Hide resolved
@jsafrane
Copy link
Contributor

/retest

@jsafrane
Copy link
Contributor

/lgtm

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

wongma7 commented Aug 13, 2019

something wrong with coveralls https://status.coveralls.io/ we use in travis

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 13, 2019
@leakingtapan
Copy link
Contributor Author

/retest

@leakingtapan
Copy link
Contributor Author

I notice removing the fstype in the driver could cause CreateVolume to fail since this key becomes unknown to the driver. Adding the key back with deprecation message.

@wongma7 wongma7 mentioned this pull request Aug 14, 2019
Merged
Fix bugs by passing fstype correctly:
1848e8b 
1. Fix fstype parameter prefix
2. Remove fstype in storageclass in favor of csi.storage.k8s.io/fstype
3. Update e2e test
4. Add static provisioning example
5. Update run-e2e-test script to build driver before create cluster
wongma7
wongma7 reviewed Aug 14, 2019
View reviewed changes
pkg/driver/controller.go
VolumeContext: map[string]string{
FsTypeKey: disk.FsType,
},
VolumeContext: map[string]string{},
Copy link
Contributor

Choose a reason for hiding this comment

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

OK: external-provisioner will fill in volume.spec.csi.fstype for us

@wongma7
Copy link
Contributor

wongma7 commented Aug 14, 2019

/lgtm

re-adding

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

merged manually since the coverage drop is due to the deprecation message added

@leakingtapan leakingtapan merged commit 2deb7c9 into kubernetes-sigs:master Aug 15, 2019
@leakingtapan leakingtapan deleted the fstype branch August 15, 2019 00:24
@leakingtapan leakingtapan mentioned this pull request Nov 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wongma7 wongma7 wongma7 left review comments

@jsafrane jsafrane jsafrane left review comments

Assignees

@jsafrane jsafrane

@wongma7 wongma7

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.

EBS CSI Driver doesn't respect fstype from existing storage
5 participants
@leakingtapan @k8s-ci-robot @coveralls @jsafrane @wongma7