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.

Sign up for GitHub

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

Jump to bottom

Minor improvments for systemd mode implementation #531

Merged
merged 2 commits into from
Nov 1, 2023
Merged

Minor improvments for systemd mode implementation #531

merged 2 commits into from
Nov 1, 2023

Conversation

ykulazhenkov
Copy link
Collaborator

@ykulazhenkov ykulazhenkov commented Oct 26, 2023 •
edited
Loading

Couple improvements for systemd mode implementation to cover edge cases:

  • check that service enabled in addition to check which validates that service exist
  • remove old result file when we update configuration for systemd service to make sure that there is no chance to read outdated info

The original implementation can't detect situation when the service exists on the node, but for some reason was disabled by the user. In this case daemon was not able to detect this misconfiguration.

@github-actions
Copy link

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
Copy link
Collaborator Author

@adrianchiris @e0ne @SchSeba please check this PR. Thanks

e0ne
e0ne approved these changes Oct 27, 2023
View reviewed changes
pkg/service/service_manager.go Outdated Show resolved Hide resolved
@github-actions
Copy link

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
Copy link

coveralls commented Oct 27, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 6709601933

  • 0 of 43 (0.0%) changed or added relevant lines in 4 files are covered.
  • 10 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.2%) to 25.409%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/plugins/k8s/k8s_plugin.go 0 2 0.0%
pkg/systemd/systemd.go 0 11 0.0%
pkg/daemon/daemon.go 0 15 0.0%
pkg/service/service_manager.go 0 15 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/daemon/daemon.go 1 38.9%
controllers/sriovibnetwork_controller.go 2 70.45%
api/v1/helper.go 3 42.94%
controllers/sriovnetwork_controller.go 4 70.68%
Totals Coverage Status
Change from base Build 6704865825: -0.2%
Covered Lines: 2329
Relevant Lines: 9166
💛 - Coveralls

bn222
bn222 reviewed Oct 27, 2023
View reviewed changes
pkg/daemon/daemon.go
@@ -572,14 +572,23 @@ func (dn *Daemon) nodeStateSyncHandler() error {
// or there is a new config we need to apply
// When using systemd configuration we write the file
if dn.useSystemdService {
r, err := systemd.WriteConfFile(latestState, dn.devMode, dn.platform)
systemdConfModified, err := systemd.WriteConfFile(latestState, dn.devMode, dn.platform)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a great opportunity to clean this up using https://github.com/coreos/go-systemd

Can you refactor and get rid of all the manual systemd calls? The above library is the official one. That would mean we can clean up pkg/systemd/systemd.go by using that lib.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @bn222 that package is great does it only work for RHEL/COREOS or also other operating systems?

Copy link
Collaborator

@adrianchiris adrianchiris Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@bn222 can you open an issue for this ? i do not want to turn this bug fix into a refactor.
indeed it must not be limited to RHEL/CoreOS

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that this is a good idea to switch to the library, but let's handle this as a separate PR with refactoring.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@SchSeba I believe it works in general.
@adrianchiris The downside of having a separate PR is that it tends to get forgotten. I've opened #533, so let's not let it linger for too long.

@bn222
Copy link
Collaborator

bn222 commented Oct 30, 2023

/lgtm

@github-actions github-actions bot added the lgtm label Oct 30, 2023
@adrianchiris
Copy link
Collaborator

@ykulazhenkov can you rebase this one ?

zeeke
zeeke reviewed Oct 31, 2023
View reviewed changes
Copy link
Member

@zeeke zeeke left a comment

Choose a reason for hiding this comment

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

LGTM
@ykulazhenkov please rebase

@github-actions GitHub Actions
Copy link

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
Add check extra check for systemd service in the daemon
2be5408 
When systemd configuration mode is used we need to make sure that
the systemd service in not only exist but also is in enabled state.

Signed-off-by: Yury Kulazhenkov <[email protected]>
@ykulazhenkov
Remove systemd service result file after configuration update
f1afab5 
This is required to ensure that we will not read outdated result in
edge case scenarios, e.g. when systemd service exist but for some
reason was disabled by the user.

Signed-off-by: Yury Kulazhenkov <[email protected]>
@github-actions GitHub Actions
Copy link

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
Copy link
Collaborator Author

PR rebased

@ykulazhenkov
Copy link
Collaborator Author

/test-all

@wizhaoredhat
Copy link
Contributor

lgtm

@e0ne e0ne merged commit 744ca04 into k8snetworkplumbingwg:master Nov 1, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bn222 bn222 bn222 left review comments

@SchSeba SchSeba SchSeba left review comments

@zeeke zeeke zeeke left review comments

@adrianchiris adrianchiris adrianchiris approved these changes

@e0ne e0ne e0ne approved these changes

Assignees
No one assigned
Labels
lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ykulazhenkov @coveralls @bn222 @adrianchiris @wizhaoredhat @e0ne @zeeke @SchSeba