Interface redesign #553
Interface redesign #553
Conversation
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
SchSeba
force-pushed
the
interface_redesign
branch
from
015052b to
ae7e5e9
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 7451460358
💛 - Coveralls |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| return FilesystemRoot | ||
| defer file.Close() |
Check warning
Code scanning / CodeQL
Writable file handle closed without error handling Warning
There was a problem hiding this comment.
@SchSeba do we need to address this alert?
SchSeba
force-pushed
the
interface_redesign
branch
from
ae7e5e9 to
2b6afe5
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
SchSeba
force-pushed
the
interface_redesign
branch
from
2b6afe5 to
6a9d270
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
1 similar comment
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
/test-all |
|
/test-e2e-nvidia-all |
|
Left some comments. |
|
@zeeke yep agree with you there is more stuff to do here. |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Hi @e0ne @zeeke @adrianchiris please take another look if you can I think I fixed everything |
|
@SchSeba im reviewing commits 1 by 1 so expect several comment batches from me :) |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Signed-off-by: Sebastian Sch <[email protected]>
like openstack and openshift add interfaces and mocks for better unit test Signed-off-by: Sebastian Sch <[email protected]>
for example MLX special mstconfig wrapper and create mock for unit tests Signed-off-by: Sebastian Sch <[email protected]>
…kage create also interfaces for everything so we can have better unit tests coverage Signed-off-by: Sebastian Sch <[email protected]>
SchSeba
force-pushed
the
interface_redesign
branch
from
adc6706 to
5a70e9e
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks! |
api/v1/helper.go
Outdated
| if ifaceSpec.Mtu > 0 { | ||
| mtu := ifaceSpec.Mtu | ||
| if mtu != ifaceStatus.Mtu { | ||
| log.V(2).Info("NeedUpdate(): MTU needs update", "desired", mtu, "current", ifaceStatus.Mtu) |
There was a problem hiding this comment.
function is now NeedToUpdateSriov instead of NeedUpdate for the log
| if err != nil { | ||
| setupLog.Error(err, "failed to create store manager") | ||
| return err | ||
| setupLog.Error(err, "failed to create vendorHelpers") |
There was a problem hiding this comment.
shouldn't it be hostHelpers?
| return Interface{}, fmt.Errorf("unable to find interface: %v", name) | ||
| } | ||
|
|
||
| func NeedToUpdateSriov(ifaceSpec *Interface, ifaceStatus *InterfaceExt) bool { |
There was a problem hiding this comment.
why can't this be part of pkg/host/sriov.go? It's used there by ConfigSriovInterfaces and both of them are only used by the generic plugin. Both can be called using the helpers. What am I missing?
There was a problem hiding this comment.
the reason I didn't put it in the host package is because that function doesn't do anything on the host. this function just make a API verification that is with it should be in the API helpers and not in the host package
There was a problem hiding this comment.
LGTM,
Tested this out on my setup both with switchdev and legacy sriov, seems to behave as expected.
@e0ne PTAL i see you requested changes, please approve if they were resolved.
id like to merge this one tommorow. as several PRs depend on it.
if we have non-critical comments remaining they can be addressed as follloup IMO
…aces Signed-off-by: Sebastian Sch <[email protected]>
Signed-off-by: Sebastian Sch <[email protected]>
SchSeba
force-pushed
the
interface_redesign
branch
from
5a70e9e to
d5eb696
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks everyone great work!!! |
Refactoring in [1] left the openstackContext uninitialized. [1] k8snetworkplumbingwg#553 Signed-off-by: Andrea Panattoni <[email protected]>
Refactoring in [1] left the openstackContext uninitialized. [1] k8snetworkplumbingwg#553 Signed-off-by: Andrea Panattoni <[email protected]>
Passing `/bin/sh -c xxx` to `RunCommand` requires xxx to be a single string representing a full command. Multiple arguments shifts the executed command from: ``` /bin/sh -c "cat file.txt" ``` to: ``` /bin/sh -c cat file.txt ``` This regression has been introduced by: - k8snetworkplumbingwg#553 Signed-off-by: Andrea Panattoni <[email protected]>
Passing `/bin/sh -c xxx` to `RunCommand` requires xxx to be a single string representing a full command. Multiple arguments shifts the executed command from: ``` /bin/sh -c "cat file.txt" ``` to: ``` /bin/sh -c cat file.txt ``` This regression has been introduced by: - k8snetworkplumbingwg#553 Signed-off-by: Andrea Panattoni <[email protected]>
Refactoring in [1] left the openstackContext uninitialized. [1] k8snetworkplumbingwg/sriov-network-operator#553 Signed-off-by: Andrea Panattoni <[email protected]>
Passing `/bin/sh -c xxx` to `RunCommand` requires xxx to be a single string representing a full command. Multiple arguments shifts the executed command from: ``` /bin/sh -c "cat file.txt" ``` to: ``` /bin/sh -c cat file.txt ``` This regression has been introduced by: - k8snetworkplumbingwg/sriov-network-operator#553 Signed-off-by: Andrea Panattoni <[email protected]>
Refactoring in [1] left the openstackContext uninitialized. [1] k8snetworkplumbingwg/sriov-network-operator#553 Signed-off-by: Andrea Panattoni <[email protected]>
Passing `/bin/sh -c xxx` to `RunCommand` requires xxx to be a single string representing a full command. Multiple arguments shifts the executed command from: ``` /bin/sh -c "cat file.txt" ``` to: ``` /bin/sh -c cat file.txt ``` This regression has been introduced by: - k8snetworkplumbingwg/sriov-network-operator#553 Signed-off-by: Andrea Panattoni <[email protected]>
This PR is mainly moving stuff around and creating interfaces so we can have much better coverage in our unit tests
This is the second part needed to have a parallel configuration after #552