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 13109407767

Details

263 of 606 (43.4%) changed or added relevant lines in 18 files are covered.
204 unchanged lines in 12 files lost coverage.
Overall coverage increased (+0.9%) to 48.173%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
controllers/sriovnetworknodepolicy_controller.go	1	2	50.0%
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/utils/shutdown.go	0	1	0.0%
controllers/drain_controller_helper.go	1	3	33.33%
pkg/daemon/config.go	30	32	93.75%
controllers/drain_controller.go	4	7	57.14%
pkg/systemd/systemd.go	2	7	28.57%
pkg/daemon/status.go	59	96	61.46%

Files with Coverage Reduction	New Missed Lines	%
pkg/client/clientset/versioned/fake/clientset_generated.go	5	46.15%
cmd/sriov-network-config-daemon/start.go	5	8.68%
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%
pkg/client/informers/externalversions/sriovnetwork/v1/sriovoperatorconfig.go	19	0.0%
api/v1/helper.go	21	73.32%

Totals
Change from base Build 13109399258:	0.9%
Covered Lines:	7318
Relevant Lines:	15191

💛 - Coveralls

test/util/namespaces/namespaces.go

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

zeeke

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

controllers/drain_controller.go

pkg/systemd/systemd.go

pkg/daemon/status.go

pkg/daemon/daemon.go

pkg/daemon/writer.go

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?

in both case, the if idx >= len(c) { can save a nil dereferencing. right?

pkg/daemon/status.go

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.

I give it a try and I needed to add it. without it I was not able to get the create calls :(

I can give it another try

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

from the code I can see that we always write the file. I am missing something?

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 ?

If we return an error it will get stuck in a loop as if the result for the systemd operator return an error we don't retry to not put the node into a bootloop

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

it was moved out of this function so we should be ok I remove this

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

yes it will make a reboot one time

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 ?

nope I open a different PR for that :)

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]>

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

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

SchSeba · 2025-02-03T10:23:20Z

Hi @zeeke @adrianchiris @ykulazhenkov can you please make another round of review here?

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

SchSeba force-pushed the daemon_redesign branch 3 times, most recently from 4ecc95a to 80564fa Compare February 2, 2025 14:07

SchSeba added 2 commits February 3, 2025 10:34

make sure drain controller adds all the initial annotations

a720d52

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

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

5c3d917

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

SchSeba added 9 commits February 3, 2025 10:34

nit: fix log levels

65331a0

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

add featureGates to vars

d1f5050

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

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

3c0411b

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

nit: return the changed objects from the annotateObj function

bf7d38e

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

add clean pools and pools CR to reporter for functional tests

7d1291a

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

run the systemd service with debug log level.

ad32830

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]>

create operatorConfig controller for config-daemon

4361d66

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

switch config-daemon to use controller-runtime

a8e0b20

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

Fix comments

00587fb

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

SchSeba force-pushed the daemon_redesign branch from 80564fa to 00587fb Compare February 3, 2025 08:34

zeeke approved these changes Feb 3, 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

SchSeba Jan 20, 2025

zeeke Feb 3, 2025

adrianchiris left a comment

adrianchiris Jan 19, 2025

SchSeba Jan 20, 2025

adrianchiris Jan 20, 2025

SchSeba Jan 29, 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

SchSeba Jan 30, 2025

adrianchiris Jan 19, 2025

SchSeba Jan 30, 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

SchSeba Jan 30, 2025

adrianchiris commented Jan 19, 2025

adrianchiris Jan 19, 2025 •

edited

Loading

SchSeba Jan 30, 2025

adrianchiris Jan 19, 2025

SchSeba Jan 30, 2025

SchSeba commented Feb 3, 2025

		@@ -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 13109407767

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchSeba commented Feb 3, 2025

coveralls commented Oct 10, 2024 •

edited

Loading

adrianchiris Jan 19, 2025 •

edited

Loading

adrianchiris Jan 19, 2025 •

edited

Loading