-
Notifications
You must be signed in to change notification settings - Fork 114
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
Add 'IB VF GUID Configuration' design doc #653
Add 'IB VF GUID Configuration' design doc #653
Conversation
Thanks for your PR,
To skip the vendors CIs use one of:
|
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.
Thx @almaslennikov.
I added few comments
e69cac9
to
4727e2f
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
4727e2f
to
b02243e
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
doc/design/ib-vf-configuration.md
Outdated
it’s proposed not to return an error in this case but assign as many GUIDs as possible. | ||
To ensure that nothing breaks when users add/remove VFs, the GUID distribution order should always be the same for each individual host. | ||
|
||
If there are fewer GUIDs than VFs, then all the GUIDs should be assigned. |
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 fail in this case ?
or do we randomly generate for the rest ?
if you have 5 GUIDS and 10 VFs, what will be the "other" 5 GUIDs ?
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.
They will be assigned randomly as it's done now. Noted that in the doc
doc/design/ib-vf-configuration.md
Outdated
### Goals | ||
|
||
* IB GUID configuration can be read from a static json file on the host | ||
* IB GUID configuration is static and created in advance |
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.
please add per node
right as you need unique pkeys in the cluster right?
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.
@almaslennikov did you add this clarification to the design doc ?
|
||
There can be fewer VFs created than GUIDs. To persist the dynamic nature of the SR-IOV Network operator, | ||
it’s proposed not to return an error in this case but assign as many GUIDs as possible. | ||
To ensure that nothing breaks when users add/remove VFs, the GUID distribution order should always be the same for each individual host. |
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.
how this will work with the parallel nic configuration that was implemented in the sriov operator?
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 believe that if we synchronize the access to the GUID pool, we shouldn't face any issues
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.
please add this one to the test or validation section so we remember to test this :)
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.
Actually, we have changed the impl so that the pool object is immutable. No additional synchronization measures are needed. Tested that on a baremetal cluster
|
||
If there are fewer GUIDs than VFs, then all the GUIDs should be assigned. | ||
|
||
### Config file |
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.
what will be the know location of this file?
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 propose to use /var/opt/infiniband_guids
. It should be writable across different cloud platforms. Noted that in the doc
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 already have folder that is in used by the sriov operator maybe we can use that one and not create another one?
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.
Let's store the file in /etc/sriov-operator/infiniband/guids
then
b02243e
to
9347961
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
thx, looks good to me |
9347961
to
8da9cce
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
8da9cce
to
e71fe7d
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
e71fe7d
to
854311c
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
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.
overall looks OK. added a few small comments, once addressed im LGTM
854311c
to
a7c1294
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
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.
one minor nit otherwise lgtm
Pull Request Test Coverage Report for Build 9203390833Details
💛 - Coveralls |
a7c1294
to
93fa079
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
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.
Left one minor comment. Other than that, LGTM
Signed-off-by: amaslennikov <[email protected]>
93fa079
to
7ead95e
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
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
LGTM, merging. |
No description provided.