panic: interface conversion: yang.Node is nil, not *yang.Module

[scottmbaker]

I've stumbled onto a problem when calling ytypes.GetOrCreateNode(). The following is a chunk of my schema:

      list connectivity-service {
        key "connectivity-service";
        leaf connectivity-service {
          type leafref {
            path "/cs:connectivity-service/cs:connectivity-service/cs:id";
          }            
          description
            "Link to connectivity services where configuration should be pushed for this enterprise's devices";
        }
        ...
      }

We call ytypes.GetOrCreateNode() which in turn calls Find on the path /cs:connectivity-service/cs:connectivity-service/cs:id. This causes Find to backup to the root, and then it encounters Entries that have a Nil node. It's failing here:

               # /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/goyang/pkg/yang/entry.go:1249
        // Since this module might use a different prefix that isn't
        // the prefix that the module itself uses then we need to resolve
        // the module into its local prefix to find it.
        pfxMap := map[string]string{
            // Seed the map with the local module - we use GetPrefix just
            // in case the module is a submodule.
            e.Node.(*Module).GetPrefix(): e.Prefix.Name, // crash here
        }

The entry e at the time of the crash looks like this:

&{Parent:<nil> Node:<nil> Name:device Description: Default: Units: Errors:[] Kind:Directory Config:unset Prefix:<nil> Mandatory:unset Dir:map[access-profile:0xc000102d80 apn-profile:0xc000103800 connectivity-service:0xc0003a4780 enterprise:0xc0003a5200 qos-profile:0xc0003a5e00 security-profile:0xc0003a8a80 subscriber:0xc0003a9680 up-profile:0xc0003afe00] Key: Type:<nil> Exts:[] ListAttr:<nil> RPC:<nil> Identities:[] Augments:[] Augmented:[] Deviations:[] Deviate:map[] deviatePresence:{hasMinElements:false hasMaxElements:false} Uses:[] Extra:map[] Annotation:map[isFakeRoot:true schemapath:/ structname:Device] namespace:<nil>}

Possibly a related issue over in the goyang tracker at https://github.com/openconfig/goyang/issues/97.

[robshakir]

Thanks for this - we know that both Parent and Node will be nil when we're traversing these entries, since that is the deserialised form that we get from the schema -- so I don't think this is a goyang issue, but rather specific to ygot. ytypes.GetOrCreateNode should be safe though, so this is a case that we need to understand a bit more.

Can you provide the stack trace from the crash? I think we need to be further up the stack to understand where this is called directly.

[scottmbaker]

Sure, here you go:

panic: interface conversion: yang.Node is nil, not *yang.Module

goroutine 71 [running]:
github.com/openconfig/goyang/pkg/yang.(*Entry).Find(0xc00033ed80, 0xc0000b0d20, 0x2a, 0xe)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/goyang/pkg/yang/entry.go:1249 +0xd65
github.com/openconfig/ygot/ytypes.makeValForInsert.func1(0xc000340090, 0xe, 0xcf4140, 0xc00057b360)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/list.go:396 +0x38c
github.com/openconfig/ygot/ytypes.makeValForInsert(0xc00033ec00, 0xcb9b80, 0xc0003d7ce0, 0xc0003d7d40, 0x40ca88, 0x60, 0xdb2d20, 0x1, 0xc0000baf60)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/list.go:427 +0x4f4
github.com/openconfig/ygot/ytypes.insertAndGetKey(0xc00033ec00, 0xcb9b80, 0xc0003d7ce0, 0xc0003d7d40, 0x0, 0x0, 0xc0005abc00, 0x2)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/list.go:499 +0xf5
github.com/openconfig/ygot/ytypes.retrieveNodeList(0xc00033ec00, 0xcb9b80, 0xc0003d7ce0, 0xc00031d800, 0xc00031d780, 0x1000000, 0x0, 0x0, 0x0, 0xc00057b301, ...)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/node.go:318 +0x74e
github.com/openconfig/ygot/ytypes.retrieveNode(0xc00033ec00, 0xcb9b80, 0xc0003d7ce0, 0xc00031d800, 0xc00031d780, 0x1000000, 0x0, 0x0, 0x0, 0xc000544fd8, ...)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/node.go:93 +0x5d5
github.com/openconfig/ygot/ytypes.retrieveNodeContainer(0xc00033ea80, 0xcf40a0, 0xc0003d7c50, 0xc00031d180, 0xc00031d100, 0x1000000, 0x0, 0x0, 0x0, 0xc00057b101, ...)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/node.go:184 +0x717
github.com/openconfig/ygot/ytypes.retrieveNode(0xc00033ea80, 0xcf40a0, 0xc0003d7c50, 0xc00031d180, 0xc00031d100, 0x1000000, 0x0, 0x0, 0x0, 0xc000544fc8, ...)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/node.go:91 +0x685
github.com/openconfig/ygot/ytypes.retrieveNodeContainer(0xc000339800, 0xcf4000, 0xc0000bac00, 0xc0007a9000, 0xc0007a9100, 0x1000000, 0x0, 0x0, 0x0, 0xf2b9e0, ...)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/node.go:184 +0x717
github.com/openconfig/ygot/ytypes.retrieveNode(0xc000339800, 0xcf4000, 0xc0000bac00, 0xc0007a9000, 0xc0007a9100, 0x1000000, 0x0, 0x0, 0x0, 0xd5a280, ...)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/node.go:91 +0x685
github.com/openconfig/ygot/ytypes.retrieveNodeList(0xc000339800, 0xcb9b20, 0xc0003d6ae0, 0xc0007a8f00, 0xc0007a8e80, 0x1000000, 0x0, 0x0, 0x0, 0xc00057a501, ...)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/node.go:322 +0x907
github.com/openconfig/ygot/ytypes.retrieveNode(0xc000339800, 0xcb9b20, 0xc0003d6ae0, 0xc0007a8f00, 0xc0007a8e80, 0x1000000, 0x0, 0x0, 0x0, 0xc000544b58, ...)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/node.go:93 +0x5d5
github.com/openconfig/ygot/ytypes.retrieveNodeContainer(0xc000339680, 0xcf3f60, 0xc0000b8018, 0xc0007a8d80, 0xc0007a8d00, 0x1000000, 0x0, 0x0, 0x0, 0xc00057a401, ...)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/node.go:184 +0x717
github.com/openconfig/ygot/ytypes.retrieveNode(0xc000339680, 0xcf3f60, 0xc0000b8018, 0xc0007a8d80, 0xc0007a8d00, 0x1000000, 0x0, 0x0, 0x0, 0xc000544b48, ...)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/node.go:91 +0x685
github.com/openconfig/ygot/ytypes.retrieveNodeContainer(0xc000082c00, 0xcda080, 0xc0005ab140, 0xc0007a8c80, 0x0, 0x1000000, 0x0, 0x0, 0x0, 0x48adb8, ...)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/node.go:184 +0x717
github.com/openconfig/ygot/ytypes.retrieveNode(0xc000082c00, 0xcda080, 0xc0005ab140, 0xc0007a8c80, 0x0, 0x1000000, 0x0, 0x0, 0x0, 0xc0005ab140, ...)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/node.go:91 +0x685
github.com/openconfig/ygot/ytypes.GetOrCreateNode(0xc000082c00, 0xcda080, 0xc0005ab140, 0xc0007a8c80, 0x2, 0x2, 0xc000440360, 0x115, 0x1)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/ygot/ytypes/node.go:339 +0x7a
github.com/onosproject/sdcore-adapter/pkg/gnmi.(*Server).doReplaceOrUpdate(0xc0003a1800, 0xc0003d64e0, 0x3, 0x0, 0xc00031cc80, 0xc000535540, 0xc0001840f0, 0x0, 0x0)
        /go/src/github.com/onosproject/sdcore-adapter/pkg/gnmi/set.go:178 +0xe40
github.com/onosproject/sdcore-adapter/pkg/gnmi.(*Server).Set(0xc0003a1800, 0xf40560, 0xc0003d6180, 0xc00013b290, 0x0, 0x0, 0x0)
        /go/src/github.com/onosproject/sdcore-adapter/pkg/gnmi/set.go:301 +0x981
github.com/onosproject/sdcore-adapter/pkg/target.(*server).Set(0xc0003a5ef0, 0xf40560, 0xc0003d6180, 0xc00013b290, 0xc0003a5ef0, 0xc0003d6180, 0xc0001faba0)
        /go/src/github.com/onosproject/sdcore-adapter/pkg/target/set.go:23 +0x123
github.com/openconfig/gnmi/proto/gnmi._GNMI_Set_Handler(0xd9bc80, 0xc0003a5ef0, 0xf40560, 0xc0003d6180, 0xc0005480c0, 0x0, 0xf40560, 0xc0003d6180, 0xc000393600, 0x576)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/github.com/openconfig/gnmi/proto/gnmi/gnmi.pb.go:3399 +0x217
google.golang.org/grpc.(*Server).processUnaryRPC(0xc0003c2380, 0xf4a260, 0xc00019c780, 0xc000386b00, 0xc0003a5f80, 0x153fd50, 0x0, 0x0, 0x0)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/google.golang.org/grpc/server.go:1180 +0x50a
google.golang.org/grpc.(*Server).handleStream(0xc0003c2380, 0xf4a260, 0xc00019c780, 0xc000386b00, 0x0)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/google.golang.org/grpc/server.go:1503 +0xcfd
google.golang.org/grpc.(*Server).serveStreams.func1.2(0xc00018ac60, 0xc0003c2380, 0xf4a260, 0xc00019c780, 0xc000386b00)
        /go/src/github.com/onosproject/sdcore-adapter/vendor/google.golang.org/grpc/server.go:843 +0xa1
created by google.golang.org/grpc.(*Server).serveStreams.func1
        /go/src/github.com/onosproject/sdcore-adapter/vendor/google.golang.org/grpc/server.go:841 +0x204

Thanks! Scott

[robshakir]

Thanks!

This looks like where I thought this might show up - we're using Find to retrieve this, and the leafref you have is an absolute path for the key of the list rather than the relative path that it would be in most OpenConfig models.

I'll take a look into this to try and see whether there's a way that we can replace the Find with something safer here.

[scottmbaker]

It's probably an acceptable solution for us to switch to relative path (../../cs:connectivity-service/cs:connectivity-service/cs:id) or something along those lines (I can play with it) if that would be more appropriate.

[robshakir]

That's certainly the easiest workaround (I believe that this should work fine), if the cs is the local module, you probably also don't need to prefix with this. However, I'm looking into this bug - since we shouldn't panic when this occurs.

[wenovus]

There is no way to marshal the Node field without removing its circular references as the json package doesn't handle it. I don't see where Node is set to nil, but I'm assuming this is what ygot does.

[robshakir]

The issue we have is that we're using Find in the schema tree that we unmarshalled to find the target of the leafref that is being used as the key. I think we can probably fix this by (rather than having the Node available to us), doing this resolution ourselves within the schema tree, knowing that we have this limitation.

The exact issue is that we're getting to the root, then needing to do a lookup against a particular module's prefix within our context. Given that for ygen we know that we will not have two parents with the same name at this point, then we can just search on node name. I'll send you something to look at for this.

[wenovus]

That makes sense to me. However, should we have a PR to enforce the no-conflicting-top-level-name property first? I just tested, and currently the conflict is only detected and an error thrown for compressed schemas.

Also, I'm not sure what the situation is for generated code that doesn't use a fakeroot.

[robshakir]

Interesting -- I need to recall what we do with the schematree in the case that we don't have a fakeroot. I think that if you have two conflicting names right now, the schema tree might not be robust to this.

I'm adding an integration test and will send you a PR.

[wenovus]

Ok, I see buildJSONTree always creates a fakeroot regardless of whether generate_fakeroot is set; the flag only determines whether some extra information is added. The proposal sounds good to me.

[robshakir]

Yes - sorry, the 'fakeroot' is just the struct that corresponds to / in the YANG schema, we always create a / in the schema.

[wenovus]

I guess this no-conflict property is openconfig-specific, however -- are we ok with that for now?

[robshakir]

I think that this is something that we're generally assuming across the toolchain today - i.e., goyang also makes the same simplification. If I create two nodes underneath a specific parent that have the same name, then the Dir will end up with having conflicting entries.

Going forward, we probably do want to fix this (we've discussed this before), but this is a relatively major set of changes.

[wenovus]

The conflict that I meant here is slightly different than the augmentation issue. Here, if we defined (instead of augmented) top-level nodes with the same name in different modules, I believe goyang avoids the conflict in this case by storing Modules at the top-level map instead of Entry, but ygot merges the trees into the fakeroot, so it's only ygot that introduces this type of conflict (i.e. top-level name conflict).

However, your unpublished implementation guidance edit explicitly disallows any conflicts for openconfig, which resolves this issue as well.

I just checked, and it looks like an error is indeed detected for both compressed and non-compressed schemas when this situation happens, unlike what I suggested above. There is issue in the error reporting though, so fixing that in #490.

[robshakir]

Sorry, to clarify, goyang supports this at the root only, but ygen cannot support this in compressed or uncompressed schemas, because creating the root of the schema tree always just uses the (*yang.Entry).Name that we receive. The handling is here. findRootEntities finds anything that would be at / in the schema tree, and since the module name isn't included in the schema path, then we do not end up with the module name that ygen gives us.

Thanks for fixing the error reporting issue.