-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ncm-network: nmstate - remove config route entry not in profile #1645
Conversation
85efc12
to
166b807
Compare
@@ -377,13 +393,13 @@ sub generate_nmstate_config | |||
# next-hop-interface: | |||
# and so on. | |||
my $routes = []; | |||
push @$routes, @{$self->make_nm_route_absent($name)}; | |||
push @$routes, \%default_rt if scalar %default_rt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit weird that at line 396 you are pushing on a single entry array containing a hashref but here you are appending a hashref instead. What does @$routes actually expect in it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@$routes, it contains array of hashref.
so will contain values such as
- next-hop-interface: bond0
state: absent
- destination: 0.0.0.0/0
next-hop-address: x.x.x.x
next-hop-interface: bond0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read perldoc -f push
and it says that it appends each element in the second list to the first. So line 390 is technically correct: the second arg must be a list. But I always recall using push like: push @a, $b;
so I guess it works to have a scalar as the second argument. Perl is weird.
…d of merging. nmstatectl apply does not replace running config instead merges current state config with desired state. This is not useful when settings have been removed such as policy routing. 'state: absent' will remove config routes entry that matches the interface as next hop and therefore will only apply the routes configured in host profile profile.
166b807
to
ab04411
Compare
LGTM but I'll let someone who actually understands the nm give the approval! |
closing PR in favour of one, #1647 |
nmstatectl apply does not replace running config instead merges current state config with desired state. This is not useful when settings have been removed such as policy routing.
'state: absent' will remove config routes entry that matches the interface as next hop and therefore will only apply the routes configured in host profile profile.
Why the change is necessary.
Allow capability to change policy routes or adding static routes after host is built and without having to rebuild the host.
What backwards incompatibility it may introduce.
None.
#1643