ncm-network: support for managing network using nmstate #1601

aka7 · 2023-07-07T22:19:06Z

starting a new module to manage networking using nmstate declarative approach. reusing functions from network.pm as much as possible. Changes made in network.pm is minimal and should be backward compatible for existing configs.

following is supported for initial tech preview.

single interface, and bonding
vlan inteface
policy routing
ethtool setting via cli support only.
new nm_manage_dns option is added to schema to decided if NM should manage dns, we use ncm-resolver so to avoid clash. NM by default manages dns which will intefere with ncm-resolver.
aliases, bridge etc configuration not yet supported, and possibly many other advance options I haven't thought about yet.

Why the change is necessary.
ifcfg support on rhel9 is not available. need a Modern way to manage networking.
What backwards incompatibility it may introduce.
None. this is new module. any changes made in parent is minimal to support this approach. should be noop to ifcfg configuration.

All tests performed on rhel9 hosts.

stdweird · 2023-07-11T08:41:05Z

@aka7 ping to join the meeting

stdweird · 2023-07-11T13:05:50Z

ncm-network/src/main/pan/components/network/config-nmstate.pan

+
+# Add dependency that can't be added to rpm directly
+prefix '/software/packages';
+'nmstate' = dict();


you want to add NetworkManager-config-server here, and possibly in the early list of kickstart rpms, like

prefix "/system/aii/osinstall/ks"; "NetworkManager-config-server" = dict();

mask the nmstate service

"/software/components/systemd/unit/nmstate/state" = "masked";

(or something like that).

@stdweird configuring another component in the context of a component configuration is quite unusual... At the same time I understand that it is a requirement and is probably better done here that somewhere in the template library. A site may use this component without using the template library or using an outdated version of it... May be worth a comment at least...

@jouvin i don't follow. the relevant comment is there: this is for rpm dependencies that we can set in the component

ncm-network/src/main/perl/nmstate.pm

starting a new module to manage networking using nmstate declarative approach. reusing functions from network.pm as much as possible. Changes made in network.pm is minimal and should be backward compatible for existing configs. following is supported for initial tech preview. - single interface, and bonding - vlan inteface - policy routing - ethtool setting via cli support only. - new nm_manage_dns option is added to schema to decided if NM should manage dns, we use ncm-resolver so to avoid clash. NM by default manages dns which will intefere with ncm-resolver. - aliases, bridge etc configuration not yet supported, and possibly many other advance options I haven't thought about yet.

Some commits to clarify perl structure

jouvin · 2023-07-14T07:22:20Z

ncm-network/src/main/pan/components/network/config-nmstate.pan

+
+# Add dependency that can't be added to rpm directly
+prefix '/software/packages';
+'nmstate' = dict();


@stdweird configuring another component in the context of a component configuration is quite unusual... At the same time I understand that it is a requirement and is probably better done here that somewhere in the template library. A site may use this component without using the template library or using an outdated version of it... May be worth a comment at least...

ncm-network/src/main/pan/components/network/core-schema.pan

- changes to generate the right hash entries needed by nmstate - addded deletion of conneciton if config is removed. - if nm_manage_dns then add resolver config for nmstate and apply.

ncm-network: nmstate: more unittest fixes

nmstate determines this automatically when applied but adding it for clearity.

- changed so config can be set accodringly when nm_manage_dnsis set. - added mtu if set.

ncm-network/src/main/pan/components/network/core-schema.pan

ncm-network/src/main/perl/nmstate.pm

stdweird · 2023-07-18T09:12:29Z

ncm-network/src/main/perl/nmstate.pm

+ my @data = ('[main]');
+
+ if ( $manage_dns ) {
+ # set nothing, will use default.


if the behaviour of nm_manage_dns = true means to not disable it, it needs a very clear explanaiton in the the schema.
i would force it here and not rely on default behaviour (eg another config file might set it to none).

stdweird · 2023-07-18T09:13:07Z

ncm-network/src/main/perl/nmstate.pm

+
+ if ( $manage_dns ) {
+ # set nothing, will use default.
+ $self->verbose("Networkmanager defaults will be used to manage resolv.conf");


could be other config files as well

ncm-network/src/main/perl/nmstate.pm

stdweird · 2023-07-18T10:05:18Z

ncm-network/src/main/perl/nmstate.pm

+
+# group all eth bound to a bond together in a hashref for to be used as
+# - port in nmstate config file
+sub get_bonded_eth


this hwole sub is

[ grep {defined($interfaces->{$_}->{master})} sort keys %$interfaces ]

But perhaps the sub is easier to read?

stdweird · 2023-07-18T10:13:34Z

ncm-network/src/main/perl/nmstate.pm

+ $filestatus = $NEW;
+ }
+ } else {
+ if ($filename ne $NM_RESOLV_YML)


invert the logc and swap the blocks: instead of

if not something x else y

it's easier to read and understand to do

if something: y else x

stdweird · 2023-07-18T10:14:40Z

ncm-network/src/main/perl/nmstate.pm

+ return $nm_dns_config
+}
+
+# enable NetworkManager service


enable and start?

stdweird · 2023-07-18T10:14:54Z

ncm-network/src/main/perl/nmstate.pm

+# nmstate service applies all files found in /etc/nmstate and changes to .applied, which will keep changing if component is managing the .yml file.
+# we don't need this.
+#
+sub disable_nmstate_service


disable and stop?

stdweird · 2023-07-18T10:17:58Z

ncm-network/src/main/perl/nmstate.pm

+ # it means this connection existed before nmstate did its first apply.
+ # doesn't break anything as nmstate resuses the conn, but worth a warning to highlight it?
+ if ("$name" ne "$ifacename"){
+ $self->warn("connection name '$name' doesn't match $ifacename for device $dev, possible connection reuse occured")


i would add the whole $output in the message to help debugging.

stdweird · 2023-07-18T10:19:49Z

ncm-network/src/main/perl/nmstate.pm

+ foreach my $conn_name (@existing_conn) {
+ $conn_name =~ s/\s+$//;
+ if ($conn_name =~ /Wired connection/){
+ $self->verbose("Clearing default connections created automatically by NetworkManager [ $conn_name ]");


make sure to add the extra rpm in kickstart and packages to make sure this doesn't happen in the first place

stdweird · 2023-07-18T10:23:47Z

ncm-network/src/main/perl/nmstate.pm

+ push(@cmds, [$NMSTATECTL, "apply", $NM_RESOLV_YML]);
+ my $out = $self->runrun(@cmds);
+ $self->verbose($out);
+ $nwsrv->reload();


why is this needed?

jrha · 2023-08-04T10:05:34Z

As requested, I'm going to squash and merge this into 23.6 as a technology preview.
The remaining comments still need to be looked at one way or another though.

ncm-network/src/main/pan/components/network/core-schema.pan

ncm-network/src/main/perl/nmstate.pm

ncm-network/src/test/perl/nmstate_simple.t

aka7 · 2023-08-04T13:03:31Z

Thanks James, I'm working on the other comments, all minor really. will create a new PR for that.

…

On Fri, 4 Aug 2023 at 11:05, James Adams ***@***.***> wrote: As requested, I'm going to squash and merge this into 23.6 as a technology preview. The remaining comments still need to be looked at one way or another though. — Reply to this email directly, view it on GitHub <#1601 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADLYRYQRVC4KHI4R6NVHKNLXTTCPTANCNFSM6AAAAAA2CLAWLY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- A. Karim

* ncm-network: support for managing network using nmstate A new module to manage networking using nmstate declarative approach. Reuses functions from network.pm as much as possible. Changes made in network.pm are minimal and should be backward compatible for existing configs. The following is supported in the initial tech preview: - single interface and bonding - vlan interface - policy routing - ethtool setting via cli support only. - new nm_manage_dns option is added to schema to decided if NM should manage DNS, we use ncm-resolver so to avoid clash. NM by default manages DNS which will interfere with ncm-resolver. - aliases, bridge etc configuration not yet supported, and possibly many other advanced options I haven't thought about yet. --------- Co-authored-by: Abdul Karim <Abdul.Karim@morganstanley.com> Co-authored-by: stdweird <stijn.deweirdt@ugent.be>

stdweird reviewed Jul 11, 2023

View reviewed changes

ncm-network/src/main/perl/nmstate.pm Outdated Show resolved Hide resolved

aka7 force-pushed the ncm_network_nmstate branch from f3a254b to 2be2fab Compare July 11, 2023 13:45

stdweird and others added 8 commits July 11, 2023 17:29

ncm-network: support custom get_current_config_post (for subclassing)

940fff1

ncm-network: cleanup link_aggregation code a bit

007355d

ncm-network: export common readonly for subclassing

4deb7d0

ncm-network: more nmstate cleanup

4d16d16

ncm-network: exported readonly must be non-local variables (ie use our)

243cb1f

ncm-network: make some progress with nmstate tests

38be4f6

ncm-network: nmstate: further testing

fa49d88

Merge pull request #3 from stdweird/aka7-ncm_network_nmstate2

0aada3b

Some commits to clarify perl structure

jouvin reviewed Jul 14, 2023

View reviewed changes

change to create right entries for hash used for nmstate yml config

9253e89

- changes to generate the right hash entries needed by nmstate - addded deletion of conneciton if config is removed. - if nm_manage_dns then add resolver config for nmstate and apply.

aka7 force-pushed the ncm_network_nmstate branch from 58e1b63 to 9253e89 Compare July 14, 2023 09:33

Abdul Karim and others added 5 commits July 14, 2023 11:15

set nm_manage_dns to false by default.

d748936

ncm-network: nmstate: more unittest fixes

af1e8e9

Merge pull request #4 from stdweird/aka7-ncm_network_nmstate3

67a8d31

ncm-network: nmstate: more unittest fixes

adding family ipv4 for route-rules

0f94cfc

nmstate determines this automatically when applied but adding it for clearity.

ncm-network: change to allow nm to manage or not to manage dns

d244ea5

- changed so config can be set accodringly when nm_manage_dnsis set. - added mtu if set.

aka7 force-pushed the ncm_network_nmstate branch from 99edc9f to d244ea5 Compare July 17, 2023 13:26

fix panlint

c4904e7

stdweird reviewed Jul 18, 2023

View reviewed changes

ncm-network/src/main/pan/components/network/core-schema.pan Outdated Show resolved Hide resolved

stdweird reviewed Jul 18, 2023

View reviewed changes

ncm-network/src/main/perl/nmstate.pm Outdated Show resolved Hide resolved

stdweird reviewed Jul 18, 2023

View reviewed changes

ncm-network/src/main/perl/nmstate.pm Outdated Show resolved Hide resolved

stdweird reviewed Jul 18, 2023

View reviewed changes

ncm-network/src/main/perl/nmstate.pm Outdated Show resolved Hide resolved

stdweird reviewed Jul 18, 2023

View reviewed changes

ncm-network/src/main/perl/nmstate.pm Outdated Show resolved Hide resolved

stdweird reviewed Jul 18, 2023

View reviewed changes

ncm-network/src/main/perl/nmstate.pm Outdated Show resolved Hide resolved

stdweird reviewed Jul 18, 2023

View reviewed changes

ncm-network: nmstate: fix unittests with supported interface names

8fb443b

aka7 marked this pull request as ready for review August 3, 2023 18:57

jrha added this to the 23.6 milestone Aug 4, 2023

jrha added the technology preview label Aug 4, 2023

jrha reviewed Aug 4, 2023

View reviewed changes

jrha added 2 commits August 4, 2023 11:53

Nitpick comments and annotations

af44ac5

Remove superfluous quotes

49319a6

jrha force-pushed the ncm_network_nmstate branch from 6a9e2bf to 669859c Compare August 4, 2023 10:55

Fix typo in debug message

5c95662

jrha force-pushed the ncm_network_nmstate branch from 669859c to 5c95662 Compare August 4, 2023 10:57

jrha merged commit 7627872 into quattor:master Aug 4, 2023
2 checks passed

jrha mentioned this pull request Jul 25, 2024

ncm-network: networkmanager module to configure using keyfile format #1587

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ncm-network: support for managing network using nmstate #1601

ncm-network: support for managing network using nmstate #1601

aka7 commented Jul 7, 2023 •

edited by jrha

Loading

stdweird commented Jul 11, 2023

stdweird Jul 11, 2023

stdweird Jul 11, 2023

jouvin Jul 14, 2023

stdweird Jul 18, 2023

jouvin Jul 14, 2023

stdweird Jul 18, 2023

stdweird Jul 18, 2023

stdweird Jul 18, 2023

jrha Aug 4, 2023

stdweird Jul 18, 2023

stdweird Jul 18, 2023

stdweird Jul 18, 2023

stdweird Jul 18, 2023

stdweird Jul 18, 2023

stdweird Jul 18, 2023

jrha commented Aug 4, 2023

aka7 commented Aug 4, 2023 via email

ncm-network: support for managing network using nmstate #1601

ncm-network: support for managing network using nmstate #1601

Conversation

aka7 commented Jul 7, 2023 • edited by jrha Loading

stdweird commented Jul 11, 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

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

jrha commented Aug 4, 2023

aka7 commented Aug 4, 2023 via email

aka7 commented Jul 7, 2023 •

edited by jrha

Loading