openconfig / ygot

A YANG-centric Go toolkit - Go/Protobuf Code Generation; Validation; Marshaling/Unmarshaling
Apache License 2.0
286 stars 107 forks source link

Clarification -exclude_modules with modules that use augmentation. #634

Open hansthienpondt opened 2 years ago

hansthienpondt commented 2 years ago

Hi,

I've got a question regarding the -exclude_modules flag in the generator.

When loading a bunch of yang files (without knowing which file contains which module), I'm trying to exclude a number of modules. The problem is, if these excluded modules contain some 'augment' statements towards an imported module, the goStructs are not generated, and error with following message:

ERROR Generating GoStruct Code: could not resolve [/compressed/root/augmented/config]() into a defined struct

the yang files with which I'm reproducing this: are augmented.yang and compressed.yang

Looking at the code, modules are parsed/processed before the exclusion logic is being handled. https://github.com/openconfig/ygot/blob/ade465147437fcb54f2451327ee419f6f87c79dc/ygen/codegen.go#L906-L916

However, it seems like the result of https://github.com/openconfig/ygot/blob/ade465147437fcb54f2451327ee419f6f87c79dc/ygen/codegen.go#L869 seems to contain the augmented paths of the module I'm trying to skip in the non-augmented module.

here https://github.com/openconfig/goyang/blob/de640d098cc1ef2b962c0fca3d3b710ebe1113ea/pkg/yang/entry.go#L98 and https://github.com/openconfig/goyang/blob/de640d098cc1ef2b962c0fca3d3b710ebe1113ea/pkg/yang/entry.go#L117

Which makes me wonder if -exclude_modules is being invoked, if this should be passed to goyang in order for it not to add the augmented paths to the Dir map and Augmented list, or if there should be some logic in ygot that figures out a path is augmented and originated from a skipped module?

Obviously, when the file augmented.yang is not passed as an input yang file to generator.go, it compiles without any issues ; if the above is expected behaviour, what's the value of an -exclude_modules flag?

I've started chasing down this, after PopulateDefaults() rendered objects defined by the yang model but restricted by the FEATURE/IF-FEATURE statements. After seeing https://github.com/openconfig/goyang/blob/master/pkg/yang/entry.go#L977-L996 I suppose this is not yet supported in goyang/ygot? Are there any plans to?

Thanks! Kind Regards, Hans

wenovus commented 2 years ago

Thanks Hans for the detailed report, I reproduced it using compress=false, and if I add another container under augmented-top I can reproduce using compress=true.

The root cause of this issue is that nodes that are two or more levels deep within an excluded module are being excluded from a temporary look-up map (dirs within mappedDefinitions) during processing, and so a later processing step errors out as it can't find this information that it expects.

If you're wondering about the details, in the snippet below the child is always added to dirs, and only on the next level of recursion do we stop, so any further children are not added to dirs. The top level is never added to dirs because the module exclusion check occurs at the beginning of the function: https://github.com/openconfig/ygot/blob/ade465147437fcb54f2451327ee419f6f87c79dc/ygen/codegen.go#L1079-L1082

There are two steps to fixing this problem:

  1. Question: What is the intended behaviour of exclude-modules? Only exclude top-level definitions from the excluded module? or, to exclude both top-level definitions and non-top-level definitions that have been augmented into other modules?
  2. Once we have answered 1., then we need to fix the current implementation, which incorrectly does both of these.

@robshakir Would be good to have your thoughts here, also on the if-feature question, which I assume OpenConfig does not use so that's why we have not implemented it.