sriov-network-operator
sriov-network-operator copied to clipboard
k8snetworkplumbingwg
Add more checks on generic plugin to discover discrepancies from the desired state
Summary
This PR does the following things:
- Adds additional test coverage for
OnNodeStateChange()function to indicate when we are draining the node - Adjusts
NeedToUpdateSriov()helper function to ensure that:- VF MAC Address is set when we are in Ethernet mode
- VF GUID is set when we are in Ethernet mode and RDMA is enabled
- VF GUID is set when we are in Infiniband mode
- PF Link is up
- (API Change) Adds necessary fields in the
SriovNetworkNodeStatestruct to facilitate the above
Partial Reconciliation
Today we reconcile on .spec change since we skip reconciliation if the generation of the object is the same as the object that was last reconciled successfully. Therefore, changes in .status, like what we try to detect with that PR, are not reconciled unless the .spec changes when the config daemon runs continuously (i.e. no restart). On daemon restart, the .status field will be reconciled the first time until the last successfully reconciled generation is saved in memory.
Decisions
- Decided to add tests on the
OnNodeStateChange()instead ofNeedToUpdateSriov()because that one looks to be the most impactful for the whole system (i.e. decides whether to drain). I can add on bothApply()andOnNodeStateChange()if we choose to split theNeedToUpdateSriov()(see open questions below). - Decided to just look for Node GUID and not the Port GUID since we expect both of them to be populated together on bind/unbind. This is the case because we parse the GUID from the RDMA link, and the node GUID is populated there on bind/unbind.
Open questions
- [ ] Is it enough to partially reconcile or do we need to change the logic to take into account changes on
.status(i.e. full reconciliation of changes that the controller does to the system)? (relatively big change in the operator behaviour I suppose) - [ ] Do we need to drain on any of the discrepancies we find? (e.g. link is not up). If not, we will need to adjust the function that is used in
Apply()andOnNodeStateChange()to be something different thanNeedToUpdateSriov(). - [ ] Are there additional end to end tests to be added? I would appreciate if you can point out the place I should be putting those (if we want to test end to end).
- [ ] Do we prefer getting info by reading directly
/sys/class/net/*or use netlink? I see both approaches:- https://github.com/k8snetworkplumbingwg/sriov-network-operator/blob/09c987aaa8e5b1913f3306b3f889209e06adb8a1/pkg/host/network.go#L195-L205
- https://github.com/k8snetworkplumbingwg/sriov-network-operator/blob/09c987aaa8e5b1913f3306b3f889209e06adb8a1/pkg/host/sriov.go#L618-L635 Based on the answer I will use or discard https://github.com/k8snetworkplumbingwg/sriov-network-operator/pull/587/commits/d18aae1e1e975cdf9b5fca18172a1d0cafc9ca0e
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Pull Request Test Coverage Report for Build 9206688050
Details
- 54 of 127 (42.52%) changed or added relevant lines in 7 files are covered.
- 6 unchanged lines in 2 files lost coverage.
- Overall coverage increased (+0.06%) to 39.655%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| pkg/host/internal/lib/netlink/netlink.go | 0 | 6 | 0.0% |
| pkg/host/internal/sriov/sriov.go | 1 | 12 | 8.33% |
| pkg/host/internal/network/network.go | 19 | 35 | 54.29% |
| pkg/helper/mock/mock_helper.go | 0 | 20 | 0.0% |
| pkg/host/mock/mock_host.go | 0 | 20 | 0.0% |
| <!-- | Total: | 54 | 127 |
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| controllers/drain_controller.go | 1 | 68.06% |
| controllers/generic_network_controller.go | 5 | 74.53% |
| <!-- | Total: | 6 |
| Totals | |
|---|---|
| Change from base Build 9203023396: | 0.06% |
| Covered Lines: | 5175 |
| Relevant Lines: | 13050 |
💛 - Coveralls
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Adding in my feedback:
Today we reconcile on .spec change since we skip reconciliation if the generation of the object is the same as the object that was last reconciled successfully. Therefore, changes in .status, like what we try to detect with that PR, are not reconciled unless the .spec changes when the config daemon runs continuously (i.e. no restart). On daemon restart, the .status field will be reconciled the first time until the last successfully reconciled generation is saved in memory.
PR #530 aims to tackle that
Do we need to drain on any of the discrepancies we find? (e.g. link is not up). If not, we will need to adjust the function that is used in Apply() and OnNodeStateChange() to be something different than NeedToUpdateSriov().
I see this as an optimization on top of what we have, mtu might also not need drain but we do it.
Do we prefer getting info by reading directly /sys/class/net/* or use netlink? I see both approaches:
I prefer netlink :)
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.
Thanks for your PR, To run vendors CIs use one of:
/test-all: To run all tests for all vendors./test-e2e-all: To run all E2E tests for all vendors./test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.
To skip the vendors CIs use one of:
/skip-all: To skip all tests for all vendors./skip-e2e-all: To skip all E2E tests for all vendors./skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.