-
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: Move nmstate options to new schema #1720
base: master
Are you sure you want to change the base?
Conversation
@@ -2,7 +2,7 @@ | |||
|
|||
declaration template components/network/core-schema; | |||
|
|||
final variable QUATTOR_TYPES_NETWORK_LEGACY ?= true; | |||
final variable QUATTOR_TYPES_NETWORK_LEGACY ?= false; |
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.
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.
👍 @aka7 is probably the best person to do that for MS. We have pretty good tools for this so it should be easy to generate, but may take a little bit to analyse any differences.
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.
@jrha thank you for this.
I plan to look into this next week.
I suspect backward incompatibility will be with nm_clean_inactive_conn too.
Presence of ':' in any of the values indicates this is IPv6 related. | ||
} | ||
type network_route = { | ||
@{The ADDRESS in ADDRESS/PREFIX via GATEWAY} |
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 would factor out all common bits. i don't know why you have this in the backend code
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.
This seemed the easiest option for having different validation rules per back-end, but I now realise I could redefine the validation function in each instead.
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.
}; | ||
|
||
type structure_network_backend_specific = { | ||
"nmstate" : structure_network_nmstate |
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.
why the nmstate subtructure? the options have a generic enough name
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 thought it best to make it clear, but happy to do whichever people prefer.
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.
Could we ever have manage_dns and nmstate/manage_dns? If they might conflict then putting it in a subtree makes sense, but creating a nmstate subtree might also be problematic if RH rename nmstate or move the manage_dns out of it?
OTOH, this currently looks to be a single item tree? In which case I am not sure it makes sense to push things down a level? What things do we expect to appear in structure_network_backend_specific versus structure_network_backend_specific/nmstate ?
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.
manage_dns was only added during nmstate configuration since network manager by default updates resolve.conf.
However upon looking at this few weeks ago for [device] configuration drop in file,
I think we need schema to configure options for [main] droppin file. one of its option is dns=none, which manage_dns adds in [main] config section, so I wonder if this should be refactored to add any override options of [main] configuration of networkmanager.conf drop in file.
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 prefer that schema name are neutral; and these are. the extra depth would only make sense if manage_dns
and nmstate/manage_dns
might be need (i.e. that they are both need to configure one or more things, yet can have different values).
this is not yet the case, so i would drop the extra nmstate
@jrha the backward incompatible is ok with us, We probably need to move manage_vips into nmstate path, because that is currently only for nsmstate, unless we plan to look at creating loopback interface for network.pm? If we are going have backward incompatible change, its worth looking at other nmstate on options I added recently. like the priority option added for nmstate config. but I'm struggling to override the two variable introduced in the schema at os level, the network types are included early on in quattor/types/system.pan (in our setup at least). So I'll be interested to know how others are going override these values QUATTOR_TYPES_NETWORK_BACKEND or QUATTOR_TYPES_NETWORK_LEGACY. Has anyone else tested this? cc @stdweird ? |
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.
Tested this internally at ms, LGTM.
Other than the comment made about overriding the variables. and we can possibly move other options into nmstate schema, this can be done later separate PR once this is merged?
Others should test and confirm though.
@stdweird @jrha just checking what is decision on this? At ms, we have tested this, with caveat that you can't override the two global variables early on before the network schema is included, but with some refactoring in our template code we can achieve this by putting in some logic to set the right values we need, legacy, vs initscripts, vs nmstate. And we are ok with backward incompatibility. I ask this, because there additional changes I have in flight which I'll create pr once this is merged. Also we are looking to go GA with rhel9 before the end of the year so any major changes, I would like to get this in before we go GA. hope this makes sense. thanks for your help. |
@aka7 as promised this splits out the nmstate specific parts of the schema into the new structure @stdweird designed.
This also defaults to using the new schema and to be honest I don't see a reason to keep the legacy schema file around.
Backwards incompatible from 23.6.0 for anyone setting
"nm_manage_dns" = true
or more generally for any sites using the current code from master.