Skip to content
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.

Sign up for GitHub

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

CSI node plugin fix for unstaging volumes #3178

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

CSI node plugin fix for unstaging volumes #3178

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions agent/csi/plugin/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,11 +263,8 @@ func (np *nodePlugin) NodeUnstageVolume(ctx context.Context, req *api.VolumeAssi

// we must unpublish before we unstage. verify here that the volume is not
// published.
if v, ok := np.volumeMap[req.ID]; ok {
if v.isPublished {
return status.Errorf(codes.FailedPrecondition, "Volume %s is not unpublished", req.ID)
}
return nil
if v, ok := np.volumeMap[req.ID]; ok && v.isPublished {
return status.Errorf(codes.FailedPrecondition, "Volume %s is not unpublished", req.ID)
}

Comment on lines +266 to 269
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super-familiar with this code, but should this actually proceed if the volume was not found in np.volumeMap ?

i.e., the volume should be present if it was successfully staged. If a failure happened during staging, it wouldn't be added. Looking at NodeStageVolume further up;

swarmkit/agent/csi/plugin/plugin.go

Lines 231 to 239 in e8ecf83

if err != nil {
return err
}
v := &volumePublishStatus{
stagingPath: stagingTarget,
}
np.volumeMap[req.ID] = v

Perhaps this should be something like;

if v, ok := np.volumeMap[req.ID]; !ok || v.isPublished {
	// volume not found or is still published
	return status.Errorf(codes.FailedPrecondition, "Volume %s is not unpublished", req.ID)
}

Or if it would be useful to have distinct errors for each situation

v, ok := np.volumeMap[req.ID
if !ok {
	return status.Errorf(codes.FailedPrecondition, "Volume %s not found", req.ID)
}
if v.isPublished {
	return status.Errorf(codes.FailedPrecondition, "Volume %s is not unpublished", req.ID)
}

Copy link
Author

@beornf beornf Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed the assumptions around state for unstaging would always hold true if np.volumeMap was made persistent:

swarmkit/agent/csi/plugin/plugin.go

Lines 60 to 63 in ea1a7ce

// volumeMap is the map from volume ID to Volume. Will place a volume once it is staged,
// remove it from the map for unstage.
// TODO: Make this map persistent if the swarm node goes down
volumeMap map[string]*volumePublishStatus

I recall during testing it is possible that a volume had been staged before the node daemon restarted and np.volumeMap was empty. In the method NodeUnpublishVolume it always unpublishes the volume irrespective of np.volumeMap.

_, err = c.NodeUnstageVolume(ctx, &csi.NodeUnstageVolumeRequest{
Expand Down