Skip to content

Fix force-remove for cluster volumes#44224

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
dperny:cluster-volumes-update
Oct 25, 2022
Merged

Fix force-remove for cluster volumes#44224
cpuguy83 merged 1 commit intomoby:masterfrom
dperny:cluster-volumes-update

Conversation

@dperny
Copy link
Copy Markdown
Contributor

@dperny dperny commented Sep 30, 2022

Signed-off-by: Drew Erny derny@mirantis.com

- What I did

Fixes force remove functionality for Swarm Cluster Volumes

- How I did it

Previously, the code assumed that attempting to force remove a local volume that did not exist would return a Not Found error. This is not the case; force-remove always returns no error. To correctly force-remove volumes, we must force locally and then send a force remove to Swarm.

Signed-off-by: Drew Erny <derny@mirantis.com>
@dperny dperny force-pushed the cluster-volumes-update branch from 9306d2e to 3246db3 Compare October 12, 2022 16:31
@neersighted neersighted modified the milestones: v-next, 22.06.0 Oct 25, 2022
@neersighted
Copy link
Copy Markdown
Member

neersighted commented Oct 25, 2022

Moved to 22.06 as @dperny considers this a blocker for the Swarm-Cluster-Volume support in that release. I'll create a backport after this is merged.

@neersighted
Copy link
Copy Markdown
Member

Backport is at #44360.

@thaJeztah thaJeztah modified the milestones: 22.06.0, v-next Oct 26, 2022
@thaJeztah
Copy link
Copy Markdown
Member

(Moved the milestone for this PR (for master) to v-next, and the backport to v22.06)

return err
// when a removal is forced, if the volume does not exist, no error will be
// returned. this means that to ensure forcing works on swarm volumes as
// well, we should always also force remove aginst the cluster.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// well, we should always also force remove aginst the cluster.
// well, we should always also force remove against the cluster.

Comment on lines +174 to 175
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're missing an return err here; docker/cli#4082 (comment)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants