Added states for docker-volume-netshare.

[jakirpatel]

Hello ,

I am sending this pull request for significant change in plugin architecture. This PR will solve two major issues:

1. States of the volumes:

If the plugin restarts then the previous version of plugin was not able to handle the state. The states of docker daemon and plugin were inconsistent. As I can see the few comments in issue either we can maintain the persistent storage to handle this or at the start of plugin we can let the plugin knows that the states are not consistent.

What I did here? Before the plugin initialize all its component like volume driver etc. I am calling the docker daemon and getting the details about volumes. That I call activeConnections.

To know all these states present I initialized the MountManager and added the states into the memory of plugin.

2.NFS inconsistent connections: While testing I observed if I remount the volume the connections were not increased. Also if you forcefully umount the volume and start container with same mount then previous version of plugin was increasing the number of connections with mount by 2.

That was the reason of unsual behaviour of unmounting the mounts. Also due to plugin is acting as the proxy and in mentioned scenario it is still assuming the connections are still there. It goes into inconsistent state.

This inconsistent connections leads problem with removal of docker volume. The current PR is solving this issue. Also it is necessary to know docker perspective that docker will also assume the stopped containers using the volume.

Before the deleting plugin must check active connections and also references with stopped containers. If both are zero then it will be able to delete the volume.

As I am using the Docker Go Client, I introduced new flag to plugin for maintaining the version compatibility for the plugin.

This flag will set the DOCKER_API_VERSION which is required for Go client. By default it will use the lastest stable version. Currently it is 1.37.

Please review it.

[jakirpatel]

Any author to review this Pull Request ?? @auhlig @gondor

[gondor]

@jakirpatel Sorry for the late response. Didn't realize my Github messages were going into another inbox after recently making some adjustments for spam.

This looks great and your findings make sense. Regarding the DOCKER_API_VERSION statement, does it require you to be on a min. version of Docker? Wondering what version ranges this introduction supports?

[jakirpatel]

@gondor

Thanks for your reply. For this enhancement, the minimum supported API version is 1.12.

[jakirpatel]

@gondor

I have updated this pull request with one more change respect to NFS driver. While testing with mounting the NFS I observed that if mount itself returns the error then the plugin was not maintaining the consistent state.

We can get the mount error in following scenarios:

1. NFS server is down.
2. Mount timeout / retry fails.
3. No permission to mount the volume.
4. Typo / Wrong mount options.

In this scenarios, the plugin must maintain the consistent state. I imagine the states in docker daemon should be same as the states in the docker-volume-netshare.

This may be applicable for all drivers but I didn't get chance to review and modify it.

[jakirpatel]

@gondor Do you get chance to view it ? :) please let me know.

[gondor]

@jakirpatel Everything looks great. I don't have time to fully test, but will this introduce breaking changes for anyone uses the plugin out of the box and on > 1.12 of docker? Just want to make sure we cover our basis.

[jakirpatel]

@gondor This is the major change in terms of architecture perspective. I have tested it, But to make sure it will be really helpful if you have little time to perform the tests on all drivers. If you think there might be any suggestion or change required let me know as well.

I have fully tested it on NFS driver (in all scenarios). There are no major changes in other drivers.

[gondor]

@jakirpatel ok I'm good with that. Went through everything again and left a comment related to the wrong import paths in a few files.

[jakirpatel]

@gondor

Thanks for your comments. I have added that changes and also changed the documentation as well please let me know if this looks good.

[jakirpatel]

@gondor

Good morning! Just following up with you if you get chance to view my modified PR :)

[jakirpatel]

@gondor Could you please review it again? I didnt heard from you :)

[jakirpatel]

@gondor

Please let me know if you get the chance to review it. The merge may be important to keep my app updated with upstream.

[jakirpatel]

@gondor

Is there any updates on my PR? I am using this now in production with NFS and seems working fine.

[jakirpatel]

@gondor Waiting for your response.

[pharazon]

I would need this patch to get my docker swarm to work on NFS.

[jakirpatel]

@pharazon You can use it without patching it. Check my fork. I have added the changes there as well.