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

CON-9040 driver can accidentally format existing volume #478

Merged

Conversation

llDrLove
Copy link
Contributor

@llDrLove llDrLove commented Mar 1, 2023

There is a race condition where DOBS volumes look to our indication method as if they'd fully be attached, but aren't completely.
The result can be a misinterpretation by CSI which causes a force-format of the volume.

This PR uses a new method to check the attachment of the volume by looking at the running state in the /sys/class/block/<device name like sda>/device/state file. This can be enabled using the new flag --validate-attachment=true flag.

@llDrLove llDrLove added the WIP label Mar 1, 2023
@llDrLove llDrLove self-assigned this Mar 1, 2023
Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

I forgot to mention the following: it'd be great if we could introduce a CLI flag that controls whether the new check is being used. That way, we can turn it off easily should anything go wrong or not work out the way we want it to be.

driver/mounter.go Outdated Show resolved Hide resolved
driver/mounter.go Outdated Show resolved Hide resolved
driver/mounter.go Outdated Show resolved Hide resolved
driver/mounter.go Outdated Show resolved Hide resolved
driver/mounter.go Outdated Show resolved Hide resolved
driver/mounter.go Outdated Show resolved Hide resolved
driver/mounter.go Outdated Show resolved Hide resolved
driver/mounter.go Outdated Show resolved Hide resolved
driver/mounter_mock.go Outdated Show resolved Hide resolved
driver/mounter_test.go Outdated Show resolved Hide resolved
driver/mounter_test.go Outdated Show resolved Hide resolved
cmd/do-csi-plugin/main.go Outdated Show resolved Hide resolved
cmd/do-csi-plugin/main.go Show resolved Hide resolved
@timoreimann
Copy link
Contributor

Let's also make sure to document the new flag in the README.

driver/mounter.go Outdated Show resolved Hide resolved
driver/mounter.go Outdated Show resolved Hide resolved
driver/mounter_test.go Outdated Show resolved Hide resolved
driver/mounter_test.go Outdated Show resolved Hide resolved
driver/mounter_test.go Outdated Show resolved Hide resolved
driver/mounter_test.go Outdated Show resolved Hide resolved
driver/mounter_test.go Outdated Show resolved Hide resolved
driver/node.go Outdated Show resolved Hide resolved
driver/mounter.go Outdated Show resolved Hide resolved
driver/mounter.go Show resolved Hide resolved
@timoreimann
Copy link
Contributor

Forgot to mention: we can already extend the CHANGELOG file for this feature.

@llDrLove llDrLove requested a review from timoreimann March 3, 2023 18:43
@llDrLove llDrLove marked this pull request as ready for review March 3, 2023 18:43
@llDrLove llDrLove removed the WIP label Mar 3, 2023
Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

Last round. :)

CHANGELOG.md Outdated
@@ -1,5 +1,9 @@
## unreleased

## v4.5.1 - 2023.03.03
Copy link
Contributor

Choose a reason for hiding this comment

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

Please omit the header -- that's going to be added at the time we do the release for sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the version header.

return false, fmt.Errorf("error comparing the state file content, expected: %s, got: %s", runningState, string(deviceStateFileContent))
}

return true, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point, we only return true when err == nil, which means we could drop the boolean return value and rely on the error return value exclusively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the boolean return and only kept the error. I've refactored the interface method with the new structure, refactored the implementation and the tests.

name string
av AttachmentValidator
errorMsg string
wantErr bool
Copy link
Contributor

Choose a reason for hiding this comment

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

wantErr is not really needed since it derives from errorMsg != "".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the wantErr and want properties and now I'm only relying on errorMsg.

if tt.wantErr {
assert.ErrorContains(t, err, tt.errorMsg)
} else {
assert.Equal(t, tt.want, got)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit / personal note: Go stdlib testing prefers outputting got first, then want. I'd match the order here given how thin of a layer gotest.tools is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored the test so now it uses the got first and want after.

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a ton Oliver. 👍

Please squash-merge whenever you're ready.

@llDrLove llDrLove merged commit 7fb9270 into master Mar 7, 2023
@llDrLove llDrLove deleted the CON-9040-driver-can-accidentally-format-existing-volume branch March 7, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants