Add support for cli and json styled SR Linux configs #343

n0shut · 2023-03-28T20:08:10Z

This is a matching PR to support CLI and JSON-styled configs for SR Linux srl-labs/srl-controller#37

The idea is that KNE calculates how the config file should be named (config.cli or config.json) based on the extension provided to the startup config file.

Then srl-controller loads the appropriate startup config file.

Changes to resetter

Resetter now resets to startup config if it was provided, or to a default config (factory) if no startup config was provided (see inline comments)

Changes to ConfigPush

Config push now works with CLI snippets expressed both in set / flat syntax as well as multiline CLI-styled blobs.

Changes to examples

I have thus removed the JSON config example and used CLI format hereafter.

coveralls · 2023-03-29T20:10:49Z

Pull Request Test Coverage Report for Build 4612322046

32 of 42 (76.19%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.03%) to 64.229%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
topo/node/srl/srl.go	32	42	76.19%

Files with Coverage Reduction	New Missed Lines	%
topo/node/srl/srl.go	2	54.68%

Totals
Change from base Build 4578300478:	0.03%
Covered Lines:	3144
Relevant Lines:	4895

💛 - Coveralls

n0shut · 2023-03-30T11:41:18Z

topo/node/srl/srl.go

-	configResetCmd      = "load factory auto-commit"
+	// srl-controller v0.6.0+ creates a named checkpoint "initial" on node startup
+	// configuration reset is therefore done by reverting to this checkpoint
+	configResetCmd = "/tools system configuration checkpoint initial revert"


resetting is now done by reverting to a checkpoint named initial.
This checkpoint is created by srl-controller:v0.6.0+ and will capture the config state of a node once it has booted and applied startup config (if present)

n0shut · 2023-03-30T11:41:38Z

topo/node/srl/srl.go

-	err := n.ControllerClient.Delete(ctx, &srlinuxv1.Srlinux{ObjectMeta: metav1.ObjectMeta{Name: n.Name()}})
+	err := n.ControllerClient.Delete(ctx, &srlinuxv1.Srlinux{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: n.GetNamespace(), Name: n.Name()}},
+	)


Namespace info was missing before in a call to Delete

n0shut · 2023-03-30T11:42:37Z

topo/node/srl/srl.go

+	// SR Linux default name for config file is either config.json or config.cli.
+	// This depends on the extension of the provided startup-config file.
 	if pb.Config.ConfigFile == "" {
-		pb.Config.ConfigFile = "config.json"
+		ext := filepath.Ext(pb.Config.GetFile())
+		if ext != ".json" {
+			ext = ".cli"
+		}
+
+		pb.Config.ConfigFile = "config" + ext
 	}


The default file name used to contain startup-config lines is derived from the file extension provided by a user in the topo.
This is done to let srl-controller know what format the config is provided with.

n0shut · 2023-03-30T11:43:12Z

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

    }
    services:{
        key: 22
        value: {
            name: "ssh"
            inside: 22
-            outside: 22


outside ports were removed, as keys already convey this info

alexmasi · 2023-03-30T17:17:16Z

I took a quick glance, the behavior is exactly what we would want to support (cli or json), simplified cfgs, reset to initial cfg commit.

Can you update the examples/multivendor/. topo + cfg as well as cloudbuild/vendors/. topo + cfg to match the new simplified cfgs?

Assigning to Paul to do an actual code review here

n0shut · 2023-03-30T19:50:21Z

Thanks @alexmasi
added CLI-styled configs for cloudbuild and multivendor

hellt · 2023-03-31T14:33:48Z

topo/node/srl/srl.go

+	// srl-controller v0.6.0+ creates a named checkpoint "initial" on node startup
+	// configuration reset is therefore done by reverting to this checkpoint
+	configResetCmd = "/tools system configuration checkpoint initial revert"
+	pushCfgFile    = "/home/admin/kne-push-config"


configpush functionality now undergoes the following steps:

config from a file is read into a string, and all double quotes are escaped

the escaped config is then transferred to the srlinux node using pty transport opened with scrapligo over kubectl exec.

the transfer happens via echo "<config lines>" > pushCfgFile command.

then a candidate is opened and pushed config is sourced via source pushCfgFile

It is then committed and persisted.

hellt · 2023-03-31T14:34:42Z

topo/node/srl/testdata/configpush_failure

+A:pod1# file cat /etc/opt/srlinux/devices/app_ephemeral.mgmt_server.ready_for_config
+loaded initial configuration


in srlinux-scrapli package v0.6.0 we improved the way to check when the NOS is ready to accept configs. This is the new way

bormanp

The Go changes are simply nits

bormanp · 2023-03-31T15:22:16Z

topo/node/srl/srl.go


-	log.V(1).Infof("config to push:\n%s", cfgs)
+	log.V(1).Infof("config to push:\n%s", cfg)


Shouldn't these lines be after the check for err?

thanks @bormanp
done in 4cb5267

bormanp · 2023-03-31T15:26:00Z

topo/node/srl/srl.go

+	err := n.ControllerClient.Delete(ctx, &srlinuxv1.Srlinux{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: n.GetNamespace(), Name: n.Name()}},
+	)


When breaking up the line you should match the {}'s.

err := n.ControllerClient.Delete(ctx, &srlinuxv1.Srlinux{ ObjectMeta: metav1.ObjectMeta{ Namespace: n.GetNamespace(), Name: n.Name(), }, })

thanks
Done in 9a561cd

alexmasi · 2023-04-03T17:00:59Z

Looks like one of those commits caused the go test to fail

hellt · 2023-04-03T17:14:45Z

@alexmasi it is a wild west in go test output there, I couldn't find which test was failing, I saw that some cluster tests failed, so maybe try to rerun to exclude flakiness?

hellt · 2023-04-03T22:17:45Z

@alexmasi looks like some metallb tests failed -- https://github.com/openconfig/kne/actions/runs/4582237788/jobs/8092328043?pr=343#step:4:643
I don't think this PR is related to it

alexmasi · 2023-04-04T00:37:15Z

I have no clue how that test could have failed, the context gets cancelled as expected. Anyway rerunning now to confirm its a flake. This PR doesn't directly modify that code so I'm even more confused, we haven't seen a flake like that before

alexmasi · 2023-04-04T00:39:02Z

/gcbrun

alexmasi

carrying over borman's approval that was dismissed, assuming the presubmit passes I will merge

sorry for the hassle here

alexmasi · 2023-04-04T21:06:15Z

/gcbrun

hellt mentioned this pull request Mar 28, 2023

Support for CLI and JSON-styled startup configs srl-labs/srl-controller#37

Merged

n0shut changed the title ~~Add support for cli and json styled configs~~ Add support for cli and json styled SR Linux configs Mar 29, 2023

n0shut marked this pull request as ready for review March 30, 2023 10:04

n0shut marked this pull request as draft March 30, 2023 10:18

n0shut commented Mar 30, 2023

View reviewed changes

n0shut marked this pull request as ready for review March 30, 2023 11:43

alexmasi requested a review from bormanp March 30, 2023 17:14

n0shut force-pushed the srl-cli-startup-cfg branch from 6a599fe to 5804601 Compare March 30, 2023 19:40

hellt reviewed Mar 31, 2023

View reviewed changes

bormanp previously approved these changes Mar 31, 2023

View reviewed changes

n0shut dismissed bormanp’s stale review via 9a561cd April 1, 2023 08:31

n0shut requested a review from bormanp April 1, 2023 08:33

alexmasi previously approved these changes Apr 4, 2023

View reviewed changes

n0shut added 5 commits April 4, 2023 23:03

add support for cli and json styled configs

e60f6a2

update manifest

c8ab5af

fixing tests to accommodate for new reset cmd

cf391c9

added test for two different config extensions

d6319dd

added namespace for delete operation

e6815ce

n0shut added 12 commits April 4, 2023 23:03

swapped json config with cli

01ce2e5

added config status

7c625b0

fixed comment

5eff257

bump srl-controller

a6e9ea7

rename to config.cfg

6498600

added cli config for multivendor lab

54aa5bb

added cli config for cloudbuild

e228715

fix config name

35dda5b

fix config push

4a504f3

fix tests with new mgmt server readiness prompt

6fb0d80

check error was misplaced

b8af5c6

braces alignment

fa62bee

n0shut dismissed alexmasi’s stale review via fa62bee April 4, 2023 21:04

n0shut force-pushed the srl-cli-startup-cfg branch from 9a561cd to fa62bee Compare April 4, 2023 21:04

alexmasi approved these changes Apr 4, 2023

View reviewed changes

alexmasi merged commit 97ecf79 into openconfig:main Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for cli and json styled SR Linux configs #343

Add support for cli and json styled SR Linux configs #343

n0shut commented Mar 28, 2023 •

edited

Loading

coveralls commented Mar 29, 2023 •

edited

Loading

n0shut Mar 30, 2023

n0shut Mar 30, 2023

n0shut Mar 30, 2023

n0shut Mar 30, 2023

alexmasi commented Mar 30, 2023

n0shut commented Mar 30, 2023

hellt Mar 31, 2023 •

edited

Loading

hellt Mar 31, 2023

bormanp left a comment

bormanp Mar 31, 2023

n0shut Apr 1, 2023

bormanp Mar 31, 2023

n0shut Apr 1, 2023

alexmasi commented Apr 3, 2023

hellt commented Apr 3, 2023

hellt commented Apr 3, 2023

alexmasi commented Apr 4, 2023

alexmasi commented Apr 4, 2023

alexmasi left a comment

alexmasi commented Apr 4, 2023

		A:pod1# file cat /etc/opt/srlinux/devices/app_ephemeral.mgmt_server.ready_for_config
		loaded initial configuration


		log.V(1).Infof("config to push:\n%s", cfgs)
		log.V(1).Infof("config to push:\n%s", cfg)

Add support for cli and json styled SR Linux configs #343

Add support for cli and json styled SR Linux configs #343

Conversation

n0shut commented Mar 28, 2023 • edited Loading

Changes to resetter

Changes to ConfigPush

Changes to examples

coveralls commented Mar 29, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4612322046

💛 - Coveralls

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 30, 2023

n0shut commented Mar 30, 2023

hellt Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bormanp 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

alexmasi commented Apr 3, 2023

hellt commented Apr 3, 2023

hellt commented Apr 3, 2023

alexmasi commented Apr 4, 2023

alexmasi commented Apr 4, 2023

alexmasi left a comment

Choose a reason for hiding this comment

alexmasi commented Apr 4, 2023

n0shut commented Mar 28, 2023 •

edited

Loading

coveralls commented Mar 29, 2023 •

edited

Loading

hellt Mar 31, 2023 •

edited

Loading