[NETCONF-820] Fail to process RESTCONF fields on the mounted device Created: 15/Sep/21  Updated: 06/Nov/22  Resolved: 06/Nov/22

Status: Resolved
Project: netconf
Component/s: netconf
Affects Version/s: None
Fix Version/s: 4.0.3

Type: Bug Priority: Medium
Reporter: Sangwook Ha Assignee: Peter Puškár
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Duplicate
is duplicated by NETCONF-849 Fields filtering fails for leafs in l... Resolved
Relates
relates to NETCONF-660 RFC 8040 query fails to return field ... Resolved

 Description   

RESCONF get call to retrieve a subset of tree on a mounted device using the fields parameter fails when it includes a list path.

For example, the following error was thrown when

GET /rests/data/network-topology:network-topology/topology=topology-netconf/node=SJ-E9-221/yang-ext:mount/ietf-interfaces:interfaces?fields=interface/type

where interface is a list.

04:54:24.938 ERROR [opendaylight-cluster-data-akka.actor.default-dispatcher-13] class org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier$NodeIdentifier cannot be cast to class org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier$NodeIdentifierWithPredicates (org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier$NodeIdentifier and org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier$NodeIdentifierWithPredicates are in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @43ad1bfd)
java.lang.ClassCastException: class org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier$NodeIdentifier cannot be cast to class org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier$NodeIdentifierWithPredicates (org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier$NodeIdentifier and org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier$NodeIdentifierWithPredicates are in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @43ad1bfd)
	at org.opendaylight.netconf.util.StreamingContext$AbstractMapMixin.emitChildTreeNode(StreamingContext.java:268) ~[bundleFile:?]
	at org.opendaylight.netconf.util.StreamingContext$AbstractComposite.streamToWriter(StreamingContext.java:181) ~[bundleFile:?]
	at org.opendaylight.netconf.util.StreamingContext$AbstractComposite.emitChildTreeNode(StreamingContext.java:188) ~[bundleFile:?]
	at org.opendaylight.netconf.util.StreamingContext$AbstractComposite.streamToWriter(StreamingContext.java:181) ~[bundleFile:?]
	at org.opendaylight.netconf.util.NetconfUtil.writeFilter(NetconfUtil.java:254) ~[bundleFile:?]
	at org.opendaylight.netconf.sal.connect.netconf.util.NetconfMessageTransformUtil.toFilterStructure(NetconfMessageTransformUtil.java:261) ~[bundleFile:?]
	at org.opendaylight.netconf.sal.connect.netconf.util.NetconfRpcStructureTransformer.toFilterStructure(NetconfRpcStructureTransformer.java:77) ~[bundleFile:?]
	at org.opendaylight.netconf.sal.connect.netconf.util.NetconfBaseOps.get(NetconfBaseOps.java:351) ~[bundleFile:?]
	at org.opendaylight.netconf.sal.connect.netconf.util.NetconfBaseOps.getData(NetconfBaseOps.java:302) ~[bundleFile:?]
	at org.opendaylight.netconf.sal.connect.netconf.sal.AbstractNetconfDataTreeService.get(AbstractNetconfDataTreeService.java:265) ~[bundleFile:?]
	at org.opendaylight.netconf.topology.singleton.impl.actors.NetconfDataTreeServiceActor.onReceive(NetconfDataTreeServiceActor.java:70) ~[?:?]
	at akka.actor.UntypedAbstractActor$$anonfun$receive$1.applyOrElse(AbstractActor.scala:332) ~[bundleFile:?]
	at akka.actor.Actor.aroundReceive(Actor.scala:537) ~[bundleFile:?]
	at akka.actor.Actor.aroundReceive$(Actor.scala:535) ~[bundleFile:?]
	at akka.actor.AbstractActor.aroundReceive(AbstractActor.scala:220) ~[bundleFile:?]
	at akka.actor.ActorCell.receiveMessage(ActorCell.scala:577) [bundleFile:?]
	at akka.actor.ActorCell.invoke(ActorCell.scala:547) [bundleFile:?]
	at akka.dispatch.Mailbox.processMailbox(Mailbox.scala:270) [bundleFile:?]
	at akka.dispatch.Mailbox.run(Mailbox.scala:231) [bundleFile:?]
	at akka.dispatch.Mailbox.exec(Mailbox.scala:243) [bundleFile:?]
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290) [?:?]
	at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020) [?:?]
	at java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656) [?:?]
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594) [?:?]
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183) [?:?]


 Comments   
Comment by Robert Varga [ 22/Oct/21 ]

I suspect the problem does not lie with netconf-util's handling of filters, but rather with restconf-nb-rfc8040's parser which creates the structure. This is a nightmare to follow, but bear with me.

The first bit is here: https://github.com/opendaylight/netconf/blob/master/restconf/restconf-nb-rfc8040/src/main/java/org/opendaylight/restconf/nb/rfc8040/rests/utils/ReadDataTransactionUtil.java#L166-L170:

            if (identifier.getMountPoint() != null) {
                builder.setFieldPaths(parseFieldsPaths(identifier, fields.get(0)));
            } else {
                builder.setFields(parseFieldsParameter(identifier, fields.get(0)));
            }

So we are doing different things for mount points (parseFieldsPaths()) and local datastore (parseFieldsParameter()). Let's focus on the former, as the latter is absolutely unimplemented. This (eventually) leads us to ParserFieldsParameter which just an atrocious piece of code.

Let's improve the diagnostics first and show whether the requested child actually matches the QName we expect here, or whether it is a mixup with a child leaf. At any rate I am putting a rewrite of that class on by TODO list.

Comment by Sangwook Ha [ 22/Oct/21 ]

Makes sense. I also noticed a difference in the behavior between mount point & internal datastore: e.g. the latter does not include the key leaves in the response if they are not included in the fields query.

Comment by Robert Varga [ 22/Oct/21 ]

Yeah, it is a pathological case of wrong layering and resulting bowl of pasta.

Anyway, I have merged the diagnostic patch to both master and 1.13.x, can you give it a try, please?

In the meantime I have started on splitting out string parsing from that monstrosity here: https://git.opendaylight.org/gerrit/c/netconf/+/98051

Comment by Sangwook Ha [ 22/Oct/21 ]

I tried the patch and the error message is clearer now:

20:11:46.492 ERROR [opendaylight-cluster-data-akka.actor.default-dispatcher-16] Unable to serialize filter element for path /(urn:ietf:params:xml:ns:yang:ietf-interfaces?revision=2014-05-08)interfaces with fields: [/(urn:ietf:params:xml:ns:yang:ietf-interfaces?revision=2014-05-08)interface[{}]/type]
java.lang.IllegalStateException: Unable to serialize filter element for path /(urn:ietf:params:xml:ns:yang:ietf-interfaces?revision=2014-05-08)interfaces with fields: [/(urn:ietf:params:xml:ns:yang:ietf-interfaces?revision=2014-05-08)interface[{}]/type]
	at org.opendaylight.netconf.sal.connect.netconf.util.NetconfMessageTransformUtil.toFilterStructure(NetconfMessageTransformUtil.java:263) ~[bundleFile:?]
	at org.opendaylight.netconf.sal.connect.netconf.util.NetconfRpcStructureTransformer.toFilterStructure(NetconfRpcStructureTransformer.java:77) ~[bundleFile:?]
	at org.opendaylight.netconf.sal.connect.netconf.util.NetconfBaseOps.get(NetconfBaseOps.java:351) ~[bundleFile:?]
	at org.opendaylight.netconf.sal.connect.netconf.util.NetconfBaseOps.getData(NetconfBaseOps.java:302) ~[bundleFile:?]
	at org.opendaylight.netconf.sal.connect.netconf.sal.AbstractNetconfDataTreeService.get(AbstractNetconfDataTreeService.java:265) ~[bundleFile:?]
	at org.opendaylight.netconf.topology.singleton.impl.actors.NetconfDataTreeServiceActor.onReceive(NetconfDataTreeServiceActor.java:70) ~[?:?]
	at akka.actor.UntypedAbstractActor$$anonfun$receive$1.applyOrElse(AbstractActor.scala:332) ~[bundleFile:?]
	at akka.actor.Actor.aroundReceive(Actor.scala:537) ~[bundleFile:?]
	at akka.actor.Actor.aroundReceive$(Actor.scala:535) ~[bundleFile:?]
	at akka.actor.AbstractActor.aroundReceive(AbstractActor.scala:220) ~[bundleFile:?]
	at akka.actor.ActorCell.receiveMessage(ActorCell.scala:580) [bundleFile:?]
	at akka.actor.ActorCell.invoke(ActorCell.scala:548) [bundleFile:?]
	at akka.dispatch.Mailbox.processMailbox(Mailbox.scala:270) [bundleFile:?]
	at akka.dispatch.Mailbox.run(Mailbox.scala:231) [bundleFile:?]
	at akka.dispatch.Mailbox.exec(Mailbox.scala:243) [bundleFile:?]
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290) [?:?]
	at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020) [?:?]
	at java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656) [?:?]
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594) [?:?]
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183) [?:?]
Caused by: java.io.IOException: Child identifier (urn:ietf:params:xml:ns:yang:ietf-interfaces?revision=2014-05-08)type is invalid in parent (urn:ietf:params:xml:ns:yang:ietf-interfaces?revision=2014-05-08)interface
	at org.opendaylight.netconf.util.StreamingContext$AbstractMapMixin.emitChildTreeNode(StreamingContext.java:270) ~[bundleFile:?]
	at org.opendaylight.netconf.util.StreamingContext$AbstractComposite.streamToWriter(StreamingContext.java:181) ~[bundleFile:?]
	at org.opendaylight.netconf.util.StreamingContext$AbstractComposite.emitChildTreeNode(StreamingContext.java:188) ~[bundleFile:?]
	at org.opendaylight.netconf.util.StreamingContext$AbstractComposite.streamToWriter(StreamingContext.java:181) ~[bundleFile:?]
	at org.opendaylight.netconf.util.NetconfUtil.writeFilter(NetconfUtil.java:254) ~[bundleFile:?]
	at org.opendaylight.netconf.sal.connect.netconf.util.NetconfMessageTransformUtil.toFilterStructure(NetconfMessageTransformUtil.java:261) ~[bundleFile:?]
	... 19 more
Comment by Robert Varga [ 22/Oct/21 ]

Right-o. So it is PathParser's fault after all – it should be inserting an NodeIdentifierWithPredicates before accessing type.

The problem is the usual confusion about addressing: we have a number of constructs which boil down to List<QName> in some shape or form, but they have distinct meanings:

  • SchemaPath has aweful definition, which makes it useless due to ambiguity
  • SchemaNodeIdentifier is QNames as seen by the likes of "augment /foo/bar" YANG statements
  • YangInstanceIdentifier corresponds to NormalizedNode layout

It is never a good sign when common code is used to build a SchemaPath/SchemaNodeIdentifier and YangInstanceIdentifier. The two differ when lists are concerned, because NormalizedNodes require MapNode->MapEntryNode (i.e. corresponding to JSON structure, not XML).

To add insult to injury, netconf-util is misusing YangInstanceIdentifier, it really should have its own class to represent what it wants. We will deal with that separately though.

Comment by Robert Varga [ 25/Oct/21 ]

So I created a FieldsParam and the associated parser to split the problem ParserFieldsParameter is dealing with into two problems:

  • FieldsParam.parse() parses the URI string into a DTO with a simple structure (done)
  • ParserFiledsParameter will then be used to bind FiledsParam to an EffectiveModelContext, creating whatever is needed (todo)

Now we are in a position where we can integrate the two, which will result in taking some of the complexity out of the picture, making it easier to refactor further.

Comment by Sangwook Ha [ 25/Oct/21 ]

Great! Thanks for the update.

Comment by Robert Varga [ 25/Oct/21 ]

Okay, so the integration is done and dusted. Now I have split up the two parsers and their test suites in https://git.opendaylight.org/gerrit/c/netconf/+/98129 . Since both should handle the same inputs, we now have an AbstractFieldsTranslatorTest, which drives the test cases and two subclasses which are evaluating assertions.

During that I realized the test suite is rather strange: it loads up an EffectiveModelContext, but also does a ton of mocking around SchemaNodes – to the point where some of the test cases do not make quite sense, I am afraid. So the next step here is clean up that part, which will probably start showing some deficiencies in the code base.

Once that is done, reproducing the failure in a nice test case should be possible and that should drive us forward.

Comment by Robert Varga [ 26/Oct/21 ]

After looking at the error more closely, it looks a bit strange.

The request is for /interfaces list, which is fine. The fields specification looks also fine: interface[{}] is the empty NodeIdentifierWithPredicates.

Without looking at the code, the state should be such that we are entering filter serialization with DataSchemaContextNode for 'interfaces' as the current node, then enter the empty NodeIdentifierWithPredicates, then enter type.

It looks, though, as if we are starting at root (or, more generally, at the parent of 'interfaces'), then use the NodeIdentifierWithPredicates to enter the node for 'interfaces' and then reject the 'type' child. If this is really happening, we should really reject the first step, as it is the first mismatch.

In any case, this is a disagreement between NETCONF and RESTCONF, so it's still not clear who is at fault here:

  • NETCONF may have good reasons to not enter the 'interfaces' node there – that encapsulation does not exist in XML mapping
  • RESTCONF does have enough state to start with 'interfaces' NodeIdentifier
  • Judging by the error message alone, a YangInstanceIdentifier concatenation of 'for path' and 'with fields' really yields what RESTCONF is looking for
  • The interface seems to be defined very weakly, so there's some room for improvement there

So after spending some quality time in RESTCONF code, it is now time to sift through the NETCONF side of things again

 

Comment by Sangwook Ha [ 26/Oct/21 ]

For the list node the NETCONF side is expecting both NodeIdentifier & NodeIdentifierWithPredicates - for example, YangInstanceIdentifier for a field datastores/datastore is created like this in the unit test:

final YangInstanceIdentifier datastoreListField = YangInstanceIdentifier.create(toId(Datastores.QNAME),
        toId(Datastore.QNAME), NodeIdentifierWithPredicates.of(Datastore.QNAME));

There are NodeIdentifier & NodeIdentifierWithPredicates for the list node datastore.

However, RESTCONF just generates a YangInstanceIdentifier with NodeIdentifierWithPredicates only.

I was trying to make it work with a NETCONF side fix but wasn't really sure which side is at fault.

Comment by Robert Varga [ 26/Oct/21 ]

So the stitching on lists is rather not obvious and is asserted by tests here: https://github.com/opendaylight/netconf/blob/master/netconf/sal-netconf-connector/src/test/java/org/opendaylight/netconf/sal/connect/netconf/schema/mapping/NetconfMessageTransformerTest.java#L1061-L1097

What this does is actually ignore the leading NodeIdentifierWithPredicates, because path is:

/(urn:ietf:params:xml:ns:yang:ietf-netconf-monitoring?revision=2010-10-04)netconf-state/sessions

i.e. it points to sessions and then field is accessed as:

/(urn:ietf:params:xml:ns:yang:ietf-netconf-monitoring?revision=2010-10-04)session/session[{(urn:ietf:params:xml:ns:yang:ietf-netconf-monitoring?revision=2010-10-04)session-id=1}]

Note how there is no empty NodeIdentifierWithPredicates on either side. So if this really is how this piece of fine engineering is meant to operate, RESTCONF should not be emitting the leading empty NodeIdentifierWithPredicates. A bit more investigation is necessary to understand whether this is specific to the stitch point, or whether there are other weird things happening.

Comment by Robert Varga [ 26/Oct/21 ]

Yes, this is how I would expect it to operate, but this test case is different: we are crossing into the list and then through its entries as part of the fields filter, i.e. the path/field stitching is at ContainerNode->MapNode and then MapNode->MapEntryNode->LeafNode is part of the field YangInstanceIdentifier.

I think the mapping code is performing a magic jump in addressing when the final node of 'path', i.e. the stitch point, is a MapNode, then the addressing is not MapNode->MapEntryNode->LeafNode, but rather MapNode->LeafNode – with the stitch point being the only place where this happens.

Comment by Robert Varga [ 26/Oct/21 ]

The thing is, though, we are not talking directly to the code being tested here, but go through this guy on netconf side:

        if (isFilterPresent(filterPath)) {
            rpcInput = NetconfMessageTransformUtil.wrap(NETCONF_GET_NODEID, transformer.toFilterStructure(
                    Collections.singletonList(FieldsFilter.of(filterPath.get(), fields))));

Here the inputs are controlled by RESTCONF. I think it is very reasonable for RESTCONF to supply the leading empty NodeIdentifierWith predicates, because that makes it consistent from the perspective of YangInstanceIdentifier addressing – concat the filterPath and any of the fields and you get a valid YangInstanceIdentifier, containing PathArguments for Container->MapNode->MapEntryNode->LeafNode steps.

The problem is that FieldsFilter does not work that way at least for the case of MapNodes, as demonstrated by the UT I pointed out. There the expectation is that filterPath contains PathArguments for Container->MapNode and field contains just the LeafNode PathArgument.

Comment by Robert Varga [ 26/Oct/21 ]

I am going offline, but a simple test to try out would be to modify this piece of code to check each of the fields and if it starts with an empty NodeIdentifierWith predicates, trim it before adding it to FieldsFilter. I strongly suspect this will fix the issue.

Comment by Robert Varga [ 26/Oct/21 ]

Ah, and it that works, we have a hotfix

In any case, the long-term fix is to ditch YangInstanceIdentifier in FieldsParam and define a new structure, which will hold just a List<QName>, similar to what SchemaNodeIdentifier does. We cannot use SchemaNodeIdentifier, as the QNames there are interpreted as steps along YANG schema tree axis. This new structure would interpret those QName along YANG data tree axis – which how NETCONF/RESTCONF addressing works natively – and hence we will be able to delete a chunk of complex code!

Comment by Sangwook Ha [ 26/Oct/21 ]

Tried another case in the lab:

  • /rests/data/network-topology:network-topology/topology=topology-netconf/node=SJ-E9-221/yang-ext:mount/ietf-interfaces:interfaces?fields=interface/type
  • /rests/data/network-topology:network-topology/topology=topology-netconf/node=SJ-E9-221/yang-ext:mount/ietf-interfaces:interfaces/interface=loopback10/host-management-std:loopback?fields=ipv6/address/ip

The first case is the original failure case where interface is a list node.

In the second case host-management-std:loopback and ipv6 are both containers, and address is a list node. And this one also fails:

nable to serialize filter element for path /(urn:ietf:params:xml:ns:yang:ietf-interfaces?revision=2014-05-08)interfaces/interface/interface[{(urn:ietf:params:xml:ns:yang:ietf-interfaces?revision=2014-05-08)name=loopback10}]/AugmentationIdentifier{childNames=[(http://www.calix.com/ns/exa/host-management-std?revision=2020-10-19)loopback]}/(http://www.calix.com/ns/exa/host-management-std?revision=2020-10-19)loopback with fields: [/(http://www.calix.com/ns/exa/host-management-std?revision=2020-10-19)ipv6/address[{}]/ip]
java.lang.IllegalStateException: Unable to serialize filter element for path /(urn:ietf:params:xml:ns:yang:ietf-interfaces?revision=2014-05-08)interfaces/interface/interface[{(urn:ietf:params:xml:ns:yang:ietf-interfaces?revision=2014-05-08)name=loopback10}]/AugmentationIdentifier{childNames=[(http://www.calix.com/ns/exa/host-management-std?revision=2020-10-19)loopback]}/(http://www.calix.com/ns/exa/host-management-std?revision=2020-10-19)loopback with fields: [/(http://www.calix.com/ns/exa/host-management-std?revision=2020-10-19)ipv6/address[{}]/ip]

But the following works without a child leaf node:

  • /rests/data/network-topology:network-topology/topology=topology-netconf/node=SJ-E9-221/yang-ext:mount/ietf-interfaces:interfaces?fields=interface
  • /rests/data/network-topology:network-topology/topology=topology-netconf/node=SJ-E9-221/yang-ext:mount/ietf-interfaces:interfaces/interface=loopback10/host-management-std:loopback?fields=ipv6/address

So fields with list node + child node seem to be the issue.

Generated at Wed Feb 07 20:15:59 UTC 2024 using Jira 8.20.10#820010-sha1:ace47f9899e9ee25d7157d59aa17ab06aee30d3d.