volplugin, systemtests: Incorporated mount locks #327
Conversation
|
|
||
| if dc.increaseMount(volName) > 1 { | ||
| if !volConfig.Unlocked && mountCount > 1 { | ||
| if mountCount > 2 { // keeping counter=2 during locked mounts helps docker unmount from removing the actual mount |
There was a problem hiding this comment.
I don't think we want to do this decrease here. If the mount is requested multiple times in parallel this will inflate the value, but still cause docker to issue an unmount and eventually go below 0 (which deliberately causes a panic). Let's somehow make sure this is tested directly if you feel the need to keep it.
There was a problem hiding this comment.
Good catch. You are right. It is not required and In fact that code path will never be executed as consecutive unmount after mount failure will always keep counter at 1 and never > 2. I'm working on fixing this.
|
I'm guessing the passing tests just answered my questions re: NFS. Missed that sorry. I think we just need a very aggressive test to trigger the mount locking race I think we still have here. If not, we still have an extra test, so this should still be a good use of time. Let's discuss offline how that test might look. |
|
Only comment I have is that you might want to configure your editor to automatically run |
|
Hmm that makes me think it might be time to add gofmt checks like our golint/vet ones, etc. |
|
That sounds goods to add gofmt along with other go tools. |
|
#328 provided for future expansion. |
|
#333 docker bug |
|
This needs to be rebased on master. |
| @@ -170,7 +167,7 @@ func (s *systemtestSuite) TestVolpluginRestartMultiMount(c *C) { | |||
| _, err := s.mon0cmd("sudo truncate -s0 /tmp/volplugin.log") | |||
| c.Assert(err, IsNil) | |||
|
|
|||
| c.Assert(s.createVolume("mon0", "policy1", "test", nil), IsNil) | |||
| c.Assert(s.createVolume("mon0", "policy1", "test", map[string]string{"unlocked": "true"}), IsNil) | |||
There was a problem hiding this comment.
this should not be unlocked.
There was a problem hiding this comment.
If its not unlocked, multiple mounts won't be possible which will contradict the test case.
There was a problem hiding this comment.
ohhh becuase it's same host. ok, thanks.
|
This patch will not do graceful unmount due to this issue #333 Until docker fixes the issue, its safe to use this mount format policy1/test:/mnt:no copy |
| @@ -2,78 +2,93 @@ package systemtests | |||
|
|
|||
| import ( | |||
| "fmt" | |||
| . "gopkg.in/check.v1" | |||
There was a problem hiding this comment.
Please put this line, surrounded by whitespace lines, on line 11. This is where https://github.com/bradfitz/goimports would put it by default (we can discuss tooling further offline if you want)
Signed-off-by: Yuva Shankar <[email protected]>
Signed-off-by: Yuva Shankar <[email protected]>
|
@unclejack it's rebased now @yuva29 is this ready for final review? |
|
@dseevr Yeah. |
|
LGTM |
|
can everyone review this today? This is the last mile for v0.2. |
|
LGTM |
fixes #295