openconfig / reference

This repository contains reference implementations, specifications and tooling related to OpenConfig-based network management.
Apache License 2.0
155 stars 88 forks source link

Add union_replace specification #188

Closed dplore closed 1 year ago

dplore commented 1 year ago

The union_replace specification defines how a new gNMI operation is performed which joins two origin datasets together and replaces the currently running configuration.

This document is a cut and paste of the gNMI union_replace proposal previously shared for OC Community and Operator review. However, there are several clarifications added based on feedback from the community collected into this markdown format. The most important are in the sections pertaining to handling of default values. Please carefully consider the default handling in case they don't meet your expectations or are unclear.

Also see the gNMI proto update

dplore commented 1 year ago

i think this cannot solely be a separate document, but it is meaty enough in its content to warrant one.

can we make changes to the specification to include the high-level operation of union_replace with references to this extension document where more clarity is required please?

i'll review the document here (i presume its similar to the one that was previously authored) after we agree here.

Agreed, the gNMI specification should be updated, I will add updates for it to this PR.

This doc is a cut and paste of the gNMI union_replace proposal shared for OC Community and Operator review. However, there are several clarifications added based on feedback from the community collected into this markdown format. The most important are in the sections pertaining to handling of default values. Please carefully consider the default handling in case they don't meet your expectations or are unclear.

hellt commented 1 year ago

Shall it be somehow synced with gnmi.proto changes and versions bumped synchronously in accordance?

dplore commented 1 year ago

Shall it be somehow synced with gnmi.proto changes and versions bumped synchronously in accordance?

Yes, I am intending to upgrade gnmi.proto to v 0.10.0 as well.

jsterne commented 1 year ago

The merge algorithm mentions to include "factory defaults" in 5.2.2 and 5.3.1. Some implementations have the concept of a "factory default" that is loaded if there is no saved config file (i.e. no startup datastore), and that is different than an "empty config" (i.e. delete at root). The factory default may contain, for example, a default user called "admin" or an event log that can be removed/deleted if desired. So I'm not sure if those items (e.g. admin user) should really be included back into the config during a union_replace.

jsterne commented 1 year ago

A more detailed example of what I mean by "factory default" (and contrasting that with an "empty" configuration):

1) When an network element (NE, e.g. router) boots up, it looks for a previously saved configuration (call this the "startup datastore"). 2) If no startup datastore is found, then the NE populates the running datastore with a set of "factory default" config statements. These config statements are indistinguishable from the same config being explicitly set by a client. The config statements appear in the output of "show configuration" (or whatever command the NE uses to view the contents of the running datastore, e.g. gNMI Get). The config statements can be deleted like any other config. 3) An example of some potential factory default config is a user called "admin" or an event log "99". 4) Some operators, for security reasons, don't want to have the factory default "admin" user or event log 99. So they delete them. 5) In their master config up in their client, the operator doesn't have any user called "admin" 6) When the operator does a union_replace they don't want to end up with any user called "admin" in the running config (it isn't up in their master config)

The new proposal by Darren ("and replace" instead of "and apply this to") does achieve this, but then I don't understand the point of step 1 of section 5.2.2. Why not just stay the candidate config starts completely empty ? (note - empty means there are lots of defaults being used by the NE, but defaults aren't explicit config, i.e. don't show up in "show configuration").

jsterne commented 1 year ago

Editorial comment: the sections skip from 5.3.3 to 5.3.6.

jsterne commented 1 year ago

Section "5.3.3.2 Overlapping values in CLI and OC" describes the two options for resolving issues. Why not allow the same two options for NY and OC? (5.3.6)?

dplore commented 1 year ago

Last call for comments. This will merge on June 10th, 2023.

@rgwilton @rolandphung @hellt

dplore commented 1 year ago

@jsterne I have responded to all your comments. PTAL at the latest revision.

hellt commented 1 year ago

Can we hold it from merging for a couple of days more?

jsterne commented 1 year ago

@jsterne I have responded to all your comments. PTAL at the latest revision.

Things are looking good for section 5.3.2 (NY+OC) is good (without a path, or a root path, means that everything in step 1 is removed).

It was really the last updates about "negation" that was causing me the most concern. The formatting of section 5.2.2 may be throwing me off (bullet numbering and indentation when I view it here: https://github.com/openconfig/reference/blob/63657555ae3ddddb44474a437d08011b247bf808/rpc/gnmi/gnmi-union_replace.md#532-union-behavior-for-native-and-openconfig) but we may want to be more clear that this "negation" part only applies to the native CLI part (i.e. is only relevant for CLI+OC).

dplore commented 1 year ago

@jsterne I have responded to all your comments. PTAL at the latest revision.

Things are looking good for section 5.3.2 (NY+OC) is good (without a path, or a root path, means that everything in step 1 is removed).

It was really the last updates about "negation" that was causing me the most concern. The formatting of section 5.2.2 may be throwing me off (bullet numbering and indentation when I view it here: https://github.com/openconfig/reference/blob/63657555ae3ddddb44474a437d08011b247bf808/rpc/gnmi/gnmi-union_replace.md#532-union-behavior-for-native-and-openconfig) but we may want to be more clear that this "negation" part only applies to the native CLI part (i.e. is only relevant for CLI+OC).

I left it open on purpose because I was thinking one could choose to change a factory default using their preferred origin (or available origin in the case where the factory default is not modeled in OC or NY).