-
Notifications
You must be signed in to change notification settings - Fork 174
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
Use exact match or regex for pfname selector #350
Conversation
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.
Thank you for the patch. Its an interesting enhancement. Id like to have this feature turned off by default.
if strings.EqualFold(strings.Split(item, "#")[0], needle) { | ||
return item | ||
} | ||
re, err := regexp.Compile(strings.Split(item, "#")[0]) |
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.
I am worried about a regex expression with '#' char itself. We should definitely throw an error if that is included.
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.
I think you should split the string once into a var once you have checked it doesn't include more than one '#' char at the start of this loop. Ignore any string that have more than one '#' and log it.
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.
Sure I understand your concern. Not sure how to make this "turned off", but one way to address both issue could be to use a special notation like "re{<regexp}#" so the "re{...}" is the way to turn on the feature and the brackets ensure that everthing included is the regexp and everything that follows is the VF list.
Regex delimiter could also be "/regexp/[#vfs]" (slash is definitely not an interface name allowed character)
@@ -171,11 +173,20 @@ func contains(hay []string, needle string) bool { | |||
return false | |||
} | |||
|
|||
func getItem(hay []string, needle string) string { | |||
func getItem(hay []string, needle string, what string) string { |
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.
I don't think the 'what' arg is needed.
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.
Sure can remove.
for _, item := range hay { | ||
// May be an exact match or a regex, try both. |
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.
I think I'd like to have this regex matching an optional feature that would be off by default. I am worried a PF name could end up being a regex expression and cause strange matches to occur.
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.
With explicit re delimiter, intent would be clear that it must compile/match.
@rozeps can you explain the problem you are trying to solve with this PR ? |
We are deploying in multi-nodes OpenShift clusters, and we support few vendors SR-IOV for use in our applications (matching on vendor PF is not sufficient here). We are reserving VFs 0-2 for internal use then 3-7 for end user applications. As interface name not fully predictive (systems end up with ens5s0f0 or enp65s0f0, and so on). I am tring to come up with configuration that would match any PF from vendor/device XYZ, take only VFs 3-7. |
@rozeps |
Well apparently yes, but I just looked at few systems that are based on same Xeon family (same socket/numa architecture), just wonder how that may differ in more heterogeneous h/w environment (that customers would be allowed to use). In my mind, would be much simpler to say selector: "match vendor=0x8086,device=0x158b,pf=/.*/#3-7" With the re enclosed in special characters (illegal ifname ones) don't think that would break much existing implementations. |
So if you would have a " |
Sure you mean no PFNames just the VFIndex as list of id that would be perfect (and cleaner I believe). |
@rozeps this PR was discussed in today's technical meeting see: https://docs.google.com/document/d/1bEyEh_K9lGwQWHgUHUH-vqodYd1W-LCWRGlW0-ZVR5A/edit# We would like to understand more about your use-case and whether or not it is possible to get persistent and predictable net device names. if it is, then using if it isnt, then a "VFIndex" selector might do the trick. |
We cannot predict all the combinations now and future, but on my test systems I have 2 nodes with mix of different cards (Mellanox/Intel) and the PCI addresses are different, and the network device names also. Anticipating all possible PF names might be error prone one day someone deploy node with slightly different config and the PF name is not on the list ... may take while to figure out. The VFIndex I think would have been a good idea if initially tought (in addition to the PFName) but right now that could conflict right one may have non-overlapping pfname with "#0-2-3" part and VFIndex. The regex enclosed withih "/" is backward compatible with existing syntax - when not present exact match on PF name is used otherwise the regex logic. |
AND is performed between different selectors, so result will be consistent @zshi-redhat, @martinkennelly any [additional] thoughts on this ? |
Do you want to introduce a match on all PF interface names or do we need a regex for advanced matchings? |
@zshi-redhat @adrianchiris @rozeps |
I think we didn't get conclusion on which approach to implement or use:
|
Not sure if it can help to decide, but in our case we'd like to have a selector based on "vendor", "device", "driver", and "vfIndex" the later could have syntax like "pfnames: ["#2", "#6", "#8"] for example i.e. no regex needed only VF number. I guess one could also want: "pfnames: ["enp66s0f0#1", "#2", "enp65s0f1"] i.e. with both, pfname only or vfIndex only. |
Hi @rozeps any update on this PR or we can close it? |
no update closing this one |
Use regular expression as PF selector.