Update README #9

atanasdinov · 2023-11-10T18:13:53Z

Closes #4

Signed-off-by: Atanas Dinov <[email protected]>

rdoxenham

Great work on this @atanasdinov! Just a few minor things that I ran into with a few suggestions. Cheers!

README.md

rdoxenham · 2023-11-11T11:55:39Z

README.md

+```shell
+$ git clone https://github.com/suse-edge/nm-configurator.git
+$ cd nm-configurator
+$ cargo build --release # optionally specify --target flag if cross compiling


Minor, but I can't get this code to compile at present.

It will not compile on macOS. There are differences between libc on Mac and on Linux but I didn't dive deeper into it since I didn't think we'd perform tests on macOS. Should we look further into this?

Or maybe just put a disclaimer that this tool is intended to be executed on linux only.

rdoxenham · 2023-11-11T12:00:29Z

README.md

-Each file contains the desired state for a single network interface (e.g. `eth0`).
-Configurations for all interfaces for all known hosts must be generated using `nmstate`.
+```shell
+$ curl -o nmc -L https://github.com/suse-edge/nm-configurator/releases/latest/download/nmc-x86_64 


The latest flag here is not currently functioning. Also, do you think we should create an aarch64-apple-darwin release for local development and testing? Reason being, the aarch64 binary that we ship in the releases is seemingly only functioning on Linux?

rdo@tiw nm-configurator % uname -a Darwin tiw.local 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun 8 22:22:23 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T6020 arm64 rdo@tiw nm-configurator % file nmc nmc: ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), statically linked, Go BuildID=7BwBJnNJlJRnUbl2thQQ/28-rOLaTmUNHHZPy7ci2/Xvgxmzw6_9BSBXz0oYxI/u3jZiVKC7Vy47aHuefd5, with debug_info, not stripped rdo@tiw nm-configurator % ./nmc zsh: exec format error: ./nmc

Whereas it works just fine on Linux:

rdo@tw-aarch64-build:~> uname -a Linux tw-aarch64-build.rdo.wales 6.5.6-1-default #1 SMP PREEMPT_DYNAMIC Fri Oct 6 11:20:48 UTC 2023 (c97c2df) aarch64 aarch64 aarch64 GNU/Linux rdo@tw-aarch64-build:~> ./nmc INFO[2023-11-11T11:59:48Z] starting network manager configurator... FATA[2023-11-11T11:59:48Z] failed to load static host configuration: open config/host_config.yaml: no such file or directory

The latest flag is not working because we haven't tagged (and produced a release) yet. It'll work once we do so.

The binaries indeed are compiled for Linux x86_64 and aarch64 targets, not Darwin. Please take a look at this comment.

I'd say to call it nmc-linux-x86_64 just in case (and do the same for aarch64)

And maybe replace the instructions as per

curl -o nmc -L https://github.com/suse-edge/nm-configurator/releases/latest/download/nmc-linux-$(uname -m) chmod +x nmc

Nice suggestion, I'll go with this, thanks!

rdoxenham · 2023-11-11T12:02:33Z

README.md

+In order to generate these config (*.nmconnection) files, NMC uses the
+[nmstate](https://github.com/nmstate/nmstate) library and requires a configuration directory as an input.
+This directory must contain the desired network state for all hosts in a <i>hostname</i>.yaml file format.
+Please refer to the official nmstate docs for [examples](https://nmstate.io/examples.html).


Great, but I think we should probably provide a really simple example here in the docs rather than referring to the upstream documentation. What do you think?

rdoxenham · 2023-11-11T12:06:08Z

README.md

-```
+$ ls -R _out
+_out:
+host_config.yaml  node1  node2  node3


Are we missing instructions on how to build the host_config.yaml here?

It is automatically generated. We list the results of nmc generate here.

rdoxenham · 2023-11-11T12:08:50Z

README.md

-INFO[2023-08-17T17:32:23+03:00] storing file /etc/NetworkManager/system-connections/eth1.nmconnection... 
-INFO[2023-08-17T17:32:23+03:00] successfully configured network manager 
+```yaml
+- hostname: node1


OK, so the host_config.yaml is here, but shouldn't it be defined/documented prior to the generate above?

This is just an example of how it looks. Users shouldn't be creating or modifying it. Do you think specifying a label of some kind will make this more clear?

rdoxenham · 2023-11-11T12:16:32Z

README.md

+[2023-11-08T21:18:12Z INFO  nmc::generate_conf] Generating config from "config/node1.yaml"...
+[2023-11-08T21:18:12Z INFO  nmc::generate_conf] Generating config from "config/node2.yaml"...
+[2023-11-08T21:18:12Z INFO  nmc::generate_conf] Generating config from "config/node3.yaml"...
+[2023-11-08T21:18:12Z INFO  nmc] Successfully generated and stored network config


I can't seem to get this to work as expected, that's why I think that we need a simple example:

rdo@tw-aarch64-build:~/nmc-example> tree config/ config/ ├── host_config.yaml ├── node1.example.com ├── node2.example.com └── node3.example.com 1 directory, 4 files rdo@tw-aarch64-build:~/nmc-example> cat config/host_config.yaml - hostname: node1.example.com interfaces: - logical_name: eth0 mac_address: FE:C4:05:42:8B:AA - hostname: node2.example.com interfaces: - logical_name: eth0 mac_address: FE:C4:05:42:8B:AB - hostname: node3.example.com interfaces: - logical_name: eth0 mac_address: FE:C4:05:42:8B:AC rdo@tw-aarch64-build:~/nmc-example> cat config/node1.example.com interfaces: - name: eth0 type: ethernet state: up ipv4: address: - ip: 192.168.122.250 prefix-length: 24 enabled: true ipv6: address: - ip: 2001:db8::1:1 prefix-length: 64 enabled: true rdo@tw-aarch64-build:~/nmc-example> ./nmc generate --config-dir config --output-dir _out INFO[2023-11-11T12:13:37Z] starting network manager configurator... FATA[2023-11-11T12:13:37Z] failed to load static host configuration: yaml: unmarshal errors: line 1: cannot unmarshal !!seq into config.Config

Just drop the host_config.yaml file. It is not expected to be created manually.

Actually, you're also running the old Go version one. As mentioned, we haven't produced a tag yet so the v0.1.0 release contains the Go binaries.

Please use this one in the meantime.

rdoxenham · 2023-11-11T12:17:13Z

README.md

+NetworkManager settings for a given host out of the pool of predefined desired states.
+
+Typically used with [Combustion](https://documentation.suse.com/sle-micro/5.5/single-html/SLE-Micro-deployment/#cha-images-combustion)
+in order to bootstrap multiple nodes using the same provisioning artefact instead of depending on different custom images per machine.


I take it that the next step here will be to push this into EIB with an example of how to integrate?

The next step would be to integrate NMC into EIB but I don't believe we should be documenting that here. The highlighted text is more of a guideline as to how the apply command would commonly be used.

rdoxenham · 2023-11-11T12:18:18Z

README.md

 ```
+
+**NOTE:** Interface names during the installation of nodes might differ from the preconfigured logical ones.


Do I understand correctly that the code will compensate for network interface name changes, and will use the MAC address of each interface as the unique identifier and will update the network connection name to the system identified one, rather than the one from nmc configuration?

Yes, exactly.

e-minguez

Some comments.

README.md

e-minguez · 2023-11-13T08:47:45Z

README.md

-Each file contains the desired state for a single network interface (e.g. `eth0`).
-Configurations for all interfaces for all known hosts must be generated using `nmstate`.
+```shell
+$ curl -o nmc -L https://github.com/suse-edge/nm-configurator/releases/latest/download/nmc-x86_64 


I'd say to call it nmc-linux-x86_64 just in case (and do the same for aarch64)

e-minguez · 2023-11-13T08:52:57Z

README.md

-Each file contains the desired state for a single network interface (e.g. `eth0`).
-Configurations for all interfaces for all known hosts must be generated using `nmstate`.
+```shell
+$ curl -o nmc -L https://github.com/suse-edge/nm-configurator/releases/latest/download/nmc-x86_64 


And maybe replace the instructions as per

curl -o nmc -L https://github.com/suse-edge/nm-configurator/releases/latest/download/nmc-linux-$(uname -m) chmod +x nmc

e-minguez · 2023-11-13T08:53:28Z

README.md

+```shell
+$ git clone https://github.com/suse-edge/nm-configurator.git
+$ cd nm-configurator
+$ cargo build --release # optionally specify --target flag if cross compiling


Or maybe just put a disclaimer that this tool is intended to be executed on linux only.

e-minguez · 2023-11-13T08:53:58Z

README.md

+In order to generate these config (*.nmconnection) files, NMC uses the
+[nmstate](https://github.com/nmstate/nmstate) library and requires a configuration directory as an input.
+This directory must contain the desired network state for all hosts in a <i>hostname</i>.yaml file format.
+Please refer to the official nmstate docs for [examples](https://nmstate.io/examples.html).


e-minguez · 2023-11-13T08:55:52Z

README.md

 ```shell
-$ curl -o nm-configurator -L https://github.com/suse-edge/nm-configurator/releases/latest/download/nm-configurator-amd64 
-$ chmod +x nm-configurator
+$ export RUST_LOG=info # set log level ("error" by default)


I miss a "this is how my config looks like before I run nmc" section :)

README.md

Signed-off-by: Atanas Dinov <[email protected]>

e-minguez · 2023-11-20T11:09:17Z

README.md


-`nm-configurator` depends on having the desired network state for all known nodes beforehand.
+```shell
+$ curl -o nmc -L https://github.com/suse-edge/nm-configurator/releases/latest/download/nmc-linux-$(uname -m)


e-minguez · 2023-11-20T11:11:24Z

README.md

-$ cd nm-configurator
-$ go build . # optionally specify GOOS and GOARCH flags if cross compiling
-```
+$ ls -R network-config


Personal preference but I find find (pun intended) output better

e-minguez · 2023-11-20T11:11:50Z

README.md

+Each of these contains the configuration files for the desired network interfaces (e.g. `eth0`).
+
+The `host_config.yaml` file on the root level maps the hosts to all of their preconfigured interfaces.
+This is necessary in order for NMC to identify which host it is running on when applying the network configurations later.


Suggested change

This is necessary in order for NMC to identify which host it is running on when applying the network configurations later.

This is required in order for NMC to identify which host it is running on when applying the network configurations later.

rdoxenham

Awesome job, buddy!

Update README

1d1c98e

Signed-off-by: Atanas Dinov <[email protected]>

atanasdinov marked this pull request as ready for review November 10, 2023 18:16

atanasdinov requested review from rdoxenham and e-minguez November 10, 2023 18:21

rdoxenham requested changes Nov 11, 2023

View reviewed changes

e-minguez reviewed Nov 13, 2023

View reviewed changes

atanasdinov mentioned this pull request Nov 13, 2023

Configure logger based on CLI args #11

Closed

atanasdinov added 2 commits November 13, 2023 14:47

Update installation steps

d8ee603

Signed-off-by: Atanas Dinov <[email protected]>

Add complete example

9258d7d

Signed-off-by: Atanas Dinov <[email protected]>

atanasdinov requested review from e-minguez and rdoxenham November 20, 2023 11:08

e-minguez reviewed Nov 20, 2023

View reviewed changes

e-minguez approved these changes Nov 20, 2023

View reviewed changes

rdoxenham approved these changes Nov 20, 2023

View reviewed changes

e-minguez merged commit 1d3b917 into suse-edge:main Nov 20, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update README #9

Update README #9

atanasdinov commented Nov 10, 2023 •

edited

Loading

rdoxenham left a comment

rdoxenham Nov 11, 2023

atanasdinov Nov 13, 2023 •

edited

Loading

e-minguez Nov 13, 2023

rdoxenham Nov 11, 2023

atanasdinov Nov 13, 2023

e-minguez Nov 13, 2023

e-minguez Nov 13, 2023

atanasdinov Nov 13, 2023

rdoxenham Nov 11, 2023

e-minguez Nov 13, 2023

rdoxenham Nov 11, 2023

atanasdinov Nov 13, 2023

rdoxenham Nov 11, 2023

atanasdinov Nov 13, 2023

rdoxenham Nov 11, 2023

atanasdinov Nov 13, 2023

atanasdinov Nov 13, 2023 •

edited

Loading

rdoxenham Nov 11, 2023

atanasdinov Nov 13, 2023

rdoxenham Nov 11, 2023

atanasdinov Nov 13, 2023

e-minguez left a comment

e-minguez Nov 13, 2023

e-minguez Nov 13, 2023

e-minguez Nov 13, 2023

e-minguez Nov 13, 2023

e-minguez Nov 13, 2023

e-minguez Nov 20, 2023

e-minguez Nov 20, 2023

e-minguez Nov 20, 2023

rdoxenham left a comment

		```

		NOTE: Interface names during the installation of nodes might differ from the preconfigured logical ones.

	This is necessary in order for NMC to identify which host it is running on when applying the network configurations later.
	This is required in order for NMC to identify which host it is running on when applying the network configurations later.

Update README #9

Update README #9

Conversation

atanasdinov commented Nov 10, 2023 • edited Loading

rdoxenham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atanasdinov Nov 13, 2023 • 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

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

atanasdinov Nov 13, 2023 • 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

Choose a reason for hiding this comment

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

rdoxenham left a comment

Choose a reason for hiding this comment

atanasdinov commented Nov 10, 2023 •

edited

Loading

atanasdinov Nov 13, 2023 •

edited

Loading

atanasdinov Nov 13, 2023 •

edited

Loading