Verify status changes on managed interfaces #530

mlguerrero12 · 2023-10-26T08:30:25Z

Users could modify the settings of VFs which have been configured by the sriov operator. This PR starts the reconciliation loop when these changes are detected in the generic plugin.

github-actions · 2023-10-26T08:30:37Z

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.

SchSeba · 2023-10-29T17:42:22Z

Hi @mlguerrero12 thanks for the PR!
question this PR is to cover a bug? asking because now everytime we will go over a much heavier function to understand that we don't need todo anything then just check the the last apply configuration

dmoessne · 2023-10-30T08:20:11Z

Hi @mlguerrero12 thanks for the PR! question this PR is to cover a bug? asking because now everytime we will go over a much heavier function to understand that we don't need todo anything then just check the the last apply configuration

Hello @SchSeba indeed this is supposed to mitigate https://issues.redhat.com/browse/OCPBUGS-22337

adrianchiris · 2023-10-30T10:06:51Z

@dmoessne can you share the details publicly ?

currently the assumption is that config daemon owns sriov configuration (unless otherwise instructed e.g via externally managed feature) so no one is expected to change the configuration.

based on that assumption comparing the states is expected to be sufficient. that said, im not against checking the system for the actual state however we need to understand IF we actually need this as it brings with it overhead in the daemon reconcile loop.

mlguerrero12 · 2023-10-30T10:53:19Z

@SchSeba, @adrianchiris, we have a customer that manually changed the mtu of some VFs that were previously configured with a sriov policy. The sriov config daemon detects these changes (via pollNicStatus which runs every 30s) and updates the status of the sriov network node state object. However, the reconciliation loop doesn't trigger because it only does when the generation field changes (this is when the spec of the sriov-network-node-state changes). We already informed the customer that they shouldn't modify the mtu without using sriov policies. I'm currently working on a PR to also make the operator react when the status doesn't match what was set in the spec.

This customer was relying on this bug. The problem was that due to a recent issue with etcd encryption, the kube-api server pods were restarted (weekly). The resource version of objects were incremented due to this, including the one for the "device-plugin-config" config map. The resource version of this config map is part of the sriov-network-node-state spec. Due to this, the sriov config daemon started its reconciliation loop. The function OnNodeStateChange of the generic plugin detected changes in the MTU and returned "needDrain = true". The node was drained and this is the reason why the customer complained. They didn't understand why the node was repeatedly draining. The Apply function of the generic plugin never reconciled the status after the draining because it only does when the spec changes. This is what I'm fixing with this PR. Regarding the cost, yes, it has an extra cost but it's just some for loops to compare some parameters of the interfaces. Nothing to be concerned about I would think but I could speed this up with some routines.

To sum up, customer shouldn't be changing the mtu without using sriov policies of VFs that are managed by the sriov operator. We identified some issues in the logic of the sriov daemon. One of them is solved with this PR.

github-actions · 2023-10-30T16:03:34Z

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 · 2023-10-30T16:05:49Z

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 · 2023-10-30T16:08:04Z

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.

mlguerrero12 · 2023-10-30T16:09:45Z

I added a commit to start the reconciliation loop of the sriov config daemon when status of interfaces are externally modified (see my previous comment). Updated title and description of PR.

github-actions · 2023-10-31T13:05:28Z

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 · 2023-10-31T13:12:54Z

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.

coveralls · 2023-10-31T13:22:46Z

Pull Request Test Coverage Report for Build 8818177298

Details

47 of 175 (26.86%) changed or added relevant lines in 8 files are covered.
9 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.3%) to 38.617%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/plugins/fake/fake_plugin.go	0	3	0.0%
pkg/plugins/k8s/k8s_plugin.go	0	3	0.0%
pkg/plugins/mellanox/mellanox_plugin.go	0	3	0.0%
pkg/plugins/virtual/virtual_plugin.go	0	3	0.0%
pkg/plugins/intel/intel_plugin.go	0	5	0.0%
pkg/plugins/mock/mock_plugin.go	4	15	26.67%
pkg/plugins/generic/generic_plugin.go	30	63	47.62%
pkg/daemon/daemon.go	13	80	16.25%

Files with Coverage Reduction	New Missed Lines	%
pkg/plugins/generic/generic_plugin.go	1	52.85%
pkg/daemon/daemon.go	3	44.16%
controllers/generic_network_controller.go	5	74.53%

Totals
Change from base Build 8817158319:	0.3%
Covered Lines:	4870
Relevant Lines:	12611

💛 - Coveralls

github-actions · 2023-11-01T17:06:59Z

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 · 2023-11-01T17:07:45Z

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 · 2023-11-01T17:09:13Z

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 · 2023-11-01T17:13:46Z

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 · 2024-04-02T10:32:29Z

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 · 2024-04-02T10:53:27Z

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 · 2024-04-02T10:54:07Z

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.

mlguerrero12 · 2024-04-02T13:41:12Z

failing test is not related

SchSeba

LGTM just small nits to add comments and we can merge this one.

nice work!

pkg/plugins/generic/generic_plugin.go

pkg/plugins/k8s/k8s_plugin.go

pkg/plugins/mellanox/mellanox_plugin.go

pkg/plugins/virtual/virtual_plugin.go

github-actions · 2024-04-04T09:24:17Z

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.

mlguerrero12 · 2024-04-04T09:26:57Z

LGTM just small nits to add comments and we can merge this one.

nice work!

Thanks @SchSeba. I added the TODO only for the generic plugin since other plugins have a different logic.

Please reopen #632. it was mistakenly closed.

github-actions · 2024-04-04T11:21:42Z

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.

SchSeba

LGTM thanks!

mlguerrero12 · 2024-04-04T11:37:58Z

@Eoghan1232 @e0ne, please have a look at this when you have time. Thanks!

github-actions · 2024-04-18T14:13:20Z

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.

ykulazhenkov

LGTM, thx

github-actions · 2024-04-24T14:09:43Z

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.

The generic plugin was applying config changes only if the desired spec of interfaces was different from the last applied spec. This logic is different from the one in OnNodeStateChange where the real status of the interfaces is used to detect changes. By removing the LastState parameter (and related code), the generic plugin will also use the real status of interfaces to decide whether to apply changes or not. The SyncNodeState function has this logic.

Users could modify the settings of VFs which have been configured by the sriov operator. This PR starts the reconciliation loop when these changes are detected in the generic plugin. Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>

Logic to check missing kernel arguments is placed in a method to be used by both OnNodeStateChange and CheckStatusChanges.

github-actions · 2024-04-24T14:11:37Z

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.

SchSeba

LGTM!

SchSeba · 2024-04-24T15:42:19Z

merging this one thanks for all the help folks!

mlguerrero12 force-pushed the removelaststate branch from d4716bf to 212a2ad Compare October 30, 2023 16:05

mlguerrero12 changed the title ~~Remove LastState parameter of GenericPlugin~~ Trigger daemon reconcilation loop when status changes Oct 30, 2023

mlguerrero12 force-pushed the removelaststate branch from 212a2ad to 9daecca Compare October 30, 2023 16:07

mlguerrero12 closed this Oct 31, 2023

mlguerrero12 reopened this Oct 31, 2023

mlguerrero12 force-pushed the removelaststate branch from 9daecca to 091f7dc Compare October 31, 2023 13:12

mlguerrero12 marked this pull request as draft November 1, 2023 10:05

mlguerrero12 force-pushed the removelaststate branch from 091f7dc to fad55a8 Compare November 1, 2023 17:06

mlguerrero12 force-pushed the removelaststate branch from fad55a8 to ae9592c Compare November 1, 2023 17:07

mlguerrero12 changed the title ~~Trigger daemon reconcilation loop when status changes~~ Verify status changes on managed interfaces Nov 1, 2023

mlguerrero12 force-pushed the removelaststate branch from ae9592c to 4e630f6 Compare November 1, 2023 17:09

mlguerrero12 force-pushed the removelaststate branch from 4e630f6 to 356043c Compare November 1, 2023 17:13

mlguerrero12 marked this pull request as ready for review November 1, 2023 17:54

mlguerrero12 force-pushed the removelaststate branch from 356043c to 98150f5 Compare November 2, 2023 09:08

mlguerrero12 force-pushed the removelaststate branch from 6c84a7f to d9911ac Compare April 2, 2024 10:32

mlguerrero12 force-pushed the removelaststate branch from d9911ac to 26d85b4 Compare April 2, 2024 10:53

mlguerrero12 force-pushed the removelaststate branch from 26d85b4 to 46cee51 Compare April 2, 2024 10:53

SchSeba approved these changes Apr 3, 2024

View reviewed changes

pkg/plugins/generic/generic_plugin.go Show resolved Hide resolved

pkg/plugins/k8s/k8s_plugin.go Show resolved Hide resolved

pkg/plugins/mellanox/mellanox_plugin.go Show resolved Hide resolved

pkg/plugins/virtual/virtual_plugin.go Show resolved Hide resolved

mlguerrero12 force-pushed the removelaststate branch from 46cee51 to 4739f7b Compare April 4, 2024 09:24

mlguerrero12 force-pushed the removelaststate branch from 4739f7b to af847fe Compare April 4, 2024 11:21

SchSeba approved these changes Apr 4, 2024

View reviewed changes

mlguerrero12 force-pushed the removelaststate branch from af847fe to 2885578 Compare April 18, 2024 14:13

ykulazhenkov approved these changes Apr 24, 2024

View reviewed changes

mlguerrero12 closed this Apr 24, 2024

mlguerrero12 reopened this Apr 24, 2024

mlguerrero12 added 4 commits April 24, 2024 16:11

Verify status changes on managed interfaces

82af069

Users could modify the settings of VFs which have been configured by the sriov operator. This PR starts the reconciliation loop when these changes are detected in the generic plugin. Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>

Abstract logic to check and load missing KArgs

21d919c

Logic to check missing kernel arguments is placed in a method to be used by both OnNodeStateChange and CheckStatusChanges.

Add e2e test for reconciliation when status changes

0d1a11a

mlguerrero12 force-pushed the removelaststate branch from 2885578 to 0d1a11a Compare April 24, 2024 14:11

SchSeba approved these changes Apr 24, 2024

View reviewed changes

SchSeba merged commit 6d367c6 into k8snetworkplumbingwg:master Apr 24, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify status changes on managed interfaces #530

Verify status changes on managed interfaces #530

mlguerrero12 commented Oct 26, 2023 •

edited

github-actions bot commented Oct 26, 2023

SchSeba commented Oct 29, 2023

dmoessne commented Oct 30, 2023

adrianchiris commented Oct 30, 2023

mlguerrero12 commented Oct 30, 2023

github-actions bot commented Oct 30, 2023

github-actions bot commented Oct 30, 2023

github-actions bot commented Oct 30, 2023

mlguerrero12 commented Oct 30, 2023

github-actions bot commented Oct 31, 2023

github-actions bot commented Oct 31, 2023

coveralls commented Oct 31, 2023 •

edited

github-actions bot commented Nov 1, 2023

github-actions bot commented Nov 1, 2023

github-actions bot commented Nov 1, 2023

github-actions bot commented Nov 1, 2023

github-actions bot commented Apr 2, 2024

github-actions bot commented Apr 2, 2024

github-actions bot commented Apr 2, 2024

mlguerrero12 commented Apr 2, 2024

SchSeba left a comment

github-actions bot commented Apr 4, 2024

mlguerrero12 commented Apr 4, 2024

github-actions bot commented Apr 4, 2024

SchSeba left a comment

mlguerrero12 commented Apr 4, 2024

github-actions bot commented Apr 18, 2024

ykulazhenkov left a comment

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

SchSeba left a comment

SchSeba commented Apr 24, 2024

Verify status changes on managed interfaces #530

Verify status changes on managed interfaces #530

Conversation

mlguerrero12 commented Oct 26, 2023 • edited

github-actions bot commented Oct 26, 2023

SchSeba commented Oct 29, 2023

dmoessne commented Oct 30, 2023

adrianchiris commented Oct 30, 2023

mlguerrero12 commented Oct 30, 2023

github-actions bot commented Oct 30, 2023

github-actions bot commented Oct 30, 2023

github-actions bot commented Oct 30, 2023

mlguerrero12 commented Oct 30, 2023

github-actions bot commented Oct 31, 2023

github-actions bot commented Oct 31, 2023

coveralls commented Oct 31, 2023 • edited

Pull Request Test Coverage Report for Build 8818177298

Details

💛 - Coveralls

github-actions bot commented Nov 1, 2023

github-actions bot commented Nov 1, 2023

github-actions bot commented Nov 1, 2023

github-actions bot commented Nov 1, 2023

github-actions bot commented Apr 2, 2024

github-actions bot commented Apr 2, 2024

github-actions bot commented Apr 2, 2024

mlguerrero12 commented Apr 2, 2024

SchSeba left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 4, 2024

mlguerrero12 commented Apr 4, 2024

github-actions bot commented Apr 4, 2024

SchSeba left a comment

Choose a reason for hiding this comment

mlguerrero12 commented Apr 4, 2024

github-actions bot commented Apr 18, 2024

ykulazhenkov left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

SchSeba left a comment

Choose a reason for hiding this comment

SchSeba commented Apr 24, 2024

mlguerrero12 commented Oct 26, 2023 •

edited

coveralls commented Oct 31, 2023 •

edited