-
Notifications
You must be signed in to change notification settings - Fork 40
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
reclaimspace: detect Kubernetes version for right StagingTargetPath #165
reclaimspace: detect Kubernetes version for right StagingTargetPath #165
Conversation
Kubernetes 1.24 and newer use a different path for staging the volume. That means the CSI-driver is requested to mount the volume at an other location, compared to previous versions of Kubernetes. CSI-drivers implementing the CSI-Addons NodeReclaimSpace procedure, must receive the correct path, otherwise the driver will not be able to free space and possibly return an error. See-also: kubernetes/kubernetes#107065 See-also: https://bugzilla.redhat.com/2096209 Signed-off-by: Niels de Vos <[email protected]>
9351866
to
1bbc6c1
Compare
Kubernetes versions < 1.24 should use the `-legacy` option, newer versions require `-drivername` to build the StagingTargetPath where the volume is mounted. Signed-off-by: Niels de Vos <[email protected]>
1bbc6c1
to
1f63bfc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -83,7 +84,17 @@ func (nrs *NodeReclaimSpace) Init(c *command) error { | |||
if c.stagingPath == "" { | |||
return fmt.Errorf("stagingpath is not set") | |||
} | |||
nrs.stagingTargetPath = fmt.Sprintf("%s/%s/globalmount", c.stagingPath, c.persistentVolume) | |||
|
|||
if !c.legacy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not do with only kube server version check?
Do we need another bool legacy parameter?
Maybe I am missing something 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could, but the command does not use any Kubernetes client at all, it is intended to be functional outside of Kubernetes too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay,
If it can run seamlessly on k8s version less than 1.24 than I am fine.
Or do we need to tweak legacy variable depending on the k8s version too?
(This change will need to go into rook)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay this is for the testing tool 🤦 my bad
Syncing latest changes from main for kubernetes-csi-addons
Kubernetes 1.24 and newer use a different path for staging the volume.
That means the CSI-driver is requested to mount the volume at an other
location, compared to previous versions of Kubernetes. CSI-drivers
implementing the CSI-Addons NodeReclaimSpace procedure, must receive the
correct path, otherwise the driver will not be able to free space and
possibly return an error.
See-also: kubernetes/kubernetes#107065
See-also: https://bugzilla.redhat.com/2096209