-
Notifications
You must be signed in to change notification settings - Fork 308
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
DAOS-16791 control: Add include_fabric_ifaces to agent config #15470
Conversation
Provide an inverse to the existing exclude_fabric_ifaces directive. In some cases, a given environment will only have a small number of valid interfaces, so it is simpler to specify that rather than having to exclude all of the invalid interfaces. Features: control Required-githooks: true Change-Id: I0d8445fcc2da267df76856d7f9f9b43eed290599 Signed-off-by: Michael MacDonald <[email protected]>
Ticket title is 'Add include_fabric_ifaces to daos_agent configuration' |
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.
Nice. I wouldn't block on the unit test comment, I do think it should be as rigorous as the other cases though.
src/control/cmd/daos_agent/fabric.go
Outdated
currentNumaDevIdx map[int]int // current device idx to use on each NUMA node | ||
currentNUMANode int // current NUMA node to search | ||
ifaceFilterSet common.StringSet // set of interface names for filtering | ||
ifaceFilterExclude bool // true: exclude filtered interfaces, false: include only filtered interfaces |
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.
bool is not the best design here (false meaning include? when the size of the set above is not 0?)
consider instead defining a new type (ifaceFilter) with mode (include/exclude as const/enum? and then not needing to check the size of the set but check if filter is nil or not.
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.
Hmm. I didn't want to overengineer this as it's all internal to the agent, but I can take another look at it. I didn't really like this either, tbh.
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.
An alternative could be to pass in the desired filter function as a parameter directly.
Change-Id: I92b23ba08e4d98ca02f3d7726759e12a28446293
Required-githooks: true Change-Id: Icabe2f76934383beb4b8fb3258a3f0b0bb1120f1 Signed-off-by: Michael MacDonald <[email protected]>
ef4aecc
Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15470/2/testReport/ |
Provide an inverse to the existing exclude_fabric_ifaces directive. In some cases, a given environment will only have a small number of valid interfaces, so it is simpler to specify that rather than having to exclude all of the invalid interfaces. Required-githooks: true Change-Id: Ifacccc8cd8340de9dbffd358117092bdc473c497 Signed-off-by: Michael MacDonald <[email protected]>
#15513) Provide an inverse to the existing exclude_fabric_ifaces directive. In some cases, a given environment will only have a small number of valid interfaces, so it is simpler to specify that rather than having to exclude all of the invalid interfaces. Signed-off-by: Michael MacDonald <[email protected]>
Provide an inverse to the existing exclude_fabric_ifaces
directive. In some cases, a given environment will only have
a small number of valid interfaces, so it is simpler to
specify that rather than having to exclude all of the
invalid interfaces.
Features: control
Required-githooks: true
Change-Id: I0d8445fcc2da267df76856d7f9f9b43eed290599
Signed-off-by: Michael MacDonald [email protected]