Cannot replace node with deviate/augment combo

[midakaloom]

We have a YANG module that we implement (ietf-routing-policy), but there are some changes that we've made. In order to maintain a separation between the IETF module and our modifications, instead of directly modifying the IETF module, we have removed unsupported nodes using deviations, and added new nodes by augmenting from a separate module.

The problem occurs when we remove an unsupported node and then try to replace it with a different node of the same name. The new node comes from a different module and therefore has a different prefix, but Entry.Dir only cares about unprefixed names. So when the deviations are applied, it wipes out the new node.

I'm aware that prefix handling is incomplete (#5) in goyang, and that we cannot change the way Entry.Dir works without serious consequences. I would like to propose a workaround, in which we maintain a secondary map Entry.PrefixedDir which acts exactly like Entry.Dir except that its map keys are qualified with a prefix.

I have a branch in which I've made these changes, but I wanted to get feedback before submitting a PR.

[wenovus]

• Does YANG allow changing a node via deviating and then augmenting it? Does pyang pass the check?
• Does YANG allow multiple nodes of the same name, but different prefixes, to exist at the same time?

@robshakir do you know the answer to the above questions? I'm finding that pyang doesn't like either.

If it does make sense to support, it looks likes a fine option to me, I'm just not sure if this behaviour is valid.

[midakaloom]

It seems to work for me (at least with pyang 2.3.2).

module foo {
  namespace "urn:foo";
  prefix "foo";

  container a {
    list b {
      key "c";
      leaf c {
        type string;
      }
    }

    leaf d {
        type string;
    }
  }
}

module foo-deviation {
  namespace "urn:foo-deviation";
  prefix "foo-deviation";

  import foo { prefix "foo"; }

  deviation "/foo:a/foo:b" {
    deviate not-supported;
  }
}

module foo-extra {
  namespace "urn:foo-extra";
  prefix "xfoo";

  import foo { prefix "foo"; }

  augment "/foo:a" {
    list b {
      key "c";
      leaf c {
        type uint32;
      }
    }

    leaf d {
        type string;
    }
  }
}

$ pyang -ftree --deviation foo-deviation.yang foo.yang foo-extra.yang 
module: foo
  +--rw a
     +--rw d?        string
     +--rw xfoo:b* [c]            // <-- replaced list b
     |  +--rw xfoo:c    uint32
     +--rw xfoo:d?   string       // <-- second leaf d

[midakaloom]

Additionally, the closest thing I can find in the spec that is a restriction on names is in 7.17 where it says (emphasis mine):

The "augment" statement MUST NOT add multiple nodes with the same
name from the same module to the target node.

[wenovus]

I see, I was putting the augment and deviation statements in the same YANG file. After I separated them into a different one like you did it worked. It must be because the augment wasn't from a different module like the spec asked.

If pyang's behaviour is correct, where

1. You can't use a uses, even from a different module, to duplicate (inner) node names (essentially uses instantiates the nodes within the module of "use"), and
2. augment, however, can duplicate node names by adding new nodes of the same name from different modules to the target module.

Then this means that the namespace of nodes within a scope is indeed "module:name". In that case, I agree with @midakaloom 's solution's direction. YANG identifiers cannot contain ":" so keeping the key type as string works, as does a struct which contains the module/prefix and the name.

I see an alternative to the proposal of adding a parallel PrefixedDir map to each Entry object:
Since string still works as a key, we don't really need to change the key type in the fix. Is it possible then to only provide the prefix/module to Dir's key when an augment happens, as that's the only case where this situation occurs?

So, another way of putting this is we fix the current handling of augments where we use the prefix or module name of the augmenting module to qualify the Entry name when we create it, so they cannot collide with existing entries. Dir entries will be prefixed if and only if the entry is augmented from a different module.

This is where the current augment implementation is:

goyang/pkg/yang/entry.go

Lines 1399 to 1423 in 1e0cba5

	// merge merges a duplicate of oe.Dir into e.Dir, setting the prefix of each
	// element to prefix, if not nil. It is an error if e and oe contain common
	// elements.
	func (e *Entry) merge(prefix *Value, namespace *Value, oe *Entry) {
	e.importErrors(oe)
	for k, v := range oe.Dir {
	v := v.dup()
	if prefix != nil {
	v.Prefix = prefix
	}
	if namespace != nil {
	v.namespace = namespace
	}
	if se := e.Dir[k]; se != nil {
	er := newError(oe.Node, `Duplicate node %q in %q from:
	%s: %s
	%s: %s`, k, e.Name, Source(v.Node), v.Name, Source(se.Node), se.Name)
	e.addError(er.Errors[0])
	} else {
	v.Parent = e
	v.Exts = append(v.Exts, oe.Exts...)
	e.Dir[k] = v
	}
	}
	}

Now this probably breaks some code somewhere, but maybe controlling it with a flag is acceptable.

The advantage of this method is consumers can flip the flag when converting to the new behaviour, instead of having to change all Dir references to PrefixedDir in addition to fixing the logic. Also we save some memory by storing less information.

@robshakir any thoughts?

[robshakir]

I will look at this today -- I want to be very careful with making changes to keys in Dir, since there are expectations from existing consumers here.

I'd like to try and approach this in a principled way so we review it in a design doc and consider pros and cons across the team before implementing. The root cause of this is that goyang made a simplification (as @midakaloom observes)such that it does not to use namespace:entity as the unique key for a child node, so there is always some chance of overlap (even without deviate/augment approaches). OpenConfig (and gNMI) removes these prefixes, based on the fact that we made this simplification throughout OpenConfig, since to introduce significant issues for a user.

[wenovus]

Can you provide an example of where without deviation or augment, we'd have a conflict? uses creates nodes where it's used, not where the grouping is defined.

[robshakir]

module a {
  prefix "a";
  namespace "urn:a";
 
 leaf foo { type string; }
}

module b {
  prefix "b";
  namespace "urn:b";
  
  leaf foo { type string; }
}

In this case foo will clash. In the real-world this happens in both ietf-interfaces and openconfig-interfaces where we have the clash of interfaces at the root.

[hellt]

I wonder why does it clash, given that the namespaces are different for the modules? Just today we've been discussing why its needed to -exclude ietf-interfaces when ygot generates schemas for openconfig-interfaces?

[robshakir]

In this case, there's only a clash if we look at the 'root' '/' that is not owned by any module itself. ygot does this, so ends up with a clash because namespaces are not maintained. The same scenario is being discussed above, where two different modules are inserting into some shared entity (actually an entity owned by another module).

In ygot, we made this choice because the developer experience of needing to know the provenance of the module for every node in the tree is pretty horrific - e.g., we might need to have calls that look like root.GetOrCreateOpenConfigInterfacesInterfaces().GetOrCreateOpenConfigInterfacesInterface().GetOrCreateOpenConfigInterfacesConfig()..... and struct names like OpenConfigInterfacesInterfaces_OpenConfigInterfacesInterface_OpenConfigInterfacesConfig.

[wenovus]

Since this is a ygot-specific issue, i.e. merge the modules without accounting for namespace:
https://github.com/openconfig/ygot/blob/50b44fcf38c0a20a5828232e058ad58ce1204d02/ygen/codegen.go#L1093-L1105

I think we can fix this particular issue just within ygot, such that as far as goyang is concerned, only augment can cause a conflict.

[robshakir]

I don't think this is solely in ygot. My example was - but:

module a {
  namespace a;
  prefix "urn:a";
  container a { }
}

module b {
  namespace b;
  prefix "urn:b";
  import a { prefix a; }
  
 augment /a:a { leaf c { type string; } }
}

module c {
  namespace "c";
  prefix "urn:c";
  import a { prefix a; }

 augment /a:a { leaf c { type string; } }
}

This will throw an error in goyang/pkg/yang I believe.

[midakaloom]

It seems that the duplicate key checking is actually missed in this particular case.

The check in Entry.add() doesn't catch it because add() is called on each individual augment, before they are merged.

And the check in Entry.merge() catches it, but the error is never found because GetErrors() never gets called again (there are no remaining modules, so the loop here is never entered).

$ ./goyang -f tree a.yang b.yang c.yang 
rw: a:a {
  rw: a:a {
    rw: string c:c
  }
}
rw: b:b {
}
rw: c:c {
}

[wenovus]

@robshakir Yes I agree augment can cause conflicts. My point is that for goyang, the conflicts can only arise from an augment. If we add the prefix to the Entry key for augments only, then there might be less impact on downstream code.

[robshakir]

I'm not quite sure that's true -- augment can be used in a number of different scenarios, many of which won't end up with clashing names -- if we do this for all augmented nodes then we'll end up with user impact - and if not, we probably have to think about whether there's a way that we can determine when to prefix, since this is dependent upon the particular module set that we're called with.

[wenovus]

Thanks @midakaloom, it looks like this is a bug in the error handling code. I've verified that capturing the error reports the Duplicate node "c" error message on Rob's example whereas it is currently silently passing.

[wenovus]

The least impactful way would be to only prefix when there is a conflict. Rob's example is an edge case: the naming would need to be done after processing each augment in order for the conflict to surface.

[midakaloom]

My 2 cents: the least impactful way would be to introduce either another directory that includes prefixes (as I suggested) or to use a flag to determine behaviour (as @wenovus suggested). That way existing code doesn't get broken, and the style used within a structure remains internally consistent.

[robshakir]

@wenovus -- the problem is that this will give non-deterministic behaviour in the Entry tree that we output. For example, if I run with module a and b from above, then we'd prefix nothing, when I run with a, b and c in the future then there would be different naming (for b's version of leaf c). Consuming code can't necessarily keep consistent naming for b:c (mitigate these changes) - without having other handling to understand the module provenance of all entries in all cases (i.e., remember that b:c was called c, such that c:c should be assigned a different name) - so I think even this non-intrusive change would mean that consuming applications have to make changes to be robust if they want consistent output to be generated across different module sets.

I think @midakaloom's suggestions here are reasonable alternatives. AFAICS, the solution space is:

a. Prefix all children's names.
b. Prefix all children's names when asked to by a flag.
c. Prefix all entries in a new data structure.
d. Selectively prefix based on observed collisions.

Let's list out the pros and cons of each, and use this to decide what approach we want to take.

[wenovus]

What about repeating options a-c (e-f) with: Prefix only nodes created by an augment?

…

On Wed, Oct 7, 2020 at 4:47 PM Rob Shakir ***@***.***> wrote: @wenovus <https://github.com/wenovus> -- the problem is that this will give non-deterministic behaviour in the Entry tree that we output. For example, if I run with module a and b from above, then we'd prefix nothing, when I run with a, b and c in the future then there would be different naming. Consuming code can't necessarily mitigate these changes - without having other handling to understand the module provenance of all entries in all cases - so I think even this non-intrusive change would mean that consuming applications have to make changes to be robust if they want consistent output to be generated across different module sets. I think @midakaloom <https://github.com/midakaloom>'s suggestions here are reasonable alternatives. AFAICS, the solution space is: a. Prefix all children's names. b. Prefix all children's names when asked to by a flag. c. Prefix all entries in a new data structure. d. Selectively prefix based on observed collisions. Let's list out the pros and cons of each, and use this to decide what approach we want to take. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#146 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMEG6EHY4XERWFLHPWAVU73SJTHUNANCNFSM4SBX53XQ> .

[robshakir]

Sure - I'm open to adding additional options here -- can you start a doc please? (Google doc is probably best, we can share it with interested contributors.)

[wenovus]

I'm a bit worried here, does gNMI support qualified names in its path proto?

If we do make the namespace (module name, node name), then this means that devices need to stream prefixed paths at least for this edge case. Is this something we want to do?

[robshakir]

gNMI does not support prefixed paths - currently by design. Based on the usability problems I described above this is something we tried to avoid.

Given that gNMI and YANG are separable, then I don't think that we need to ensure that both technologies support this. goyang can support the use case above, but not gNMI.

[midakaloom]

Is there a doc that got created for this? I'd like to be able to follow the progress if possible.

[wenovus]

Ok, here it is: design doc.

[aish-bigb]

hi @midakaloom, i am facing the same issue mentioned in this open issue. i saw your comment that you have a solution handy and you have implemented the same. could you please share the details of your branch?