Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

e0ne
Copy link
Collaborator

@e0ne e0ne commented Feb 18, 2025

No description provided.

Copy link

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.

@e0ne e0ne force-pushed the ubuntu-kargs branch 2 times, most recently from 596b809 to dfb9915 Compare February 18, 2025 13:54
@coveralls
Copy link

coveralls commented Feb 18, 2025

Pull Request Test Coverage Report for Build 13436956034

Details

  • 3 of 5 (60.0%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.03%) to 47.949%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/plugins/generic/generic_plugin.go 3 5 60.0%
Files with Coverage Reduction New Missed Lines %
controllers/drain_controller_helper.go 1 63.59%
controllers/helper.go 1 69.66%
controllers/drain_controller.go 3 78.57%
Totals Coverage Status
Change from base Build 13402559676: 0.03%
Covered Lines: 7270
Relevant Lines: 15162

💛 - Coveralls

@e0ne e0ne force-pushed the ubuntu-kargs branch 5 times, most recently from 508a194 to 265f915 Compare February 20, 2025 08:05
@rollandf
Copy link
Member

Missing Signed-off?

@@ -81,7 +81,8 @@ type genericPluginOptions struct {
skipBridgeConfiguration bool
}

const scriptsPath = "bindata/scripts/kargs.sh"
const daemonScriptsPath = "bindata/scripts/kargs.sh"

Choose a reason for hiding this comment

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

nit. try to refrain of relative path

Suggested change
const daemonScriptsPath = "bindata/scripts/kargs.sh"
const daemonScriptsPath = "/bindata/scripts/kargs.sh"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -81,7 +81,8 @@ type genericPluginOptions struct {
skipBridgeConfiguration bool
}

const scriptsPath = "bindata/scripts/kargs.sh"
const daemonScriptsPath = "bindata/scripts/kargs.sh"
const systemdScriptsPath = "//var/lib/sriov/kargs.sh"

Choose a reason for hiding this comment

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

nit. remove double '/'

Suggested change
const systemdScriptsPath = "//var/lib/sriov/kargs.sh"
const systemdScriptsPath = "/var/lib/sriov/kargs.sh"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -116,7 +116,7 @@ spec:
command:
- /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

Choose a reason for hiding this comment

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

nit. This is a very long command, and can tend to be error prune. You should consider using another script (e.g. init-systemd.sh) and break this command to several lines in script

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch is it ok to do it in a follow up PR?

@e0ne e0ne merged commit f0619bf into Mellanox:network-operator-25.1.x Feb 21, 2025
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants