srlinux: Use controller-runtime dynamic client instead of a generated clientset #326

n0shut · 2023-03-10T18:38:12Z

Hi all,

It seems it is considered to be a common (good?) practice nowadays to use an API client provided by the controller-runtime package that allows users to work with both structured and unstructured objects while removing the need to generate clientsets [1] [2] [3] [4].

In this PR I am removing srl-controller clientset in favor of the controller-runtime client. Currently I put controller-runtime client under the Node struct, but if this approach will be considered good by the kne team it definitely makes sense to initialize a controller once for all node implementations. Each node implementation just needs to register its own schema with the client to make it all work.

This PR works with the tip of the branch that nokia works on in parallel - srl-labs/srl-controller#31.

Looking forward to your review comments about this approach.

/cc @marcushines

[1] -- https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.14.5/pkg/client
[2] -- https://hackernoon.com/platforms-on-k8s-with-golang-watch-any-crd-0v2o3z1q#h-interacting-with-cr-ds-the-new-way
[3] -- kubernetes-sigs/kubebuilder#1152 (comment)
[4] -- https://medium.com/@timebertt/kubernetes-controllers-at-scale-clients-caches-conflicts-patches-explained-aa0f7a8b4332#053d

n0shut · 2023-03-11T09:21:21Z

A fake client exists from controller-runtime client -- https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake?utm_source=godoc

This allows to write unit tests for the CRs in question. Example - https://github.com/elastic/cloud-on-k8s/blob/main/pkg/controller/apmserver/controller_test.go

coveralls · 2023-03-11T10:48:15Z

Pull Request Test Coverage Report for Build 4441135746

5 of 25 (20.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.01%) to 62.065%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
topo/node/srl/srl.go	5	25	20.0%

Totals
Change from base Build 4420828528:	-0.01%
Covered Lines:	2837
Relevant Lines:	4571

💛 - Coveralls

n0shut · 2023-03-12T10:19:28Z

Hi @alexmasi @marcushines
I have updated this PR to pass all KNE tests using the proposed change to srl-controller in srl-labs/srl-controller#31.

I have also updated the srlinux example topology and startup file to feature srlinux 22.11.2 version and recent kind/kne versions.
For testing purposes I have released srl-controller:0.5.0-rc1 controller container and matching manifests. I have installed it on a kind cluster with

kubectl apply -k https://github.com/srl-labs/srl-controller/config/default

Additionally, I verified that all OC services work based on the updated example topology.

I would appreciate your comments and presubmit testing (granted you align the manifests to the new versions)

marcushines · 2023-03-13T17:22:42Z

topo/node/srl/srl_test.go

@@ -62,6 +64,11 @@ func (f *fakeWatch) ResultChan() <-chan watch.Event {
 }

 func TestNew(t *testing.T) {


func setUp() func() {
origSrlinuxClient := newSrlinuxClient
newSrlinuxClient = func(_ *rest.Config) (ctrlclient.Client, error) {
return nil, nil
}
return func() {
newSrlLinuxClient = origSrLinuxClient
}

then your tests

defer setUp()()

@marcushines I used a different name, but kept the logic. Interesting way of doing setup/teardown, huh.

cde31b6

…clientset

n0shut · 2023-03-15T12:55:42Z

Hi @alexmasi I updated the manifest, would be cool to see if we can run a presubmit using this branch

alexmasi · 2023-03-15T17:55:15Z

examples/nokia/srlinux-services/srl-openconfig.cfg.json

@@ -2361,9 +2401,6 @@
    }
  },
  "srl_nokia-network-instance:network-instance": [
-    {
-      "name": "DEFAULT"
-    },


I would like this line to be reverted, helps align this topo with the featureprofiles defaults

done a5ecb13

alexmasi · 2023-03-15T17:57:17Z

topo/node/srl/srl.go

-	if err != nil {
-		return err
-	}
+	n.ControllerClient.Delete(ctx, &srlinuxv1.Srlinux{ObjectMeta: metav1.ObjectMeta{Name: n.Name()}})


done 3190719

alexmasi · 2023-03-15T17:59:14Z

go.mod

@@ -21,7 +21,7 @@ require (
 	github.com/scrapli/scrapligocfg v1.0.0
 	github.com/spf13/cobra v1.6.1
 	github.com/spf13/pflag v1.0.5
-	github.com/srl-labs/srl-controller v0.4.6
+	github.com/srl-labs/srl-controller v0.4.7-0.20230311230716-e1c96973d463


the container is v0.5.0 and the corresponding lib is v0.4.7, is that intended?

yes, just for testing inside this PR.
If tests are ok I will take both as 0.5.0

I spoke to Marcus and I will regenerate the manifests to use :latest container tag, so that manifest stays more-or-less static

yeah I tested on my machine, good to switch to 0.5.0

alexmasi · 2023-03-15T17:59:29Z

/gcbrun

alexmasi · 2023-03-15T23:41:03Z

manifests/controllers/srlinux/manifest.yaml

      - args:
        - --health-probe-bind-address=:8081
        - --metrics-bind-address=127.0.0.1:8080
        - --leader-elect
        command:
        - /manager
-        image: ghcr.io/srl-labs/srl-controller:0.4.6
+        image: ghcr.io/srl-labs/srl-controller:latest


move this to 0.5.0

alexmasi · 2023-03-15T23:41:41Z

go.mod

@@ -21,7 +21,7 @@ require (
 	github.com/scrapli/scrapligocfg v1.0.0
 	github.com/spf13/cobra v1.6.1
 	github.com/spf13/pflag v1.0.5
-	github.com/srl-labs/srl-controller v0.4.6
+	github.com/srl-labs/srl-controller v0.4.7-0.20230311230716-e1c96973d463


yeah I tested on my machine, good to switch to 0.5.0

alexmasi · 2023-03-15T23:42:54Z

examples/nokia/srlinux-services/2node-srl-ixr6-with-oc-services.pbtxt

@@ -10,7 +10,7 @@ nodes: {
    vendor: NOKIA
    model: "ixr6e"
    config:{
-        image: "ghcr.io/nokia/srlinux:22.6.4"
+        image: "ghcr.io/nokia/srlinux:22.11.2"


just remove this line so it defaults to the :latest tag (also in the other nokia examples as well)

done, I added a comment about this behavior if that's ok. 6443adf

alexmasi · 2023-03-15T23:44:33Z

Okay testing 0.4.7-xxxxxx commit with 0.5.0-rc worked on my machine. The presubmit in its current state will not pass, I am working on a better presubmit to test things like this

Once the versions are 0.5.0 and the comments are addressed I will get this merged

marcushines · 2023-03-16T19:57:55Z

/gcbrun

n0shut changed the title ~~Use controller-runtime dynamic client instead of a generated clientset~~ srlinux: Use controller-runtime dynamic client instead of a generated clientset Mar 11, 2023

n0shut marked this pull request as ready for review March 11, 2023 10:52

n0shut marked this pull request as draft March 11, 2023 10:52

marcushines reviewed Mar 13, 2023

View reviewed changes

hellt mentioned this pull request Mar 13, 2023

Removing clientset and upgrading scaffolding using kubebuilder v3.8.0 srl-labs/srl-controller#31

Merged

n0shut added 8 commits March 15, 2023 12:50

use controller-runtime dynamic client

7bbd413

bump srl-controller to temp version

ba58ea9

added fake ctrl client for tests

6de48fe

updated temp srl-controller version to point to the one with removed …

73f34c2

…clientset

updated srlinux example with srlinux 22.11.2

5a6dace

updated version number

173261f

updated go.mod with tip of srl-controller#31

b61c314

go.mod update

cbde803

n0shut force-pushed the update-srl-controller-1.25 branch from 3cdf0c1 to cbde803 Compare March 15, 2023 12:03

n0shut added 2 commits March 15, 2023 13:42

use test setup step

cde31b6

updated manifest

f9e30f0

alexmasi reviewed Mar 15, 2023

View reviewed changes

n0shut added 3 commits March 15, 2023 20:35

use latest tag in the manifest

f5e09bd

bring back DEFAULT netinst

a5ecb13

added error check

3190719

n0shut marked this pull request as ready for review March 15, 2023 19:44

n0shut requested a review from alexmasi March 15, 2023 19:44

alexmasi reviewed Mar 15, 2023

View reviewed changes

fix srl-controller release to v0.5.0

67a00a5

n0shut added 2 commits March 16, 2023 18:08

bump srl-controller to v0.5.0

07c432e

use latest srl container image in examples

6443adf

n0shut requested a review from alexmasi March 16, 2023 17:14

marcushines previously approved these changes Mar 16, 2023

View reviewed changes

fix container image tag

107379f

n0shut dismissed marcushines’s stale review via 107379f March 16, 2023 20:01

alexmasi approved these changes Mar 16, 2023

View reviewed changes

alexmasi merged commit 8c1e29f into openconfig:main Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

srlinux: Use controller-runtime dynamic client instead of a generated clientset #326

srlinux: Use controller-runtime dynamic client instead of a generated clientset #326

n0shut commented Mar 10, 2023 •

edited

Loading

n0shut commented Mar 11, 2023 •

edited

Loading

coveralls commented Mar 11, 2023 •

edited

Loading

n0shut commented Mar 12, 2023 •

edited

Loading

marcushines Mar 13, 2023

n0shut Mar 15, 2023

n0shut commented Mar 15, 2023

alexmasi Mar 15, 2023

n0shut Mar 15, 2023

alexmasi Mar 15, 2023

n0shut Mar 15, 2023

alexmasi Mar 15, 2023

hellt Mar 15, 2023

alexmasi Mar 15, 2023

n0shut Mar 16, 2023

alexmasi commented Mar 15, 2023

alexmasi Mar 15, 2023

n0shut Mar 16, 2023

alexmasi Mar 15, 2023

alexmasi Mar 15, 2023

n0shut Mar 16, 2023

alexmasi commented Mar 15, 2023

marcushines commented Mar 16, 2023

		@@ -62,6 +64,11 @@ func (f *fakeWatch) ResultChan() <-chan watch.Event {
		}

		func TestNew(t *testing.T) {

srlinux: Use controller-runtime dynamic client instead of a generated clientset #326

srlinux: Use controller-runtime dynamic client instead of a generated clientset #326

Conversation

n0shut commented Mar 10, 2023 • edited Loading

n0shut commented Mar 11, 2023 • edited Loading

coveralls commented Mar 11, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4441135746

💛 - Coveralls

n0shut commented Mar 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n0shut commented Mar 15, 2023

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

alexmasi commented Mar 15, 2023

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

alexmasi commented Mar 15, 2023

marcushines commented Mar 16, 2023

n0shut commented Mar 10, 2023 •

edited

Loading

n0shut commented Mar 11, 2023 •

edited

Loading

coveralls commented Mar 11, 2023 •

edited

Loading

n0shut commented Mar 12, 2023 •

edited

Loading