Inject resources when sriovToken label exists in annotations field #21
Conversation
|
@edwarnicke can you please give me approval for the workflows to run ? |
Signed-off-by: Dimitrios Markou <dimitrios.markou@est.tech>
mardim91
force-pushed
the
sriovResources
branch
from
4b5b2ed
to
4f03da5
Compare
main.go
Outdated
| @@ -164,7 +167,38 @@ func (s *admissionWebhookServer) createVolumesPatch(p string, volumes []corev1.V | |||
| return jsonpatch.NewOperation("add", path.Join(p, "volumes"), volumes) | |||
| } | |||
|
|
|||
| func parseInterfacePools(v string, logger *zap.SugaredLogger) map[string]int { | |||
There was a problem hiding this comment.
Probably this one would be better?
| func parseInterfacePools(v string, logger *zap.SugaredLogger) map[string]int { | |
| func parseSRIOVLabels(v string, logger *zap.SugaredLogger) map[string]int { |
There was a problem hiding this comment.
ok I will fix that. Thanks!
|
@mardim91 |
@Bolodya1997 Yes I have tested it live. The initContainer terminates but device plugin doesn't mark the device as free because the devices are allocated per Pod and not per Container so everything works as it should be. |
Signed-off-by: Dimitrios Markou <dimitrios.markou@est.tech>
@mardim91 OK... that is both incredibly cool and unexpected :) Kudos to you for catching that :) Question though... is that the contract for DevicePlugin, or just how it happens to be implemented. I'm trying to understand whether we have a risk to manage around the behavior possibly changing in the future. |
|
@denis-tingaikin Thoughts? |
@edwarnicke I think is the contract and not how it happens to be implemented. I am saying that because before using nsm I have tested the same thing with SRIOVDevicePlugin and the behaviour was exactly the same. |
| u := (*nsurl.NSURL)(networkService) | ||
| labels := u.Labels() | ||
| if _, ok := labels["sriovToken"]; ok { | ||
| interfacePools := strings.Split(labels["sriovToken"], ",") |
There was a problem hiding this comment.
@Bolodya1997 Do we have a const for sriovToken?
There was a problem hiding this comment.
@denis-tingaikin we have this:
but the constant starts with a lower case letter so it is not exported and golang doesn't allow to use it. So I did no change.
There was a problem hiding this comment.
It sounds to me like the const should be exported to avoid hardcoded strings in other places.
@Bolodya1997 Thoughts?
There was a problem hiding this comment.
It is actually mostly a contract than dependency, because the most common usage of this constant is manually using it in .yaml file - in such case there is no way to import it.
But it can be changed to exported to be used here as a dependency, probably it would be better to do such thing.
There was a problem hiding this comment.
@denis-tingaikin @Bolodya1997 If there is no other objection maybe we can proceed with the merging of the PR as it is and when the variable gets exported correctly then we can change the "sriovToken" string in a later PR. What do you think?
There was a problem hiding this comment.
@mardim91 I think it should be easy to export the constant from the package. Could you open the PR for sdk-sriov? We will consider it as soon as possible.
There was a problem hiding this comment.
@denis-tingaikin I can do that yes. The only thing that I will do is upper case the vars
There was a problem hiding this comment.
@mardim91 Finally I think it can be considered by separate PRs.
There was a problem hiding this comment.
@denis-tingaikin What do you mean it can be considered by separate PRs ? I didn't get that
|
Added some cosmetic comments, in general looks good. |
Signed-off-by: Dimitrios Markou <dimitrios.markou@est.tech>
Signed-off-by: Dimitrios Markou <dimitrios.markou@est.tech>
|
@denis-tingaikin Has your feedback been responded to? Is this good to go? |
|
@edwarnicke LGTM |
Signed-off-by: Dimitrios Markou dimitrios.markou@est.tech