Fix/ub 1722 soft link issue on uncleared node #265
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @olgashtivelman and @shay-berman)
controller/controller.go, line 691 at r1 (raw file):
defer logger.Trace(logs.INFO, logs.Args{{"mountPoint", mountPoint}})() slinkStat, err := executer.Stat(mountPoint)
If I got it right, then this function get the real mountpoint /ubiqutiy/WWN
So slinkstat var name is a bit confusing, because its not the slink, its the real mountpoint directory - so maybe use name "mountPointStat".
controller/controller.go, line 696 at r1 (raw file):
} rootDirStat, err := executer.Lstat(mountPoint + "/..")
Please change the "/.." to more golang orientation with https://golang.org/pkg/path/filepath/#Dir
its more generic and works for windows as well. (its like linux dirname command line)
controller/controller.go, line 709 at r1 (raw file):
func checkSlinkAlreadyExistsOnMountPoint(mountPoint string, k8sMountPoint string, logger logs.Logger, executer utils.Executor) (error){
If this PR will break scale then we cannot merge it.
@deepak please let us know what do you think here. is the check Olga did is ok by scale as well? if not then this PR also need to some switch case for scale vs scbe. I think you mentioned that you don't want to do this checkSlinkAlreadyExistsOnMountPoint at all for scale. waiting for your instructions.
controller/controller.go, line 761 at r1 (raw file):
slinks = append(slinks, file) } else { logger.Warning("found a mounted slink to current mount point - which is not in use.", logs.Args{{"file", file}})
please improve the warning text message - its not clear
"Found a different Pod ID [%s] that uses the same PVC [%s] but the slink [%s] is pointing to directory [%s] that is NOT mounted. It mate be a stale POD."
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @olgashtivelman and @shay-berman)
controller/controller.go, line 691 at r1 (raw file):
Previously, shay-berman wrote…
If I got it right, then this function get the real mountpoint /ubiqutiy/WWN
So slinkstat var name is a bit confusing, because its not the slink, its the real mountpoint directory - so maybe use name "mountPointStat".
actually I made some changes - so I already know the /ubiquity/WWN (from the previous function ) so I am just using it to begin with.
controller/controller.go, line 696 at r1 (raw file):
Previously, shay-berman wrote…
Please change the "/.." to more golang orientation with https://golang.org/pkg/path/filepath/#Dir
its more generic and works for windows as well. (its like linux dirname command line)
Done.
controller/controller.go, line 709 at r1 (raw file):
Previously, shay-berman wrote…
If this PR will break scale then we cannot merge it.
@deepak please let us know what do you think here. is the check Olga did is ok by scale as well? if not then this PR also need to some switch case for scale vs scbe. I think you mentioned that you don't want to do this checkSlinkAlreadyExistsOnMountPoint at all for scale. waiting for your instructions.
as far as I understand scale are not using this function at all am I wrong? this is why I did not add them to the review.
|
controller/controller.go, line 709 at r1 (raw file): Previously, olgashtivelman wrote…
Yes we will not call checkSlinkAlreadyExistsOnMountPoint. Kindly refer PR #264 |
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @shay-berman)
controller/controller.go, line 761 at r1 (raw file):
Previously, shay-berman wrote…
please improve the warning text message - its not clear
"Found a different Pod ID [%s] that uses the same PVC [%s] but the slink [%s] is pointing to directory [%s] that is NOT mounted. It mate be a stale POD."
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @shay-berman)
controller/controller.go, line 691 at r1 (raw file):
Previously, olgashtivelman wrote…
actually I made some changes - so I already know the /ubiquity/WWN (from the previous function ) so I am just using it to begin with.
Done.
controller/controller.go, line 709 at r1 (raw file):
Previously, yadaven (Yadavendra Yadav) wrote…
Yes we will not call checkSlinkAlreadyExistsOnMountPoint. Kindly refer PR #264
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved
controller/controller.go, line 709 at r1 (raw file):
Previously, olgashtivelman wrote…
Done.
please make sure you merge it only after yadav merge his PR.please make sure you merge it only after yadav merge his PR.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: 1 of 3 files reviewed, all discussions resolved
in this PR the issue that is solved is the following:
as part of the mount there is a check made to see if there are any solf links pointing to the desired mount point ( to /ubiquity/WWN) -this is needed in order to not allow 2 pods to be attached to the same PVC.
an issue may accur when one of those soft links is not actually active (for example of a pod moving to another node in the cluster and back to the original one due to shutdowns) - this is what this PR solves.
beside checking if any soft links exists - a check is made to see if the mount point is mounted. if it is then this is a valid solf link and the current mount should fail. otherwise the current mount can continue un-interrupted.
This change is