rbd: discover if StagingTargetPath in NodeExpandVolume exists #3624
Conversation
Madhu-1
force-pushed
the
fix-3623
branch
3 times, most recently
from
dc9209c
to
a653ee5
Compare
internal/rbd/nodeserver.go
Outdated
| @@ -1026,6 +1026,31 @@ func (ns *NodeServer) NodeUnstageVolume( | |||
| return &csi.NodeUnstageVolumeResponse{}, nil | |||
| } | |||
|
|
|||
| // getStagingPath returns the staging path for the volume from the volume path. | |||
| /* | |||
There was a problem hiding this comment.
mixing of comment styles is a little odd.
internal/rbd/nodeserver.go
Outdated
| // If stagingTargetPath is not found in volumePath then use | ||
| // volumePath | ||
| volumePath = req.GetVolumePath() | ||
| log.ErrorLog(ctx, "failed to find stagingTargetPath from volumePath %v: %v", volumePath, err) |
There was a problem hiding this comment.
This isn't always an error, and there is an alternative value used... Maybe log as info?
internal/rbd/nodeserver.go
Outdated
| var err error | ||
| volumePath, err = ns.getStagingPath(volumePath) | ||
| if err != nil { | ||
| // If stagingTargetPath is not found in volumePath then use | ||
| // volumePath | ||
| volumePath = req.GetVolumePath() | ||
| log.UsefulLog(ctx, "failed to find stagingTargetPath from volumePath %v: %v", volumePath, err) | ||
| } |
There was a problem hiding this comment.
@Madhu-1
Did you intend the code to the as follows ?
Otherwise the volumePath being sent to getStagingPath() is empty from the check on line 1066
| var err error | |
| volumePath, err = ns.getStagingPath(volumePath) | |
| if err != nil { | |
| // If stagingTargetPath is not found in volumePath then use | |
| // volumePath | |
| volumePath = req.GetVolumePath() | |
| log.UsefulLog(ctx, "failed to find stagingTargetPath from volumePath %v: %v", volumePath, err) | |
| } | |
| var err error | |
| volumePath, err = ns.getStagingPath(req.GetVolumePath()) | |
| if err != nil { | |
| // If stagingTargetPath is not found in volumePath then use | |
| // volumePath | |
| volumePath = req.GetVolumePath() | |
| log.UsefulLog(ctx, "failed to find stagingTargetPath from volumePath %v: %v", volumePath, err) | |
| } |
There was a problem hiding this comment.
Yes that correct, fixed now.
mergify
bot
dismissed
nixpanic’s stale review
Pull request has been modified.
|
@Mergifyio rebase |
✅ Branch has been successfully rebased |
|
/test ci/centos/k8s-e2e-external-storage/1.23 |
|
/test ci/centos/k8s-e2e-external-storage/1.24 |
|
/test ci/centos/k8s-e2e-external-storage/1.25 |
|
/test ci/centos/mini-e2e-helm/k8s-1.23 |
|
/test ci/centos/mini-e2e-helm/k8s-1.24 |
|
/test ci/centos/mini-e2e-helm/k8s-1.25 |
|
/test ci/centos/mini-e2e/k8s-1.23 |
mergify
bot
added
ok-to-test
I've seen that happen somewhere else too. But mostly the approvals are kept. |
|
/test ci/centos/k8s-e2e-external-storage/1.23 |
|
/test ci/centos/k8s-e2e-external-storage/1.24 |
|
/test ci/centos/k8s-e2e-external-storage/1.25 |
|
/test ci/centos/mini-e2e-helm/k8s-1.23 |
|
/test ci/centos/mini-e2e-helm/k8s-1.24 |
|
/test ci/centos/k8s-e2e-external-storage/1.23 |
|
/test ci/centos/mini-e2e-helm/k8s-1.25 |
|
/test ci/centos/k8s-e2e-external-storage/1.24 |
|
/test ci/centos/mini-e2e/k8s-1.23 |
|
/test ci/centos/k8s-e2e-external-storage/1.25 |
|
/test ci/centos/mini-e2e/k8s-1.24 |
|
/test ci/centos/mini-e2e-helm/k8s-1.23 |
|
/test ci/centos/mini-e2e/k8s-1.25 |
|
/test ci/centos/mini-e2e-helm/k8s-1.24 |
|
/test ci/centos/upgrade-tests-cephfs |
|
/test ci/centos/mini-e2e-helm/k8s-1.25 |
|
/test ci/centos/upgrade-tests-rbd |
|
/test ci/centos/mini-e2e/k8s-1.23 |
|
/test ci/centos/mini-e2e/k8s-1.24 |
|
/test ci/centos/mini-e2e/k8s-1.25 |
|
/test ci/centos/upgrade-tests-cephfs |
|
/test ci/centos/upgrade-tests-rbd |
The StagingTargetPath is an optional entry in NodeExpandVolumeRequest, We cannot expect it to be permanently set, and at the same time, cephcsi depended on the StaingTargetPath to retrieve some metadata
information.
This commit will check all the mount ref and identifies the stagingTargetPath by reviewing the image-meta.json file that exists. This is a costly operation as we need to loop through all the mounts and check image-meta.json in each mount, but this happens only if the StaingTargetPath needs to be set in the NodeExpandVolumeRequest.
fixes #3623
Note :- gofmt tool I am using also fixed the indentation problem in the comment section in nodeserver.go
Signed-off-by: Madhu Rajanna madhupr007@gmail.com