Flexvolume dynamic plugin discovery: Prober unit tests and basic e2e test. #51474

verult · 2017-08-28T20:51:30Z

What this PR does / why we need it: Tests for changes introduced in PR #50031 .
As part of the prober unit test, I mocked filesystem, filesystem watch, and Flexvolume plugin initialization.
Moved the filesystem event goroutine to watcher implementation.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #51147

Special notes for your reviewer:
First commit contains added functionality of the mock filesystem.
Second commit is the refactor for moving mock filesystem into a common util directory.
Third commit is the unit and e2e tests.

Release note:

NONE

/release-note-none
/sig storage
/assign @saad-ali @liggitt
/cc @mtaufen @chakri-nelluri @wongma7

wongma7

e2e test lgtm

chakri-nelluri · 2017-08-29T17:13:53Z

pkg/volume/flexvolume/watcher.go

@@ -0,0 +1,69 @@
+/*
+Copyright 2017 The Kubernetes Authors.


Move this to util/filesystem?

chakri-nelluri · 2017-08-29T17:19:45Z

pkg/volume/flexvolume/probe.go

-			case err := <-prober.watcher.Errors:
-				glog.Errorf("Received an error from watcher: %s", err)
-			}
+	prober.watcher.Init(func(event fsnotify.Event) {


Move this block to initWatcher?

chakri-nelluri · 2017-08-29T17:21:27Z

pkg/volume/flexvolume/probe.go

 }

 // Creates a new filesystem watcher and adds watches for the plugin directory
 // and all of its subdirectories.
 func (prober *flexVolumeProber) initWatcher() error {
 	var err error
-	if prober.watcher, err = fsnotify.NewWatcher(); err != nil {
-		return fmt.Errorf("Error creating new watcher: %s", err)
+	if prober.watcher == nil {


Do we need the nil check? Is initWatcher called more than once?

The mock watcher is injected by creating an instance of flexVolumeProber with watcher already assigned. The nil check is to make sure the mock is not overridden.

Ideally code shouldn't know whether or not it's working with a mock, doing so breaks the abstraction.
Can we always assign the watcher to the flexVolumeProber before we get here?

We can initialize the watcher in GetDynamicPluginProber() (the prober constructor, essentially), but it would have to handle the possibility of an error somehow. Returning the error from a constructor seems strange, and we shouldn't handle the error here. IMO it's better to call it in Init().

Or, it gets passed into the constructor as a dependency. I don't think the caller should worry about the details of underlying watcher implementation, especially when most of the time it's the default watcher anyway.

From the perspective of this code, it's doing a safety check to make sure watcher is always initialized. There isn't any code that depends on the watcher being a mock or not.

It's fine to return an error from the constructor, IMO.

chakri-nelluri · 2017-08-29T17:22:15Z

pkg/volume/flexvolume/probe.go

@@ -213,8 +209,8 @@ func (prober *flexVolumeProber) initWatcher() error {

 // Creates the plugin directory, if it doesn't already exist.
 func (prober *flexVolumeProber) createPluginDir() error {
-	if _, err := os.Stat(prober.pluginDir); os.IsNotExist(err) {
-		err := os.MkdirAll(prober.pluginDir, 0755)
+	if _, err := prober.fs.Stat(prober.pluginDir); os.IsNotExist(err) {


Log a warning & event from here that Flex volume plugin directory is not present. This is not supposed to happen.

@derekwaynecarr is this an appropriate place for an event or should they just log an error? IIRC events are really more appropriate for alerting on state changes, and it's probably better to just log an error here. Though I suppose you could consider "Recreating" a state change...

chakri-nelluri · 2017-08-29T17:23:19Z

pkg/volume/flexvolume/probe_test.go

+	"fmt"
+	"github.com/fsnotify/fsnotify"
+	"github.com/stretchr/testify/assert"
+	utilfs "k8s.io/kubernetes/pkg/util/filesystem"


Fix package order

chakri-nelluri · 2017-08-29T17:30:38Z

pkg/volume/flexvolume/probe_test.go

+}
+
+// Mock filesystem watcher
+type mockWatcher struct {


Move this to separate file?

+1. I'd also rename it to fakeWatcher, which is more conventional in our codebase

Keeping the file inside Flexvolume directory since fakes may look different depending on test.

chakri-nelluri · 2017-08-29T17:32:18Z

pkg/util/filesystem/fakefs.go

@@ -21,6 +21,7 @@ import (
 	"time"

 	"github.com/spf13/afero"
+	"path/filepath"


Fix package order

verult · 2017-08-29T23:55:14Z

fixes #51570

mtaufen · 2017-08-30T19:50:30Z

pkg/util/filesystem/defaultfs.go

 func (DefaultFs) ReadFile(filename string) ([]byte, error) {
 	return ioutil.ReadFile(filename)
 }

-// TempFile via os.TempFile
+// TempFile via ioutil.TempFile


mtaufen · 2017-08-30T19:51:24Z

pkg/util/filesystem/fakefs.go

@@ -62,12 +63,12 @@ func (fs *fakeFs) Chtimes(name string, atime time.Time, mtime time.Time) error {
 	return fs.a.Fs.Chtimes(name, atime, mtime)
 }

-// ReadFile via afero.Fs.ReadFile
+// ReadFile via afero.ReadFile


mtaufen · 2017-08-30T19:51:27Z

pkg/util/filesystem/fakefs.go

 func (fs *fakeFs) ReadFile(filename string) ([]byte, error) {
 	return fs.a.ReadFile(filename)
 }

-// TempFile via afero.Fs.TempFile
+// TempFile via afero.TempFile


mtaufen · 2017-08-30T21:57:35Z

pkg/util/filesystem/watcher.go

+	"github.com/fsnotify/fsnotify"
+)
+
+// A callback-based filesystem watcher abstraction for fsnotify.


Comment should look like "FSWatcher..."

mtaufen · 2017-08-30T21:57:48Z

pkg/util/filesystem/watcher.go

+	// Add a filesystem path to watch
+	AddWatch(path string) error
+}
+type FSEventHandler func(event fsnotify.Event)


Please add a godoc comment

mtaufen · 2017-08-30T22:42:57Z

pkg/volume/flexvolume/probe_test.go

+func TestProberMultipleEvents(t *testing.T) {
+	// Arrange
+	_, fs, watcher, prober := initTestEnvironment(t)
+	for i := 0; i < 5; i++ {


put the number of iterations in a variable so you can adjust more easily/calculate the number for your assertions w/o hardcoding

mtaufen · 2017-08-30T22:47:45Z

pkg/volume/flexvolume/probe_test.go

+}
+
+// Mock filesystem watcher
+type mockWatcher struct {


+1. I'd also rename it to fakeWatcher, which is more conventional in our codebase

mtaufen · 2017-08-30T22:49:50Z

pkg/volume/flexvolume/probe_test.go

+}
+
+// Mock Flexvolume plugin
+type mockPluginFactory struct {


Rename this to fakePluginFactory and move to a separate file. We typically put the fakes in the same package as the real ones, but in a different file.

mtaufen · 2017-08-30T22:55:03Z

test/e2e/storage/flexvolume.go

@@ -170,7 +174,7 @@ var _ = SIGDescribe("Flexvolumes [Disruptive] [Feature:FlexVolume]", func() {
 		driverInstallAs := driver + "-" + suffix

 		By(fmt.Sprintf("installing flexvolume %s on node %s as %s", path.Join(driverDir, driver), node.Name, driverInstallAs))
-		installFlex(&node, "k8s", driverInstallAs, path.Join(driverDir, driver))
+		installFlex(&node, "k8s", driverInstallAs, path.Join(driverDir, driver), true /* restart */)


Not sure I'm a fan of comments interleaved with code in the same line.

The comment is mainly to clarify the parameter name, and is used in several places in the code base AFAIK.

mtaufen · 2017-08-30T22:58:19Z

test/e2e/storage/flexvolume.go

+		driverInstallAs := driver + "-" + suffix
+
+		By(fmt.Sprintf("installing flexvolume %s on node %s as %s", path.Join(driverDir, driver), node.Name, driverInstallAs))
+		installFlex(&node, "k8s", driverInstallAs, path.Join(driverDir, driver), false /* restart */)


Again not sure I like the interleaved comments. And this one should probably read "skip restart", in any case.

…olume prober unit tests.

chakri-nelluri

One minor comment. Other than that looks good.

chakri-nelluri · 2017-09-01T15:23:24Z

pkg/volume/flexvolume/fake_watcher.go

@@ -0,0 +1,53 @@
+/*


Move this to util too?

I decided to leave it in Flexvolume directory because fakes could be different depending on test.

saad-ali · 2017-09-01T18:17:30Z

/lgtm
/approve

saad-ali · 2017-09-01T18:19:30Z

/test pull-kubernetes-e2e-gce-etcd3

chakri-nelluri · 2017-09-01T19:16:12Z

Thanks @verult.
/lgtm

chakri-nelluri · 2017-09-01T19:17:01Z

/approve

thockin · 2017-09-01T20:21:49Z

/approve

k8s-github-robot · 2017-09-01T20:22:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chakri-nelluri, saad-ali, thockin, verult

Associated issue: 50031

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/OWNERS~~ [thockin]
~~test/e2e/storage/OWNERS~~ [saad-ali,thockin]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-09-03T18:10:04Z

Automatic merge from submit-queue (batch tested with PRs 51805, 51725, 50925, 51474, 51638)

Moving filesystem mock to pkg/util, and added some functionality

fde9541

k8s-ci-robot assigned liggitt and saad-ali Aug 28, 2017

k8s-ci-robot requested review from chakri-nelluri, mtaufen and wongma7 August 28, 2017 20:51

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/storage Categorizes an issue or PR as relevant to SIG Storage. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 28, 2017

k8s-github-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 28, 2017

verult force-pushed the ProberTest branch 3 times, most recently from 6b0f5f4 to 43f9872 Compare August 28, 2017 21:50

Refactoring for filesystem mock move

8618e28

verult force-pushed the ProberTest branch from 43f9872 to e724ffe Compare August 28, 2017 23:22

wongma7 reviewed Aug 29, 2017

View reviewed changes

chakri-nelluri reviewed Aug 29, 2017

View reviewed changes

verult force-pushed the ProberTest branch from e724ffe to dbba3cc Compare August 29, 2017 23:54

verult force-pushed the ProberTest branch 2 times, most recently from 061138e to 9545c15 Compare August 30, 2017 18:34

mtaufen reviewed Aug 30, 2017

View reviewed changes

verult force-pushed the ProberTest branch 3 times, most recently from 952439b to 096f6c7 Compare August 31, 2017 22:40

Added basic Flexvolume dynamic plugin discovery e2e test; added Flexv…

52a77aa

…olume prober unit tests.

verult force-pushed the ProberTest branch from 096f6c7 to 52a77aa Compare August 31, 2017 23:50

chakri-nelluri reviewed Sep 1, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017

k8s-ci-robot assigned chakri-nelluri Sep 1, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2017

abgworrall modified the milestone: v1.8 Sep 2, 2017

k8s-github-robot merged commit f07279a into kubernetes:master Sep 3, 2017

verult mentioned this pull request Jan 5, 2018

Default GetVolumeName() log formatting error #51570

Closed

verult deleted the ProberTest branch March 24, 2018 01:34

Flexvolume dynamic plugin discovery: Prober unit tests and basic e2e test. #51474

Flexvolume dynamic plugin discovery: Prober unit tests and basic e2e test. #51474

Conversation

verult commented Aug 28, 2017 • edited

wongma7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verult commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chakri-nelluri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Sep 1, 2017

saad-ali commented Sep 1, 2017

chakri-nelluri commented Sep 1, 2017

chakri-nelluri commented Sep 1, 2017

thockin commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

k8s-github-robot commented Sep 3, 2017

verult commented Aug 28, 2017 •

edited