Daemon redesign - using controller-runtime #788

SchSeba · 2024-10-10T09:44:00Z

No description provided.

github-actions · 2024-10-10T09:44:13Z

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/daemon/daemon.go

coveralls · 2024-10-10T10:30:47Z

Pull Request Test Coverage Report for Build 12559552266

Details

261 of 604 (43.21%) changed or added relevant lines in 15 files are covered.
209 unchanged lines in 14 files lost coverage.
Overall coverage increased (+0.8%) to 48.003%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/host/internal/network/network.go	0	1	0.0%
pkg/host/store/store.go	0	1	0.0%
pkg/plugins/mellanox/mellanox_plugin.go	0	1	0.0%
pkg/daemon/config.go	30	32	93.75%
controllers/drain_controller.go	4	7	57.14%
pkg/systemd/systemd.go	2	6	33.33%
pkg/daemon/status.go	59	96	61.46%
cmd/sriov-network-config-daemon/start.go	7	141	4.96%
pkg/daemon/daemon.go	140	300	46.67%

Files with Coverage Reduction	New Missed Lines	%
controllers/drain_controller_helper.go	1	63.59%
pkg/systemd/systemd.go	4	25.24%
pkg/client/clientset/versioned/fake/clientset_generated.go	5	46.15%
cmd/sriov-network-config-daemon/start.go	5	8.64%
pkg/client/informers/externalversions/sriovnetwork/interface.go	6	0.0%
pkg/plugins/fake/fake_plugin.go	6	20.0%
pkg/client/informers/externalversions/sriovnetwork/v1/interface.go	9	0.0%
pkg/client/clientset/versioned/typed/sriovnetwork/v1/fake/fake_sriovoperatorconfig.go	15	10.81%
pkg/client/clientset/versioned/typed/sriovnetwork/v1/fake/fake_sriovnetworknodestate.go	17	25.68%
pkg/client/informers/externalversions/sriovnetwork/v1/sriovnetworknodestate.go	19	0.0%

Totals
Change from base Build 12557982099:	0.8%
Covered Lines:	7248
Relevant Lines:	15099

💛 - Coveralls

test/util/namespaces/namespaces.go

Signed-off-by: Sebastian Sch <[email protected]>

It's only a one time run script so it should not be a problem to run with debug logs. Signed-off-by: Sebastian Sch <[email protected]>

cmd/sriov-network-config-daemon/service.go

Signed-off-by: Sebastian Sch <[email protected]>

zeeke

Left a few minor comments. This PR hugely improves the config-daemon shape.

zeeke · 2025-01-07T11:13:19Z

controllers/drain_controller.go

-	nodeStateDrainAnnotationCurrent, nodeStateExist, err := dr.ensureAnnotationExists(ctx, nodeNetworkState, constants.NodeStateDrainAnnotationCurrent)
+	nodeStateDrainAnnotationCurrent, currentNodeStateExist, err := dr.ensureAnnotationExists(ctx, nodeNetworkState, constants.NodeStateDrainAnnotationCurrent)
+	if err != nil {
+		reqLogger.Error(err, "failed to ensure nodeStateDrainAnnotation")


Suggested change

reqLogger.Error(err, "failed to ensure nodeStateDrainAnnotation")

reqLogger.Error(err, "failed to ensure nodeStateDrainAnnotationCurrent")

can you also fix on L123 with nodeStateDrainAnnotation -> nodeDrainAnnotation ?

zeeke · 2025-01-07T11:35:49Z

pkg/systemd/systemd.go

 			log.Log.V(2).Info("ReadSriovResult(): file does not exist, return empty result")
-			return &SriovResult{}, nil
+			return nil, false, nil


This does not return an empty SriovResult anymore. who's right, the log or the code?

code always! :P

zeeke · 2025-01-07T12:55:00Z

pkg/daemon/status.go

+	return false, nil
+}
+
+func (dn *DaemonReconcile) getHostNetworkStatus(nodeState *sriovnetworkv1.SriovNetworkNodeState) error {


nit: please rename to updateStatusFromHost() or something similar.
a get*() function does not sound like something that changes the passed parameters.

zeeke · 2025-01-07T12:59:40Z

pkg/daemon/daemon.go

-	var sriovResult = &systemd.SriovResult{SyncStatus: consts.SyncStatusSucceeded, LastSyncError: ""}
-	dn.desiredNodeState, err = dn.sriovClient.SriovnetworkV1().SriovNetworkNodeStates(vars.Namespace).Get(context.Background(), vars.NodeName, metav1.GetOptions{})
+	// if we are running in systemd mode we want to get the sriov result from the config-daemon that runs in systemd
+	sriovResult, exist, err := dn.checkSystemdStatus(ctx, desiredNodeState)


Suggested change

sriovResult, exist, err := dn.checkSystemdStatus(ctx, desiredNodeState)

sriovResult, sriovResultExists, err := dn.checkSystemdStatus(ctx, desiredNodeState)

Renaming this variable might be useful when reading around L235, where it's not obvious to know what "exist" refers to.

zeeke · 2025-01-07T13:02:27Z

pkg/daemon/writer.go

@@ -1,290 +0,0 @@
-package daemon


It's great to see this file go away! :)

zeeke · 2025-01-07T13:06:04Z

pkg/daemon/status.go

+	c := current.Status.DeepCopy().Interfaces
+	d := desiredNodeState.Status.DeepCopy().Interfaces
+	for idx := range d {
+		// check if it's a new device


Suggested change

// check if it's a new device

if idx >= len(c) {

return true, nil

}

// check if it's a new device

I don't know if it can ever happen (maybe if a new device is hot-plugged between reconcile loops). But it's safer to check for slice length

but in theory (not sure if it's possible) you can have a new device like hot-unplug and after that hot-plug so the len will be the same but the device is not no?

zeeke · 2025-01-07T13:07:18Z

pkg/daemon/status.go

+	return nil
+}
+
+func (dn *DaemonReconcile) shouldUpdateStatus(current, desiredNodeState *sriovnetworkv1.SriovNetworkNodeState) (bool, error) {


nit: this function returns no error. can get rid of the second return parameter?

you are right!

adrianchiris

partial review

adrianchiris · 2025-01-19T10:01:46Z

controllers/helper.go

@@ -108,14 +108,7 @@ type DrainStateAnnotationPredicate struct {
 }

 func (DrainStateAnnotationPredicate) Create(e event.CreateEvent) bool {


WDYT about removing the Create fn ?

the default of predicate.Funcs is true.

is object being nil a real concern here ?

the concern is not nil. without this when a new node is added to the cluster the reconcile will not get called without this.
and if the reconcile is not created no one adds the labels to make the drain possible.

without this when a new node is added to the cluster the reconcile will not get called without this

are you sure ? i thought by default it will call reconcile since predicate.Funcs contains implementation that will return true. which means to enqueue event for reconcile.

adrianchiris · 2025-01-19T10:04:02Z

cmd/sriov-network-config-daemon/start.go

-		// creates the in-cluster config
-		config, err = rest.InClusterConfig()
+func getOperatorConfig(kClient runtimeclient.Client) (*sriovnetworkv1.SriovOperatorConfig, error) {
+	// Init feature gates once to prevent race conditions.


nit: unrelated comment

adrianchiris · 2025-01-19T10:04:26Z

cmd/sriov-network-config-daemon/start.go


-	refreshCh := make(chan daemon.Message)
-	defer close(refreshCh)
+func UseKubeletKubeConfig() {


does it need to start with upper case ?

adrianchiris · 2025-01-19T10:14:07Z

cmd/sriov-network-config-daemon/start.go

+
+	// Setup reconcile loop with manager
+	if err = dm.SetupWithManager(mgr); err != nil {
+		setupLog.Error(err, "unable to create setup daemon manager for SriovNetworkNodeState")


nit: unable to setup daemon with manager for SriovNetworkNodeState

adrianchiris · 2025-01-19T10:17:03Z

pkg/daemon/daemon.go

-}
-
-type Daemon struct {
+type DaemonReconcile struct {


DaemonReconciler ? or better yet, NodeReconciler ?

adrianchiris · 2025-01-19T11:54:57Z

pkg/daemon/daemon.go

+	ns := &sriovnetworkv1.SriovNetworkNodeState{}
+	// init openstack info
+	if vars.PlatformType == consts.VirtualOpenStack {
+		ns, err = dn.HostHelpers.GetCheckPointNodeState()


can we limit the usage of nodestate from checkpoint to just this block ? i.e define a new var.

it seems odd that other platform types we dont load from checkpoint during init (which initializes InitialState global var

adrianchiris · 2025-01-19T12:02:20Z

pkg/daemon/daemon.go


+	// save init state


nit: // save init state if it does not exist on host

adrianchiris · 2025-01-19T12:07:30Z

pkg/daemon/daemon.go

-		log.Log.Error(err, "nodeStateSyncHandler(): Failed to fetch node state", "name", vars.NodeName)
-		return err
+		reqLogger.Error(err, "failed to check systemd status unexpected error")
+		return ctrl.Result{}, nil


we dont want to fail here ?

adrianchiris · 2025-01-19T12:09:23Z

pkg/daemon/daemon.go

-				<-dn.syncCh
-				return nil
-			}
+			return ctrl.Result{RequeueAfter: 30 * time.Second}, nil


nit: lets define requeueTime as a const

adrianchiris · 2025-01-19T12:09:59Z

pkg/daemon/daemon.go

-				return err
-			}
-			postNetworkServiceEnabled, err := dn.HostHelpers.IsServiceEnabled(systemd.SriovPostNetworkServicePath)
+		// if there are no node drifted changes, and we are on the latest applied policy


nit: // if there are no host state drift changes, ...

adrianchiris · 2025-01-19T12:26:12Z

pkg/daemon/daemon.go

 		}
+		reqDrain = reqDrain || systemdConfModified || !exist
+		// require reboot if drain needed for systemd mode
+		reqReboot = reqReboot || systemdConfModified || reqDrain || !exist


nit: you can drop : || systemdConfModified and || !exist
they are checked in reqDrain

adrianchiris · 2025-01-19T12:29:50Z

pkg/daemon/daemon.go

-			log.Log.Error(err, "nodeStateSyncHandler(): failed to write supported nic ids file for systemd mode")
-			return err
-		}
+func (dn *DaemonReconcile) checkSystemdStatus(ctx context.Context, desiredNodeState *sriovnetworkv1.SriovNetworkNodeState) (*systemd.SriovResult, bool, error) {


can you add docstring explaining the returned args ?

adrianchiris · 2025-01-19T12:48:12Z

pkg/daemon/daemon.go

+
+	// update the lastAppliedGeneration
+	dn.lastAppliedGeneration = desiredNodeState.Generation
+	return ctrl.Result{RequeueAfter: 30 * time.Second}, nil


nit: same as previous comment. lets use a const

adrianchiris · 2025-01-19T12:56:12Z

pkg/daemon/daemon.go

+	//	return sriovResult, err
+	//}
+
+	// TODO: check if we need this


i believe we need.

in case systemd services were enabled, and there is error in sriovResult, we want to reflect that in status and stop reconcile. else we end up in a reboot loop no ? (which we dont want here)

@ykulazhenkov for some more feedback on this bit

adrianchiris · 2025-01-19T13:06:13Z

once we get this in, i think we should move the node controllers under controllers/node/ pkg

adrianchiris · 2025-01-19T13:07:39Z

bindata/manifests/sriov-config-service/kubernetes/sriov-config-service.yaml

@@ -7,7 +7,7 @@ contents: |

  [Service]
  Type=oneshot
-  ExecStart=/var/lib/sriov/sriov-network-config-daemon -v 2 --zap-log-level 2 service --phase pre
+  ExecStart=/var/lib/sriov/sriov-network-config-daemon service --phase pre


this means reboot on update ? since systemd file changes

adrianchiris · 2025-01-19T13:09:03Z

hack/run-e2e-conformance.sh

@@ -7,4 +7,4 @@ GOPATH="${GOPATH:-$HOME/go}"
 JUNIT_OUTPUT="${JUNIT_OUTPUT:-/tmp/artifacts}"
 export PATH=$PATH:$GOPATH/bin

-${root}/bin/ginkgo -output-dir=$JUNIT_OUTPUT --junit-report "unit_report.xml" -v "$SUITE" -- -report=$JUNIT_OUTPUT
+${root}/bin/ginkgo --timeout 3h -output-dir=$JUNIT_OUTPUT --junit-report "unit_report.xml" -v "$SUITE" -- -report=$JUNIT_OUTPUT


is this change related ?

github-advanced-security bot found potential problems Oct 10, 2024

View reviewed changes

pkg/daemon/daemon.go Fixed Show fixed Hide fixed

SchSeba force-pushed the daemon_redesign branch 3 times, most recently from 9f5207e to 7d4e946 Compare October 10, 2024 10:23

SchSeba force-pushed the daemon_redesign branch 2 times, most recently from e17b584 to ddc70f2 Compare October 10, 2024 12:45

SchSeba force-pushed the daemon_redesign branch 2 times, most recently from 929886d to 9377404 Compare December 12, 2024 11:54

github-advanced-security bot found potential problems Dec 12, 2024

View reviewed changes

test/util/namespaces/namespaces.go Fixed Show fixed Hide fixed

SchSeba force-pushed the daemon_redesign branch 4 times, most recently from 79cc69b to dc6eb11 Compare December 16, 2024 14:03

SchSeba added 9 commits December 25, 2024 12:00

make sure drain controller adds all the initial annotations

dd5aeb3

Signed-off-by: Sebastian Sch <[email protected]>

extend ginkgo timeout

411977a

Signed-off-by: Sebastian Sch <[email protected]>

nit: fix virtual-cluster-redeploy.sh to add the right exports

e698d34

Signed-off-by: Sebastian Sch <[email protected]>

nit: fix log levels

ef95ca7

Signed-off-by: Sebastian Sch <[email protected]>

add featureGates to vars

20af59f

Signed-off-by: Sebastian Sch <[email protected]>

return bool to check if the sriovResult file exist on the host

c88204f

Signed-off-by: Sebastian Sch <[email protected]>

nit: return the changed objects from the annotateObj function

78f4774

Signed-off-by: Sebastian Sch <[email protected]>

add clean pools and pools CR to reporter for functional tests

7ded39b

Signed-off-by: Sebastian Sch <[email protected]>

run the systemd service with debug log level.

92b3e34

It's only a one time run script so it should not be a problem to run with debug logs. Signed-off-by: Sebastian Sch <[email protected]>

SchSeba force-pushed the daemon_redesign branch from dc6eb11 to 50ad79a Compare December 26, 2024 11:34

SchSeba changed the title ~~[WIP] Daemon redesign~~ Daemon redesign - using controller-runtime Dec 26, 2024

ykulazhenkov reviewed Dec 30, 2024

View reviewed changes

cmd/sriov-network-config-daemon/service.go Outdated Show resolved Hide resolved

SchSeba added 2 commits December 31, 2024 14:26

create operatorConfig controller for config-daemon

a5e3a74

Signed-off-by: Sebastian Sch <[email protected]>

switch config-daemon to use controller-runtime

f369b65

Signed-off-by: Sebastian Sch <[email protected]>

SchSeba force-pushed the daemon_redesign branch from 50ad79a to f369b65 Compare December 31, 2024 12:26

zeeke reviewed Jan 7, 2025

View reviewed changes

adrianchiris reviewed Jan 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Daemon redesign - using controller-runtime #788

Daemon redesign - using controller-runtime #788

SchSeba commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

coveralls commented Oct 10, 2024 •

edited

Loading

zeeke left a comment

zeeke Jan 7, 2025

zeeke Jan 7, 2025

SchSeba Jan 20, 2025

zeeke Jan 7, 2025

zeeke Jan 7, 2025

zeeke Jan 7, 2025

zeeke Jan 7, 2025

SchSeba Jan 20, 2025

zeeke Jan 7, 2025

SchSeba Jan 20, 2025

adrianchiris left a comment

adrianchiris Jan 19, 2025

SchSeba Jan 20, 2025

adrianchiris Jan 20, 2025

adrianchiris Jan 19, 2025

adrianchiris Jan 19, 2025

adrianchiris Jan 19, 2025

adrianchiris Jan 19, 2025

adrianchiris Jan 19, 2025

adrianchiris Jan 19, 2025

adrianchiris Jan 19, 2025

adrianchiris Jan 19, 2025

adrianchiris Jan 19, 2025

adrianchiris Jan 19, 2025

adrianchiris Jan 19, 2025

adrianchiris Jan 19, 2025

adrianchiris Jan 19, 2025 •

edited

Loading

adrianchiris Jan 19, 2025

adrianchiris commented Jan 19, 2025

adrianchiris Jan 19, 2025 •

edited

Loading

adrianchiris Jan 19, 2025

	reqLogger.Error(err, "failed to ensure nodeStateDrainAnnotation")
	reqLogger.Error(err, "failed to ensure nodeStateDrainAnnotationCurrent")

	sriovResult, exist, err := dn.checkSystemdStatus(ctx, desiredNodeState)
	sriovResult, sriovResultExists, err := dn.checkSystemdStatus(ctx, desiredNodeState)

		@@ -108,14 +108,7 @@ type DrainStateAnnotationPredicate struct {
		}

		func (DrainStateAnnotationPredicate) Create(e event.CreateEvent) bool {

Daemon redesign - using controller-runtime #788

Are you sure you want to change the base?

Daemon redesign - using controller-runtime #788

Conversation

SchSeba commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

coveralls commented Oct 10, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12559552266

Details

💛 - Coveralls

zeeke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris commented Jan 19, 2025

adrianchiris Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 10, 2024 •

edited

Loading

adrianchiris Jan 19, 2025 •

edited

Loading

adrianchiris Jan 19, 2025 •

edited

Loading