Automatically create folder for KV cache #1757

obitech · 2019-06-20T21:01:45Z

What this PR does / why we need it:

Special notes for your reviewer:

NONE

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

obitech · 2019-06-20T21:02:54Z

@richardartoul

richardartoul · 2019-06-21T02:59:24Z

src/cluster/kv/etcd/store.go

@@ -616,7 +617,32 @@ func (c *client) writeCacheToFile() error {
 	return nil
 }

+func (c *client) createCacheDir() error {
+	path := path.Dir(c.opts.CacheFileFn()(c.opts.Prefix()))


I think os.MkdirAll is what you're looking for here.

This might be helpful as an example: https://github.com/m3db/m3/blob/master/src/dbnode/server/server.go#L188

Ah good point with os.MkdirAll, thanks for the hint.

I tried using lockfile.CreateAndAcquire but somehow the other tests are failing then:

--- FAIL: TestCache (0.70s) Error Trace: store_test.go:198 Error: Received unexpected error "grpc: the client connection is closing" --- FAIL: TestStaleDelete__FromGet (61.54s) Error Trace: store_test.go:832 Error: Should be true Messages: timed out waiting for client to read values from cache --- FAIL: TestStaleDelete__FromWatch (62.06s) Error Trace: store_test.go:917 Error: Should be true Messages: timed out waiting for client to read values from cache

I'll just leave it with os.MkDirAll for now, but happy to take advice on how to properly implement the fs lock, if required.

Sorry I wasn't suggesting that you use lockfile.CreateAndAcquire I was just pointing to its implementation which uses os.MkdirAll (and passes the right permissions as well)

richardartoul · 2019-06-21T03:00:22Z

src/cluster/kv/etcd/store_test.go

+	defer os.RemoveAll(tdir)
+	require.NoError(t, err)
+
+	cdir := path.Join(tdir, randSeq(10))


not sure why you need the randSeq when you're already using a new random test directory

I think when calling ioutil.TempDir, it will already create a random directory. So I'm using TempDir and then choose a random dir name cdir be created by NewStore into TempDir. Then I can test for the existence of cdir.

nit: this feels like an overkill, a const name would do here.

Ok I can just use a const. I was thinking that there might be a case where this test could be run in parallel on the same machine, which might lead to multiple tests trying to create the same directory. But maybe I was overthinking :)

richardartoul · 2019-06-21T03:00:45Z

@cw9 Mind taking a look at this? I know you had some experience with the caching

cw9 · 2019-06-21T14:37:39Z

src/cluster/kv/etcd/store_test.go

@@ -125,6 +126,28 @@ func TestNoCache(t *testing.T) {
 	require.Equal(t, 0, len(store.(*client).cacheUpdatedCh))
 }

+func TestCacheDirCreation(t *testing.T) {
+	ec, opts, closeFn := testStore(t)
+


move defer closeFn() here?

cw9 · 2019-06-21T14:40:16Z

src/cluster/kv/etcd/store_test.go

+	defer os.RemoveAll(tdir)
+	require.NoError(t, err)
+
+	cdir := path.Join(tdir, randSeq(10))


nit: this feels like an overkill, a const name would do here.

cw9 · 2019-06-21T14:40:42Z

src/cluster/kv/etcd/store_test.go

+	ec, opts, closeFn := testStore(t)
+
+	tdir, err := ioutil.TempDir("", "m3tests")
+	defer os.RemoveAll(tdir)


Should this defer be called after require.NoError?

cw9 · 2019-06-21T14:42:51Z

src/cluster/kv/etcd/store_test.go

+
+	cdir := path.Join(tdir, randSeq(10))
+	fname := path.Join(cdir, "mk3kv.json")
+	opts = opts.SetCacheFileFn(func(string) string {


Instead of this, can you test to make sure the dir you created is compatible with c.opts.Prefix()?, try some writes and then read from the cache maybe.

codecov · 2019-06-22T11:50:40Z

Codecov Report

Merging #1757 into master will decrease coverage by 1.3%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #1757      +/-   ##
=========================================
- Coverage    72.3%   70.9%    -1.4%     
=========================================
  Files         986      76     -910     
  Lines       83010   10472   -72538     
=========================================
- Hits        60028    7433   -52595     
+ Misses      19006    2536   -16470     
+ Partials     3976     503    -3473

Flag	Coverage Δ
#aggregator	`82.5% <ø> (ø)`	⬆️
#cluster	`?`
#collector	`32.2% <ø> (-31.5%)`	⬇️
#dbnode	`71.7% <ø> (-8.4%)`	⬇️
#m3em	`?`
#m3ninx	`?`
#m3nsch	`?`
#metrics	`?`
#msg	`?`
#query	`?`
#x	`?`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0811843...e95c6f7. Read the comment docs.

richardartoul · 2019-06-24T09:30:48Z

src/cluster/kv/etcd/store.go

+	path := path.Dir(c.opts.CacheFileFn()(c.opts.Prefix()))
+	if _, err := os.Stat(path); err != nil {
+		if os.IsNotExist(err) {
+			if err := os.MkdirAll(path, 0755); err != nil {


Sorry I was a little vague about this. I think if you use os.MkdirAll you can avoid the os.Stat and os.IsNotExist. If the path is already a directory then it will just return nil so I think the existing checks are redundant.

The permissions thing is gonna be kind of annoying :/

We actually flow the new directory permissions around the codebase as options, so if you take a look at server.go you'll see that when we call lockfile.CreateAndAcquire(lockPath, newDirectoryMode) (again, you dont need to use that function, just using it as an example that does something similar) the newDirectoryMode is passed in based on whatever its configured in our FilesystemOptions so I think we'll need to allow setting a NewDirectoryMode on whatever options are used to construct this store and then flow that into here if that makes sense.

Ahh I didn't get it the first time, sorry about that. Fixed it now.

And the permission thing was a bit annoying indeed haha. I think I got it now but it was quite the treasure hunt and I had to touch a couple of files along my way (tried to recap it in my commit message of 0d53e7e). Let me know if there is anything wrong or if I forgot anything

robskillington · 2019-07-02T12:03:57Z

Nw, triggering a build on this now for CI.

robskillington · 2019-07-02T12:06:38Z

src/cluster/client/etcd/config.go

@@ -111,7 +113,8 @@ func (cfg Configuration) NewOptions() Options {
 		SetCacheDir(cfg.CacheDir).
 		SetClusters(cfg.etcdClusters()).
 		SetServicesOptions(cfg.SDConfig.NewOptions()).
-		SetWatchWithRevision(cfg.WatchWithRevision)
+		SetWatchWithRevision(cfg.WatchWithRevision).
+		SetNewDirectoryMode(cfg.NewDirectoryMode)


Hm, you might want to only set this if it's been set, maybe make it a pointer type?

Otherwise the option will always be set to os.FileMode(0) which is not desirable, we'd prefer the default of 0755 I believe?

NewDirectoryMode *os.FileMode` yaml:"newDirectoryMode"` if v := cfg.NewDirectoryMode; v != nil { opts = opts.SetNewDirectoryMode(*v) }

True. I made it a pointer type and included some unit tests as well.

robskillington · 2019-07-02T12:07:43Z

src/cluster/kv/etcd/store.go

+			zap.String("path", path),
+			zap.Error(err),
+		)
+	}


I think you want to return err here, otherwise it will flow through to the create successful path no?

Absolutely! Forgot that to put back in after refactoring, fixed now.

- `os.MkDir` -> `os.MkDirAll` - Test is now checking if created directory is indeed a directory - Caught error of function in `initCache()`

- Move from random dir name to constant - Use `c.opts.Prefix()` for file name - Test cache is working with `Set` and `Get`

In `dbnode/server.go` `envCfg` is holding `kv.Store`. In `dbnode/environment/config.go` during configuration `etcdclient` gets initiated, so its options had to be extended that `etcdclient.opts.NewDirMode()` yields the necessary dirMode (which gets passed to `envCfg`). `NewStore()` in `cluster/kv/etcd/store.go` can then access the dirMode through the client options.

robskillington

LGTM

obitech force-pushed the create_cache_dir branch from 5de13d4 to 13714de Compare June 20, 2019 21:02

richardartoul reviewed Jun 21, 2019

View reviewed changes

obitech force-pushed the create_cache_dir branch from 1f684d4 to 1bdf215 Compare June 21, 2019 11:40

cw9 reviewed Jun 21, 2019

View reviewed changes

obitech force-pushed the create_cache_dir branch from 1bdf215 to 804924d Compare June 22, 2019 11:49

richardartoul reviewed Jun 24, 2019

View reviewed changes

obitech force-pushed the create_cache_dir branch from 804924d to 119fe0d Compare July 1, 2019 20:21

robskillington reviewed Jul 2, 2019

View reviewed changes

obitech force-pushed the create_cache_dir branch from 119fe0d to ca9dc87 Compare July 3, 2019 19:25

obitech added 9 commits July 8, 2019 21:13

Create cache directory if it doesn't exist

a57cda7

Add tests for cache dir creation

df597d4

Add os.MkDirAll, extended test

8c35e7c

- `os.MkDir` -> `os.MkDirAll` - Test is now checking if created directory is indeed a directory - Caught error of function in `initCache()`

Refactor tests for cache directory

a29f124

- Move from random dir name to constant - Use `c.opts.Prefix()` for file name - Test cache is working with `Set` and `Get`

Use os.MkdirAll and remove extranous error checking

1e4765a

Add pointer type to NewDirectoryMode in client etcd config, unit tests

e054e31

Returning err on directory creation

4b83afe

Add additional unit tests for NewDirectoryMode

faec8b6

obitech force-pushed the create_cache_dir branch from 251aa82 to faec8b6 Compare July 8, 2019 19:13

robskillington approved these changes Jul 10, 2019

View reviewed changes

robskillington and others added 4 commits July 10, 2019 16:55

Merge branch 'master' into create_cache_dir

770350f

Adjust m3dbbnode/config test case for default config

e95c6f7

Merge branch 'master' into create_cache_dir

4e48836

Merge branch 'master' into create_cache_dir

235836d

richardartoul added ci Triggers CI (useful for external contributors) and removed ci Triggers CI (useful for external contributors) labels Jul 15, 2019

robskillington merged commit e21933a into m3db:master Jul 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically create folder for KV cache #1757

Automatically create folder for KV cache #1757

obitech commented Jun 20, 2019

obitech commented Jun 20, 2019

richardartoul Jun 21, 2019

obitech Jun 21, 2019

richardartoul Jun 21, 2019

richardartoul Jun 21, 2019

obitech Jun 21, 2019

cw9 Jun 21, 2019

obitech Jun 21, 2019

richardartoul commented Jun 21, 2019

cw9 Jun 21, 2019

obitech Jun 22, 2019

cw9 Jun 21, 2019

cw9 Jun 21, 2019

obitech Jun 22, 2019

cw9 Jun 21, 2019

obitech Jun 22, 2019

codecov bot commented Jun 22, 2019 •

edited

Loading

richardartoul Jun 24, 2019

obitech Jul 1, 2019

robskillington commented Jul 2, 2019

robskillington Jul 2, 2019

obitech Jul 3, 2019

robskillington Jul 2, 2019

obitech Jul 3, 2019

robskillington left a comment

Automatically create folder for KV cache #1757

Automatically create folder for KV cache #1757

Conversation

obitech commented Jun 20, 2019

obitech commented Jun 20, 2019

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

richardartoul commented Jun 21, 2019

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

codecov bot commented Jun 22, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington commented Jul 2, 2019

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

robskillington left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 22, 2019 •

edited

Loading