Kernel params set by the config daemon should be verified #486

wizhaoredhat · 2023-08-03T22:40:59Z

In the case of vfio-pci, the config daemon might not notice it has to reboot to add kernel parameters. This could happen if the vfio driver state was updated but the config daemon was interrupted (e.g policy was removed and readded quickly).

This could lead to a pending change to the kernel parameters via grubby or os-tree but without a reboot, these changes wouldn't be applied.

This change introduces a map to hold the desired kernel parameters that the config daemon wishes to apply. This map is then validated against "/proc/cmdline" to see if the desired kernel parameters are actually applied. If not then there will be a subsequent attempt to apply these parameters.

We need to make sure that the kernel parameters are set, thus we should pass the errors up to the config daemon instead of silently consuming the errors when setting kernel parameters.

github-actions · 2023-08-03T22:41:11Z

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.

wizhaoredhat · 2023-08-03T22:41:35Z

/cc @SchSeba
/cc @zeeke
/cc @e0ne
/cc @adrianchiris
/cc @zshi-redhat

wizhaoredhat · 2023-08-03T22:47:12Z

/cc @lmilleri

coveralls · 2023-08-03T22:48:09Z

Pull Request Test Coverage Report for Build 6502105920

24 of 89 (26.97%) changed or added relevant lines in 2 files are covered.
12 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-0.2%) to 25.039%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/utils/utils.go	0	17	0.0%
pkg/plugins/generic/generic_plugin.go	24	72	33.33%

Files with Coverage Reduction	New Missed Lines	%
controllers/sriovnetwork_controller.go	2	71.32%
api/v1/helper.go	3	42.04%
pkg/plugins/generic/generic_plugin.go	3	42.62%
controllers/sriovibnetwork_controller.go	4	68.22%

Totals
Change from base Build 6501142942:	-0.2%
Covered Lines:	2238
Relevant Lines:	8938

💛 - Coveralls

wizhaoredhat · 2023-08-04T19:20:07Z

/hold

wizhaoredhat · 2023-08-04T21:30:37Z

@bn222

pkg/plugins/generic/generic_plugin.go

bn222 · 2023-08-07T14:44:06Z

I don't think this is fixing the issue. We aren't reconciling the state of the system to a desired state whenever the current state is different. What we want is to trigger setting the desired kargs at any point when they are set to something else then we expect.

Reference for this: OCPBUGS-16909

It's a step in the good direction though, by performing the necessary check when we are making a change. We just need to have this check be executed more frequently.

pkg/plugins/generic/generic_plugin.go

pkg/utils/utils.go

bn222 · 2023-08-08T12:50:47Z

We have one remaining situation to fix due to how enable-kargs.sh works. Let's say we've run enable-kargs.sh, and we returned non-zero, i.e. we made a change and we need to reboot. Now, before we actually get to the point of the reboot, the daemon is restarted. Now, assume again execution gets to the point of running enable-kargs.sh but this time, since rpm-ostree kargs contains the pending kargs, it will return 0 since from the perspective of rpm-ostree we are good and we won't reboot. We need to fix this on line 10 in enable-kargs.sh

pkg/utils/utils.go

wizhaoredhat · 2023-08-08T23:10:04Z

We have one remaining situation to fix due to how enable-kargs.sh works. Let's say we've run enable-kargs.sh, and we returned non-zero, i.e. we made a change and we need to reboot. Now, before we actually get to the point of the reboot, the daemon is restarted. Now, assume again execution gets to the point of running enable-kargs.sh but this time, since rpm-ostree kargs contains the pending kargs, it will return 0 since from the perspective of rpm-ostree we are good and we won't reboot. We need to fix this on line 10 in enable-kargs.sh

@bn222
I thought the logic works in your case. But maybe I misunderstood.

        if [[ $args != *${t}* ]];then  // This would be true in your case because cmdLine does not have the kernel parameter (we made a change and we didn't reboot). This will always be true because we didn't reboot.
            ...
            let ret++   // We increment this value and return it.
        fi

If ret > 0 then we will be marked for reboot.

wizhaoredhat · 2023-08-08T23:21:50Z

I don't think this is fixing the issue. We aren't reconciling the state of the system to a desired state whenever the current state is different. What we want is to trigger setting the desired kargs at any point when they are set to something else then we expect.

Reference for this: OCPBUGS-16909

It's a step in the good direction though, by performing the necessary check when we are making a change. We just need to have this check be executed more frequently.

Yes, I agree this is an issue. I was thinking of calling it earlier in nodeStateSyncHandler.

bn222 · 2023-08-09T06:53:15Z

We have one remaining situation to fix due to how enable-kargs.sh works. Let's say we've run enable-kargs.sh, and we returned non-zero, i.e. we made a change and we need to reboot. Now, before we actually get to the point of the reboot, the daemon is restarted. Now, assume again execution gets to the point of running enable-kargs.sh but this time, since rpm-ostree kargs contains the pending kargs, it will return 0 since from the perspective of rpm-ostree we are good and we won't reboot. We need to fix this on line 10 in enable-kargs.sh

@bn222 I thought the logic works in your case. But maybe I misunderstood.
        if [[ $args != *${t}* ]];then                                                                             // This would be true in your case because cmdLine does not have the kernel parameter (we made a change and we didn't reboot). This will always be true because we didn't reboot.
            ...
            let ret++                                                                                                        // We increment this value and return it.
        fi
If ret > 0 then we will be marked for reboot.

Yes, that happens in normal execution. What happens when we kill the pod immediately after we are marked for reboot? I think we will run through the whole logic from start again, but this time we will not mark for reboot, and we will just wait. iow, I believe there is an edge case due to non-idempotency.

SchSeba · 2023-08-09T11:43:35Z

Hi folks,

let me add some ideas here.

we can improve this one with a simple fix I think.
instead of checking the rpm-ostree command return to see if we change it or not we just need to look on what is really in the system by doing cat /proc/cmdline

and I don't think we need to revalidate this every few seconds it's a waster of resource as this variable is loaded or unloaded after reboot you can't do it when the system is already running.

meaning if the user disable it but didn't reboot all good we still have it.
and after reboot the operator will see that the variable is not there and will apply it and reboot.

so again I think the only change needed is to check if we need to reboot by looking on the source of true that is /proc/cmdline and not the output of the rpm-ostree or grubby

bn222 · 2023-08-09T12:42:55Z

that is correct and also what I'm proposing. No need to recheck that every time. A reboot implies a start of execution from the beginning, but we do need to check /proc/cmdline instead.

While at it, please move the .sh to .go (maybe as a separate patch). I don't see any need to use .sh here.

bn222 · 2023-08-09T13:20:39Z

/lgtm

bn222 · 2023-08-09T13:20:50Z

discussed with William offline. This is good to go from my side.

wizhaoredhat · 2023-08-15T21:03:22Z

/unhold

github-actions · 2023-08-15T21:19:35Z

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.

wizhaoredhat · 2023-08-15T21:20:07Z

/hold cancel

bn222 · 2023-08-21T15:45:08Z

LGTM from me on this. We would want to have a separate go function that does the configuring and another that checks if the config needs to be done), but we will address that in a PR that cleans up the .sh file (by moving that functionality into .go) and separates out the two tasks (check & set).

wizhaoredhat · 2023-08-21T15:51:25Z

@adrianchiris @e0ne Could you PTAL, Thanks!

pkg/plugins/generic/generic_plugin.go

github-actions · 2023-09-28T14:02:47Z

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-09-28T14:53: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.

wizhaoredhat · 2023-09-28T16:13:09Z

/test-all

github-actions · 2023-09-28T17:54:40Z

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.

pkg/utils/utils.go

github-actions · 2023-09-30T02:08:30Z

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.

bn222 · 2023-10-02T07:05:10Z

LGTM

github-actions · 2023-10-02T13:33:53Z

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.

wizhaoredhat · 2023-10-02T15:07:13Z

@adrianchiris Please take a look, much appreciated!

Eoghan1232 · 2023-10-12T21:32:16Z

@wizhaoredhat I see the CI failed, could you take a look into that? I will give this PR a quick review tomorrow once I am online

github-actions · 2023-10-12T21:43: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.

In the case of vfio-pci, the config daemon might not notice it has to reboot to add kernel arguments. This could happen if the vfio driver state was updated but the config daemon was interrupted (e.g policy was removed and readded quickly). This could lead to a pending change to the kernel arguments via grubby or os-tree but without a reboot, these changes wouldn't be applied. This change introduces a map to hold the desired kernel arguments that the config daemon wishes to apply. This map is then validated against "/proc/cmdline" to see if the desired kernel arguments are actually applied. If not then there will be a subsequent attempt to apply these arguments. We need to make sure that the kernel arguments are set, thus we should pass the errors up to the config daemon instead of silently consuming the errors when setting kernel arguments. Signed-off-by: William Zhao <wizhao@redhat.com>

github-actions · 2023-10-12T23:42: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.

wizhaoredhat · 2023-10-13T01:17:28Z

/test-all

When creating VFs via sysfs sriov_numvfs, there is a chance of getting the error syscall.ENOMEM "cannot allocate memory". This could occur when the BIOS is not providing enough MMIO space for VFs. A solution is to reallocate the MMIO space via "pci=realloc". Signed-off-by: William Zhao <wizhao@redhat.com>

github-actions · 2023-10-13T01:44: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.

Eoghan1232 · 2023-10-13T08:04:39Z

pkg/plugins/generic/generic_plugin.go

@@ -285,33 +337,39 @@ func (p *GenericPlugin) needDrainNode(desired sriovnetworkv1.Interfaces, current
 	return
 }

-func needRebootIfVfio(state *sriovnetworkv1.SriovNetworkNodeState, driverMap DriverStateMapType) (needReboot bool) {
-	driverState := driverMap[Vfio]
+func (p *GenericPlugin) addVfioDesiredKernelArg(state *sriovnetworkv1.SriovNetworkNodeState) {


nit: addVfioDesiredKernelArg -> addVfioDesiredKernelArgs to keep it the same as other methods ? WDYT.
Although, we are only adding a single arg here....

I prefer singular. It's only adding 1 arg.

I also prefer it being singular. Let's keep it as is. Thanks for reviewing!

its adding two, what am i missing ?

L343, L344

Sorry I grouped IOMMU as singular. There is infact Intel IOMMU and the general IOMMU. However this is a small NIT on naming.

Eoghan1232

overall, PR lgtm.
Can you confirm CI failure is due to CI itself and not this PR?

zeeke · 2023-10-13T16:52:13Z

overall, PR lgtm. Can you confirm CI failure is due to CI itself and not this PR?

k8s and ocp jobs also fail in #518. Not related to this job

adrianchiris · 2023-10-15T08:32:57Z

pkg/plugins/generic/generic_plugin.go

+		set := utils.IsKernelArgsSet(kargs, desiredKarg)
+		if !set {
+			if attempted {
+				glog.V(2).Infof("generic-plugin syncDesiredKernelArgs(): previously attempted to set kernel arg %s", desiredKarg)


@wizhaoredhat if you previously attempted to set the arg, it means setKernlelArg succeeded.
then why do we need to set it again ? shouldnt we "continue" here ?

This captures the case where we hit this case, but either the kernel arg wasn't set properly or if the reboot did not occur for some reason. This results in IsKernelArgsSet returning false even though it was set before. So we should try again.

github-actions bot requested a review from zshi-redhat August 3, 2023 22:41

github-actions bot added the hold label Aug 4, 2023

SchSeba reviewed Aug 6, 2023

View reviewed changes

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

bn222 reviewed Aug 7, 2023

View reviewed changes

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

bn222 reviewed Aug 7, 2023

View reviewed changes

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

bn222 reviewed Aug 8, 2023

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

bn222 reviewed Aug 8, 2023

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

github-actions bot added the lgtm label Aug 9, 2023

github-actions bot removed the hold label Aug 15, 2023

wizhaoredhat force-pushed the add_pci_realloc branch from c52c3cc to 0d2a741 Compare August 23, 2023 18:20

bn222 reviewed Sep 28, 2023

View reviewed changes

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

wizhaoredhat force-pushed the add_pci_realloc branch from 27b8785 to 335d64d Compare September 28, 2023 14:02

wizhaoredhat force-pushed the add_pci_realloc branch from 335d64d to 6cee22e Compare September 28, 2023 14:53

wizhaoredhat force-pushed the add_pci_realloc branch from 6cee22e to 0cd892a Compare September 28, 2023 17:54

bn222 reviewed Sep 29, 2023

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

wizhaoredhat force-pushed the add_pci_realloc branch from 0cd892a to 07eb354 Compare September 30, 2023 02:08

wizhaoredhat force-pushed the add_pci_realloc branch from 07eb354 to fa46710 Compare October 2, 2023 13:33

wizhaoredhat force-pushed the add_pci_realloc branch from fa46710 to 882dee0 Compare October 12, 2023 21:43

wizhaoredhat force-pushed the add_pci_realloc branch from 882dee0 to 99e5aa9 Compare October 12, 2023 23:42

wizhaoredhat force-pushed the add_pci_realloc branch from 99e5aa9 to 3eaac7e Compare October 13, 2023 01:44

Eoghan1232 reviewed Oct 13, 2023

View reviewed changes

Eoghan1232 approved these changes Oct 13, 2023

View reviewed changes

bn222 merged commit 49fff50 into k8snetworkplumbingwg:master Oct 13, 2023
10 of 11 checks passed

adrianchiris reviewed Oct 15, 2023

View reviewed changes

Kernel params set by the config daemon should be verified #486

Kernel params set by the config daemon should be verified #486

Conversation

wizhaoredhat commented Aug 3, 2023 • edited Loading

github-actions bot commented Aug 3, 2023

wizhaoredhat commented Aug 3, 2023

wizhaoredhat commented Aug 3, 2023

coveralls commented Aug 3, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6502105920

💛 - Coveralls

wizhaoredhat commented Aug 4, 2023

wizhaoredhat commented Aug 4, 2023

bn222 commented Aug 7, 2023

bn222 commented Aug 8, 2023

wizhaoredhat commented Aug 8, 2023 • edited Loading

wizhaoredhat commented Aug 8, 2023

bn222 commented Aug 9, 2023

SchSeba commented Aug 9, 2023

bn222 commented Aug 9, 2023

bn222 commented Aug 9, 2023

bn222 commented Aug 9, 2023

wizhaoredhat commented Aug 15, 2023

github-actions bot commented Aug 15, 2023

wizhaoredhat commented Aug 15, 2023

bn222 commented Aug 21, 2023

wizhaoredhat commented Aug 21, 2023

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

wizhaoredhat commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 30, 2023

bn222 commented Oct 2, 2023

github-actions bot commented Oct 2, 2023

wizhaoredhat commented Oct 2, 2023

Eoghan1232 commented Oct 12, 2023 • edited Loading

github-actions bot commented Oct 12, 2023

github-actions bot commented Oct 12, 2023

wizhaoredhat commented Oct 13, 2023

github-actions bot commented Oct 13, 2023

Eoghan1232 Oct 13, 2023

Choose a reason for hiding this comment

bn222 Oct 13, 2023

Choose a reason for hiding this comment

wizhaoredhat Oct 13, 2023

Choose a reason for hiding this comment

adrianchiris Oct 15, 2023

Choose a reason for hiding this comment

wizhaoredhat Oct 16, 2023

Choose a reason for hiding this comment

Eoghan1232 left a comment

Choose a reason for hiding this comment

zeeke commented Oct 13, 2023

adrianchiris Oct 15, 2023 • edited Loading

Choose a reason for hiding this comment

wizhaoredhat Oct 16, 2023

Choose a reason for hiding this comment

wizhaoredhat commented Aug 3, 2023 •

edited

Loading

coveralls commented Aug 3, 2023 •

edited

Loading

wizhaoredhat commented Aug 8, 2023 •

edited

Loading

Eoghan1232 commented Oct 12, 2023 •

edited

Loading

adrianchiris Oct 15, 2023 •

edited

Loading