-
Notifications
You must be signed in to change notification settings - Fork 10
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
LUN Fuzzy vs. RegEx Search #118
Comments
we are testing a solution to this, the solution is going to replace the strings.Contain() with a regex MatchString(). The regex pattern is taken from the upstream example: FcPath := "^(pci-.*-fc|fc)-0x" + wwn + "-lun-" + lun + "$" This will fix the matching logic to find the correct device path. Also along with this fix we are changing the polling interval and frequency for the findMultipathDevice() function. The old values were to sleep 1 Second between polling iterations with up to 10 iterations. With the upcoming change the polling sleep time is set to 500 Milliseconds with up to 20 iterations. This should improve the performance of the findMultipathDevice(). Additionally, timing log messages at the DEBUG level have been added so that users can look at the node logs for FC timing values if needed. |
the solution or fix to this will be made available via a patch release (2.16.1). We are planning that release for hopefully next week. |
@jmccormick2001 I think I just found another issue in the same method. It appears that for LUN numbers > 255,
The REGEX does a search for the Seems this s known in the upstream and the general guidance is to not trust It appears the only use of Looks like an optional method was added in upstream k8s for searching specifically by ID: |
@singlecheeze nice find, it will require a fix for sure. |
@singlecheeze fyi, the 255 limit fix will land in the 2.17.0 release in the coming weeks, this fix is coded and it seems to test ok. |
@jmccormick2001 is that by chance pushed to github or will it be pushed when 4.17 is ready? |
@singlecheeze we generally do not push prerelease source to Github, but
we'll push it when 2.17.0 is ready. Let's correspond via email if there's a
need ahead of that.
… Message ID: ***@***.***
com>
|
infinibox-csi-driver/storage/fcnode.go
Line 780 in 24830ee
I wrote a quick comparison of the
findFcDisk
method and compared it to the upstream. I provisioned 300 LUNs on my SAN, and am invoking a debug pod in OpenShift and outputtingls /host/dev/disk/by-path/
.Note: The upstream inclusion of the
$
in the RegEx which is very important.With
strings.Contains
in the current implementation, and given a/host/dev/disk/by-path/ of fc-0x100000109bc3914e-0x21000024ff1e85b6-lun-10
there are many instances where higher LUN numbers could be returned.The current driver returns as soon as the first
strings.Contains
is matched, which is illustrated by (I deleted LUN 1 from the array and rescanned to make sure CoreOS picked it up):Testing contains for fc-0x100000109bc3914e-0x21000024ff1e85b6-lun-1
should not match, but it does and LUN 10 would be returned by the current driver, when in reality, this is a completely different device.The text was updated successfully, but these errors were encountered: