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

Minor improvments for systemd mode implementation #531

Merged
merged 2 commits into from
Nov 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions pkg/daemon/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ func (dn *Daemon) nodeStateSyncHandler() error {

if dn.nodeState.GetGeneration() == latest {
if dn.useSystemdService {
serviceExist, err := dn.serviceManager.IsServiceExist(systemd.SriovServicePath)
serviceEnabled, err := dn.serviceManager.IsServiceEnabled(systemd.SriovServicePath)
if err != nil {
log.Log.Error(err, "nodeStateSyncHandler(): failed to check if sriov-config service exist on host")
return err
Expand All @@ -475,8 +475,9 @@ func (dn *Daemon) nodeStateSyncHandler() error {
// if the service doesn't exist we should continue to let the k8s plugin to create the service files
// this is only for k8s base environments, for openshift the sriov-operator creates a machine config to will apply
// the system service and reboot the node the config-daemon doesn't need to do anything.
if !serviceExist {
sriovResult = &systemd.SriovResult{SyncStatus: syncStatusFailed, LastSyncError: "sriov-config systemd service doesn't exist on node"}
if !serviceEnabled {
sriovResult = &systemd.SriovResult{SyncStatus: syncStatusFailed,
LastSyncError: "sriov-config systemd service is not available on node"}
} else {
sriovResult, err = systemd.ReadSriovResult()
if err != nil {
Expand All @@ -495,7 +496,6 @@ func (dn *Daemon) nodeStateSyncHandler() error {
<-dn.syncCh
return nil
}
return nil
}
log.Log.V(0).Info("nodeStateSyncHandler(): Interface not changed")
if latestState.Status.LastSyncError != "" ||
Expand Down Expand Up @@ -583,13 +583,22 @@ 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

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.

if err != nil {
log.Log.Error(err, "nodeStateSyncHandler(): failed to write configuration file for systemd mode")
return err
}
reqDrain = reqDrain || r
reqReboot = reqReboot || r
if systemdConfModified {
// remove existing result file to make sure that we will not use outdated result, e.g. in case if
// systemd service was not triggered for some reason
err = systemd.RemoveSriovResult()
if err != nil {
log.Log.Error(err, "nodeStateSyncHandler(): failed to remove result file for systemd mode")
return err
}
}
reqDrain = reqDrain || systemdConfModified
reqReboot = reqReboot || systemdConfModified
log.Log.V(0).Info("nodeStateSyncHandler(): systemd mode WriteConfFile results",
"drain-required", reqDrain, "reboot-required", reqReboot, "disable-drain", dn.disableDrain)

Expand Down
6 changes: 3 additions & 3 deletions pkg/plugins/k8s/k8s_plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -322,13 +322,13 @@ func (p *K8sPlugin) switchdevServiceStateUpdate() error {

func (p *K8sPlugin) sriovServiceStateUpdate() error {
log.Log.Info("sriovServiceStateUpdate()")
exist, err := p.serviceManager.IsServiceExist(p.sriovService.Path)
isServiceEnabled, err := p.serviceManager.IsServiceEnabled(p.sriovService.Path)
if err != nil {
return err
}

// create the service if it doesn't exist
if !exist {
// create and enable the service if it doesn't exist or is not enabled
if !isServiceEnabled {
p.updateTarget.sriovScript = true
} else {
p.updateTarget.sriovScript = p.isSystemServiceNeedUpdate(p.sriovService)
Expand Down
21 changes: 20 additions & 1 deletion pkg/service/service_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

type ServiceManager interface {
IsServiceExist(string) (bool, error)
IsServiceEnabled(string) (bool, error)
ReadService(string) (*Service, error)
EnableService(service *Service) error
}
Expand All @@ -27,7 +28,7 @@ func NewServiceManager(chroot string) ServiceManager {
return &serviceManager{root}
}

// ReadService read service from given path
// IsServiceExist check if service unit exist
func (sm *serviceManager) IsServiceExist(servicePath string) (bool, error) {
_, err := os.Stat(path.Join(sm.chroot, servicePath))
if err != nil {
Expand All @@ -40,6 +41,24 @@ func (sm *serviceManager) IsServiceExist(servicePath string) (bool, error) {
return true, nil
}

// IsServiceEnabled check if service exist and enabled
func (sm *serviceManager) IsServiceEnabled(servicePath string) (bool, error) {
exist, err := sm.IsServiceExist(servicePath)
if err != nil || !exist {
return false, err
}
serviceName := filepath.Base(servicePath)
// Change root dir
exit, err := utils.Chroot(sm.chroot)
if err != nil {
return false, err
}
defer exit()

cmd := exec.Command("systemctl", "is-enabled", serviceName)
return cmd.Run() == nil, nil
}

// ReadService read service from given path
func (sm *serviceManager) ReadService(servicePath string) (*Service, error) {
data, err := os.ReadFile(path.Join(sm.chroot, servicePath))
Expand Down
14 changes: 14 additions & 0 deletions pkg/systemd/systemd.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,20 @@ func ReadSriovResult() (*SriovResult, error) {
return result, err
}

func RemoveSriovResult() error {
err := os.Remove(SriovHostSystemdResultPath)
if err != nil {
if os.IsNotExist(err) {
log.Log.V(2).Info("RemoveSriovResult(): result file not found")
return nil
}
log.Log.Error(err, "RemoveSriovResult(): failed to remove sriov result file", "path", SriovHostSystemdResultPath)
return err
}
log.Log.V(2).Info("RemoveSriovResult(): result file removed")
return nil
}

func WriteSriovSupportedNics() error {
_, err := os.Stat(sriovHostSystemdSupportedNicPath)
if err != nil {
Expand Down
Loading