Make kargs.sh work with systemd service and for Ubuntu #847

e0ne · 2025-02-18T13:50:38Z

No description provided.

github-actions · 2025-02-18T13:50:50Z

Thanks for your PR,
To run vendors CIs, Maintainers can 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, Maintainers can 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/plugins/generic/generic_plugin.go

ykulazhenkov · 2025-02-20T09:38:53Z

pkg/plugins/generic/generic_plugin.go

 }

 // editKernelArg Tries to add the kernel args via ostree or grubby.
 func editKernelArg(helper helper.HostHelpersInterface, mode, karg string) error {


Will this logic really work in the systemd mode? How it will behaves?

Reboot

systemd service exec

systemd service edited the grub config

Host booted

sriov-daemon pod starts

sriov-daemon detects that running kernel is missing required args

host reboot again

...

stable state

Is the sequence above describes the flow properly?

in systemd mode kernel arg are configured by sriov-service so it doesn't do reboot loop

@e0ne wont kernel args be applied only after reboot ?
hence two reboots will be required for this case ?

(unless we set kargs in daemon even in systemd mode)

ykulazhenkov · 2025-02-20T09:45:08Z

bindata/manifests/daemon/daemonset.yaml

            - /bin/bash
            - -c
-            - mkdir -p /host/var/lib/sriov/ && cp /usr/bin/sriov-network-config-daemon /host/var/lib/sriov/sriov-network-config-daemon && chcon -t bin_t /host/var/lib/sriov/sriov-network-config-daemon | true # Allow systemd to run the file, use pipe true to not failed if the system doesn't have selinux or apparmor enabled
+            - mkdir -p /host/var/lib/sriov/ && cp /usr/bin/sriov-network-config-daemon /host/var/lib/sriov/sriov-network-config-daemon && chcon -t bin_t /host/var/lib/sriov/sriov-network-config-daemon | true  && cp /bindata/scripts/kargs.sh /host/var/lib/sriov/kargs.sh && chcon -t bin_t /host/var/lib/sriov/kargs.sh | true # Allow systemd to run the file, use pipe true to not failed if the system doesn't have selinux or apparmor enabled


I think the original command has issue. Should it has || (or) instead of | (pipe)? The same question for the changes you added.

yes, we need to fix it and we should we need to think how to simplify this line. thanks for pointing me on this!

just tested again: we need only one | to not fail if chcon execution failed

Yes, this will work because | will mask errors from the previous command (by default, if set -o pipefail is not set). The || looks cleaner to me, but I'm okay to keep the current implementation.

adrianchiris · 2025-03-20T11:19:36Z

bindata/scripts/kargs.sh

-args=$(chroot /host/ cat /proc/cmdline)

-IS_OS_UBUNTU=true; [[ "$(chroot /host/ grep -i ubuntu /etc/os-release -c)" == "0" ]] && IS_OS_UBUNTU=false
+if [[ -d "/bindata/scripts" ]];then


nit :add comment, its non obvious that in this case we run in from config daemon and the other by system service.

can you check the comment again. it might have been dropped by a git rebase

I don't see the comment also

bindata/scripts/kargs.sh

adrianchiris

overall LGTM. some minor nits.

coveralls · 2025-03-31T09:40:18Z

Pull Request Test Coverage Report for Build 19567899770

Details

5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.08%) to 62.155%

Files with Coverage Reduction	New Missed Lines	%
controllers/drain_controller_helper.go	1	67.43%

Totals
Change from base Build 19534412227:	0.08%
Covered Lines:	8775
Relevant Lines:	14118

💛 - Coveralls

zeeke · 2025-04-29T15:52:08Z

bindata/manifests/daemon/daemonset.yaml

              cp /usr/bin/sriov-network-config-daemon /host/var/lib/sriov/sriov-network-config-daemon
              chcon -t bin_t /host/var/lib/sriov/sriov-network-config-daemon || true # Allow systemd to run the file, use pipe true to not failed if the system doesn't have selinux or apparmor enabled
+              cp /bindata/scripts/kargs.sh /host/var/lib/sriov/kargs.sh
+              chcon -t bin_t /host/var/lib/sriov/kargs.sh | true # Allow systemd to run the file, use pipe true to not failed if the system doesn't have selinux or apparmor enabled


nit: mind making congruent to L127?

Suggested change

chcon -t bin_t /host/var/lib/sriov/kargs.sh | true # Allow systemd to run the file, use pipe true to not failed if the system doesn't have selinux or apparmor enabled

chcon -t bin_t /host/var/lib/sriov/kargs.sh || true # Allow systemd to run the file, use pipe true to not failed if the system doesn't have selinux or apparmor enabled

e0ne · 2025-07-04T09:48:44Z

@SchSeba could you please review this PR?

ykulazhenkov · 2025-07-04T10:04:53Z

bindata/scripts/kargs.sh

 fi

-echo $ret
+echo $ret


nit: no new line at the end

adrianchiris · 2025-07-14T06:24:29Z

bindata/scripts/kargs.sh

+                fi
+            done
+
+            if [ $found == false ];then


what will happen if we have a karg already set with a certain value and we want to change to a different value ?

example:
current grub cmdline args have: ib_core.netns_mode=1 then we want to set (using kargs.sh) ib_core.netns_mode=0

in this case we will end up with both set in grub cmdline IIUC since we compare the full key=val

i think we have the same issue with rpm-ostree as well as we use only append and delete.

imo we need to seach for the key of the karg first, if exists and its value is different then we remove and readd with new value (rpm-ostree has --replace flag)

This can be addressed as a separate PR though, as we have the same issue with rpm-ostree as well.

Let's address it in a follow-up PR both for Ubuntu and RHEL-based OSes

+1 from my side to handle it on a following up PR.
please if you can just open an issue so we don't forget about it

SchSeba · 2025-07-22T10:51:22Z

Hi @e0ne,

I must say I don't follow this change.
we copy the kargs.sh into the host that is fine so when the service runs it will be able to call it.

but inside the script we still have pointers to /host and that will not work.
I think we need to add a check to see if we are inside the container or not.
maybe try to run a chroot and if that failed we know we are on the host and we need to changes stuff like cp /host/${grub_config}

/hold

e0ne · 2025-08-01T08:43:38Z

Hi @e0ne,

I must say I don't follow this change. we copy the kargs.sh into the host that is fine so when the service runs it will be able to call it.

but inside the script we still have pointers to /host and that will not work. I think we need to add a check to see if we are inside the container or not. maybe try to run a chroot and if that failed we know we are on the host and we need to changes stuff like cp /host/${grub_config}

/hold

hi @SchSeba. I updated my PR with a fix

rbakhtaraev · 2025-09-08T19:48:37Z

Hi guys, what about adding debian support?
Looks like debian is configured similarly :)

e0ne · 2025-09-10T09:01:15Z

Hi guys, what about adding debian support? Looks like debian is configured similarly :)

We don't test Debian but I suppose it should work. We'll be glad to review PR with Debian support

SchSeba · 2025-10-15T18:24:58Z

pkg/plugins/generic/generic_plugin.go

+	if _, err := os.Stat(consts.Host); errors.Is(err, os.ErrNotExist) {
+		script = systemdScriptsPath
+	}
+	_, _, err := helper.RunCommand("/bin/bash", script, mode, karg)


can we stay with sh here as before? or swtich the other calls to bash but please lets be consistent

an updated scripts doesn't work with sh beacause it uses bash. do we want ti switch to bash everywhere?

SchSeba · 2025-10-15T18:25:28Z

pkg/daemon/daemon_test.go

 		// general
 		hostHelper.EXPECT().Chroot(gomock.Any()).Return(func() error { return nil }, nil).AnyTimes()
 		hostHelper.EXPECT().RunCommand("/bin/sh", gomock.Any(), gomock.Any(), gomock.Any()).Return("", "", nil).AnyTimes()
+		hostHelper.EXPECT().RunCommand("/bin/bash", gomock.Any(), gomock.Any(), gomock.Any()).Return("", "", nil).AnyTimes()


please check the comment below about this

SchSeba · 2025-10-15T18:28:47Z

bindata/scripts/kargs.sh

 ret=0
 args=$(chroot /host/ cat /proc/cmdline)

 IS_OS_UBUNTU=true; [[ "$(chroot /host/ grep -i ubuntu /etc/os-release -c)" == "0" ]] && IS_OS_UBUNTU=false


question how this one will work when we run on systemd mode?
the /host folder doesn't exist in the host itself only inside the container.

SchSeba · 2025-10-15T18:28:50Z

bindata/scripts/kargs.sh

 if ${IS_OS_UBUNTU} ; then
+    grub_config="/etc/default/grub"
+    # Operate on the copy of the file
+    cp /host/${grub_config} /tmp/grub


question how this one will work when we run on systemd mode?
the /host folder doesn't exist in the host itself only inside the container.

rbakhtaraev · 2025-10-28T06:20:52Z

Hi guys, what about adding debian support? Looks like debian is configured similarly :)

We don't test Debian but I suppose it should work. We'll be glad to review PR with Debian support

I've tested this patch on Debian by editing kargs.sh something like that:

(chroot /host/ grep -iE "ubuntu|debian" /etc/os-release -c)

Everything works fine, is it really necessary to support Debian within a another request?

SchSeba · 2025-11-02T19:50:21Z

bindata/scripts/kargs.sh

@@ -1,63 +1,144 @@
 #!/bin/bash
+# Copyright 2025 sriov-network-device-plugin authors


I think this copy right is wrong no?

SchSeba · 2025-11-02T19:51:06Z

bindata/scripts/kargs.sh

-args=$(chroot /host/ cat /proc/cmdline)

-IS_OS_UBUNTU=true; [[ "$(chroot /host/ grep -i ubuntu /etc/os-release -c)" == "0" ]] && IS_OS_UBUNTU=false
+if [[ -d "/bindata/scripts" ]];then


I don't see the comment also

SchSeba · 2025-11-02T19:52:26Z

bindata/scripts/kargs.sh

 if ${IS_OS_UBUNTU} ; then
+    grub_config="/etc/default/grub"
+    # Operate on the copy of the file
+    cp ${chroot_path}/${grub_config} /tmp/grub


remote the /
${chroot_path}/${grub_config} -> ${chroot_path}${grub_config}

SchSeba · 2025-11-02T19:53:22Z

bindata/scripts/kargs.sh

+
+    if [ $ret -ne 0 ];then
+        # Update grub only if there were changes
+        cp /tmp/grub ${chroot_path}/${grub_config}


same here lets remove the /

SchSeba · 2025-11-02T19:55:23Z

pkg/plugins/generic/generic_plugin.go

+	if _, err := os.Stat(consts.Host); errors.Is(err, os.ErrNotExist) {
+		script = systemdScriptsPath
+	}
+	_, _, err := helper.RunCommand("/bin/bash", script, mode, karg)


can we just pass here if we are in chroot or not (boolean) and just use it in the script? this way we can remove the not clear check if [[ -d "/bindata/scripts" ]];then?

Signed-off-by: Ivan Kolodiazhnyi <[email protected]>

Many customers rely on the Ubuntu operating system, and the ability to automatically update GRUB parameters is essential. Signed-off-by: Aleksey Senin <[email protected]>

e0ne force-pushed the ubuntu-kargs branch 4 times, most recently from 5ab8f47 to 8d58511 Compare February 18, 2025 15:04

e0ne marked this pull request as draft February 18, 2025 15:34

e0ne force-pushed the ubuntu-kargs branch 2 times, most recently from 1d136d7 to 508a194 Compare February 19, 2025 13:06

e0ne marked this pull request as ready for review February 19, 2025 13:06

e0ne force-pushed the ubuntu-kargs branch 3 times, most recently from aea78cf to 659a0ab Compare February 20, 2025 08:46

ykulazhenkov reviewed Feb 20, 2025

View reviewed changes

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

ykulazhenkov reviewed Feb 20, 2025

View reviewed changes

e0ne force-pushed the ubuntu-kargs branch from 659a0ab to 438abf3 Compare February 20, 2025 13:59

ykulazhenkov approved these changes Mar 20, 2025

View reviewed changes

adrianchiris reviewed Mar 20, 2025

View reviewed changes

bindata/scripts/kargs.sh Show resolved Hide resolved

adrianchiris reviewed Mar 20, 2025

View reviewed changes

e0ne force-pushed the ubuntu-kargs branch 3 times, most recently from b43840c to 6a5d7e8 Compare March 24, 2025 12:04

adrianchiris approved these changes Mar 24, 2025

View reviewed changes

e0ne force-pushed the ubuntu-kargs branch 3 times, most recently from 04df2c0 to c5ffada Compare March 31, 2025 09:32

e0ne force-pushed the ubuntu-kargs branch from c5ffada to 6fa08a6 Compare March 31, 2025 14:19

e0ne requested a review from SchSeba April 3, 2025 12:21

e0ne force-pushed the ubuntu-kargs branch 2 times, most recently from ae0761d to 9d4fd60 Compare April 29, 2025 06:54

zeeke reviewed Apr 29, 2025

View reviewed changes

e0ne added the hold label May 13, 2025

e0ne force-pushed the ubuntu-kargs branch from 9d4fd60 to 966d6f4 Compare June 2, 2025 19:10

e0ne removed the hold label Jun 2, 2025

e0ne mentioned this pull request Jun 2, 2025

Add Ubuntu support #890

Open

ykulazhenkov approved these changes Jul 4, 2025

View reviewed changes

ykulazhenkov reviewed Jul 4, 2025

View reviewed changes

bindata/scripts/kargs.sh

fi

echo $ret

echo $ret

Copy link

Collaborator

ykulazhenkov Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no new line at the end

adrianchiris reviewed Jul 14, 2025

View reviewed changes

github-actions bot added the hold label Jul 22, 2025

SchSeba reviewed Oct 15, 2025

View reviewed changes

e0ne force-pushed the ubuntu-kargs branch from 966d6f4 to fbab698 Compare October 16, 2025 13:19

github-actions bot added the tests label Oct 16, 2025

SchSeba reviewed Nov 2, 2025

View reviewed changes

e0ne and others added 2 commits November 21, 2025 12:41

Make kargs.sh work with systemd service and for Ubuntu

2c3607c

Signed-off-by: Ivan Kolodiazhnyi <[email protected]>

Add Ubuntu support

644525a

Many customers rely on the Ubuntu operating system, and the ability to automatically update GRUB parameters is essential. Signed-off-by: Aleksey Senin <[email protected]>

e0ne force-pushed the ubuntu-kargs branch from fbab698 to 644525a Compare November 21, 2025 10:41

	chcon -t bin_t /host/var/lib/sriov/kargs.sh \| true # Allow systemd to run the file, use pipe true to not failed if the system doesn't have selinux or apparmor enabled
	chcon -t bin_t /host/var/lib/sriov/kargs.sh \|\| true # Allow systemd to run the file, use pipe true to not failed if the system doesn't have selinux or apparmor enabled

		@@ -1,63 +1,144 @@
		#!/bin/bash
		# Copyright 2025 sriov-network-device-plugin authors

Make kargs.sh work with systemd service and for Ubuntu #847

Are you sure you want to change the base?

Make kargs.sh work with systemd service and for Ubuntu #847

Conversation

e0ne commented Feb 18, 2025

Uh oh!

github-actions bot commented Feb 18, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adrianchiris left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 19567899770

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

e0ne commented Jul 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SchSeba commented Jul 22, 2025

Uh oh!

e0ne commented Aug 1, 2025

Uh oh!

rbakhtaraev commented Sep 8, 2025

Uh oh!

e0ne commented Sep 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbakhtaraev commented Oct 28, 2025

coveralls commented Mar 31, 2025 •

edited

Loading