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

Create labels when volume exists only remotely #34896

Merged
merged 1 commit into from Nov 15, 2017

Conversation

Projects
None yet
7 participants
@cpuguy83
Contributor

cpuguy83 commented Sep 19, 2017

Before this, if a volume exists in a driver but not in the local cache,
the store would just return a bare volume. This means that if a user
supplied options or labels, they will not get stored.

Instead only return early if we have the volume stored locally. Note
this could still have an issue with labels/opts passed in by the user
differing from what is stored, however this isn't really a new problem.

There is a minor behavior change in the interaction with the volume
driver in that create is called each time instead of doing a
get and then create. This shouldn't be a problem as the get was just a
best effort and create is expected to be idempotent.

This fixes a problem where if there is a shared storage backend between
two docker nodes, a create on one node will have labels stored and a
create on the other node will not.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83
Contributor

cpuguy83 commented Sep 19, 2017

@sixcounts

This comment has been minimized.

Show comment
Hide comment
@sixcounts

sixcounts Oct 23, 2017

@tiborvass any idea when this pull request will get merged for an engine release?

Thank you

sixcounts commented Oct 23, 2017

@tiborvass any idea when this pull request will get merged for an engine release?

Thank you

@sixcounts

This comment has been minimized.

Show comment
Hide comment
@sixcounts

sixcounts Nov 6, 2017

@tiborvass any ETA as to when this will get merged into which release? I have a customer that has been having problems with this Docker Certified Rex-Ray PlugIn for EMC's Isilon for several months now.

Thank you,

Josh

sixcounts commented Nov 6, 2017

@tiborvass any ETA as to when this will get merged into which release? I have a customer that has been having problems with this Docker Certified Rex-Ray PlugIn for EMC's Isilon for several months now.

Thank you,

Josh

@sixcounts

This comment has been minimized.

Show comment
Hide comment
@sixcounts

sixcounts Nov 6, 2017

@cpuguy83 @tiborvass there is a meeting with this customer this week and we need a solid update to give them. Can someone provide an update on to when this will be merged and what release will address it? This stemmed from the Docker Certified Plugin for Rex-Rex for EMC Isilon:

rexray/rexray#989

Thank you,

Josh

sixcounts commented Nov 6, 2017

@cpuguy83 @tiborvass there is a meeting with this customer this week and we need a solid update to give them. Can someone provide an update on to when this will be merged and what release will address it? This stemmed from the Docker Certified Plugin for Rex-Rex for EMC Isilon:

rexray/rexray#989

Thank you,

Josh

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 7, 2017

Member

@sixcounts for certified plugins it'll have to end up in a Docker EE release, so that would have to be back-ported by Docker; it's best to open a backport request for that internally.

Member

thaJeztah commented Nov 7, 2017

@sixcounts for certified plugins it'll have to end up in a Docker EE release, so that would have to be back-ported by Docker; it's best to open a backport request for that internally.

@moby moby deleted a comment from sixcounts Nov 8, 2017

@thaJeztah thaJeztah added this to backlog in maintainers-session Nov 9, 2017

Show outdated Hide outdated volume/store/store.go Outdated
Create labels when volume exists only remotely
Before this, if a volume exists in a driver but not in the local cache,
the store would just return a bare volume. This means that if a user
supplied options or labels, they will not get stored.

Instead only return early if we have the volume stored locally. Note
this could still have an issue with labels/opts passed in by the user
differing from what is stored, however this isn't really a new problem.

This fixes a problem where if there is a shared storage backend between
two docker nodes, a create on one node will have labels stored and a
create on the other node will not.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Nov 13, 2017

Collaborator

@cpuguy83 if is ready to merge ?

Collaborator

vieux commented Nov 13, 2017

@cpuguy83 if is ready to merge ?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Nov 13, 2017

Contributor

I'm happy with it, no behavior change in volume driver interaction like was originally implemented.

On mobile so I can't inspect that windows error at the moment.

Contributor

cpuguy83 commented Nov 13, 2017

I'm happy with it, no behavior change in volume driver interaction like was originally implemented.

On mobile so I can't inspect that windows error at the moment.

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Nov 13, 2017

Collaborator

@cpuguy83

21:33:58 ----------------------------------------------------------------------
21:33:58 FAIL: docker_cli_logs_test.go:92: DockerSuite.TestLogsTail
21:33:58 
21:33:58 docker_cli_logs_test.go:113:
21:33:58     c.Assert(lines, checker.HasLen, testLen+1)
21:33:58 ... obtained []string = []string{""}
21:33:58 ... n int = 101
21:33:58 
21:34:05 
21:34:05 ----------------------------------------------------------------------

I restarted

Collaborator

vieux commented Nov 13, 2017

@cpuguy83

21:33:58 ----------------------------------------------------------------------
21:33:58 FAIL: docker_cli_logs_test.go:92: DockerSuite.TestLogsTail
21:33:58 
21:33:58 docker_cli_logs_test.go:113:
21:33:58     c.Assert(lines, checker.HasLen, testLen+1)
21:33:58 ... obtained []string = []string{""}
21:33:58 ... n int = 101
21:33:58 
21:34:05 
21:34:05 ----------------------------------------------------------------------

I restarted

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Nov 14, 2017

Collaborator

LGTM

Collaborator

vieux commented Nov 14, 2017

LGTM

1 similar comment
@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Nov 15, 2017

Member

LGTM

Member

tonistiigi commented Nov 15, 2017

LGTM

@vieux vieux merged commit f62aeae into moby:master Nov 15, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37782 has succeeded
Details
janky Jenkins build Docker-PRs 46511 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6901 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18098 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6711 has succeeded
Details

@thaJeztah thaJeztah removed this from backlog in maintainers-session Nov 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment