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

Fix IsVolumeFormatted issue on checking volume type "Unknown" #127

Merged
merged 1 commit into from
May 9, 2021

Conversation

jingxu97
Copy link
Contributor

@jingxu97 jingxu97 commented Apr 14, 2021

This PR fixes issue in IsVolumeFormatted on checking volume type
"Unknown"

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. 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. labels Apr 14, 2021
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 14, 2021
@wongma7
Copy link
Contributor

wongma7 commented Apr 15, 2021

thx so much for quick fix, I just tested this patch and can confirm it works for me. I added a little log statement to verify that my "raw" volumes are showing up with Unknown FS for me and matching the new condition:

W0415 19:13:08.177499 10684 api.go:82] STRING "Unknown"

diff --git a/internal/os/volume/api.go b/internal/os/volume/api.go
index 74f0401..afdcf3b 100644
--- a/internal/os/volume/api.go
+++ b/internal/os/volume/api.go
@@ -7,6 +7,8 @@ import (
        "regexp"
        "strconv"
        "strings"
+
+       "k8s.io/klog/v2"
 )
 
 const formatFilesystem = "ntfs"
@@ -76,7 +78,9 @@ func (VolAPIImplementor) IsVolumeFormatted(volumeID string) (bool, error) {
        if err != nil {
                return false, fmt.Errorf("error checking if volume is formatted. cmd: %s, output: %s, error: %v", cmd, string(out), err)
        }
-       if len(strings.TrimSpace(string(out))) == 0 {
+       stringOut := strings.TrimSpace(string(out))
+       klog.Warningf("STRING %q", stringOut)
+       if len(stringOut) == 0 || strings.EqualFold(stringOut, "Unknown") {
                return false, nil
        }
        return true, nil

@wongma7
Copy link
Contributor

wongma7 commented Apr 15, 2021

/lgtm

@k8s-ci-robot
Copy link
Contributor

@wongma7: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

manueltellez pushed a commit to manueltellez/csi-proxy that referenced this pull request Apr 19, 2021
1d60e77 Merge pull request kubernetes-csi#131 from pohly/kubernetes-1.20-tag
9f10459 prow.sh: support building Kubernetes for a specific version
fe1f284 Merge pull request kubernetes-csi#121 from kvaps/namespace-check
8fdf0f7 Merge pull request kubernetes-csi#128 from fengzixu/master
1c94220 fix: fix a bug of csi-sanity
a4c41e6 Merge pull request kubernetes-csi#127 from pohly/fix-boilerplate
ece0f50 check namespace for snapshot-controller
dbd8967 verify-boilerplate.sh: fix path to script
9289fd1 Merge pull request kubernetes-csi#125 from sachinkumarsingh092/optional-spelling-boilerplate-checks
ad29307 Make the spelling and boilerplate checks optional
5f06d02 Merge pull request kubernetes-csi#124 from sachinkumarsingh092/fix-spellcheck-boilerplate-tests
48186eb Fix spelling and boilerplate errors
71690af Merge pull request kubernetes-csi#122 from sachinkumarsingh092/include-spellcheck-boilerplate-tests
981be3f Adding spelling and boilerplate checks.
2bb7525 Merge pull request kubernetes-csi#117 from fengzixu/master
3b6d17b Merge pull request kubernetes-csi#118 from pohly/cloud-build-timeout
9318c6c cloud build: double the timeout, now 1 hour
4ab8b15 use the tag to replace commit of csi-test
5d74e45 change the csi-test import path to v4
7dcd0a9 upgrade csi-test to v4.0.2
86ff580 Merge pull request kubernetes-csi#116 from andyzhangx/export-image-name
c3a9662 allow export image name and registry name
c6a88c6 Merge pull request kubernetes-csi#113 from xing-yang/install_snapshot_controller
45ec4c6 Fix the install of snapshot CRDs and controller
5d874cc Merge pull request kubernetes-csi#112 from xing-yang/cleanup
79bbca7 Cleanup
d437673 Merge pull request kubernetes-csi#111 from xing-yang/update_snapshot_v1_rc
57718f8 Update snapshot CRD version

git-subtree-dir: release-tools
git-subtree-split: 1d60e7792624a9938c0bd1b045211fbb89e513d6
@jingxu97
Copy link
Contributor Author

/lgtm

@k8s-ci-robot
Copy link
Contributor

@jingxu97: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

@jingxu97
Copy link
Contributor Author

/assign @ddebroy

@ddebroy
Copy link
Contributor

ddebroy commented May 4, 2021

/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 May 4, 2021
This PR fixes issue in IsVolumeFormatted on checking volume type
"Unknown"
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. labels May 5, 2021
@jingxu97
Copy link
Contributor Author

jingxu97 commented May 5, 2021

/lgtm

@k8s-ci-robot
Copy link
Contributor

@jingxu97: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, ddebroy, jingxu97

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:
  • OWNERS [andyzhangx,ddebroy,jingxu97]

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 merged commit f010bac into kubernetes-csi:master May 9, 2021
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants