Need Clarification on section 3.3.2 The GetResponse message

[dlakshma]

Hi All,

The GetResponse message consists of:

notification - a set of Notification messages, as defined in Section 2.1. The target MUST generate a Notification message for each path specified in the client's GetRequest, and hence MUST NOT collapse data from multiple paths into a single Notification within the response. The timestamp field of the Notification message MUST be set to the time at which the target's snapshot of the relevant path was taken.

On GetRequest, if the path contains wild card entry for example path='/bgp/neighbors/neighbor [neighbor-address = *]', then how the GetResponse should be formed? Should it contain related Notifications for each of Neighbor entry under Neighbors?.

Thanks.

[hellt]

Hi @wenovus @robshakir I would like to make our gNMI implementation to be as compliant as possible and this question was recently raised by one of our customers as well.

I would like to add another related question to OP's question, so that it becomes a list of two questions:

1. if GetRequest's Path message contains a wildcard, such as /bgp/neighbors/neighbor[neighbor-address = *], should gNMI target expand this path and treat it as multiple paths and hence return Notification per expanded path?
2. if not, what should GetResponse contain in its Path message? The original path with a wildcard or no path at all?

[wenovus]

I think this is a good question, so the three candidates I see are

1. server concatenates updates in a JSON list. Concatenation is necessary because the query path could end beyond the wildcarded node, or there could be multiple wildcards in the path.

   notification {
   update: {
   path: "/bgp/neighbors/neighbor[neighbor-address = *]"
   val: "[{neighbor1}, {neighbor2}]"
   }
   }

   This seems problematic as far as client unmarshalling is concerned. The reason is the query path could be /bgp/neighbors/neighbor[neighbor-address = *]/config/peer-as, or some other leaf, and this means it is impossible to tell which key is associated with each value without the val: field itself being coerced into a custom JSON object type with the key fields somehow being present, and which we will need some further spec clarification on how this object is to be constructed.

2. One update per expanded path.

   notification {
   update: {
   path: "/bgp/neighbors/neighbor[neighbor-address = 1]"
   val: "[{neighbor1}]"
   }
   update: {
   path: "/bgp/neighbors/neighbor[neighbor-address = 2]"
   val: "[{neighbor2}]"
   }
   }

   This seems reasonable to me since it fits the spec's description, while allowing the user to associate each expanded path with the key value with each type. Although you would not see the wildcard path and be able to match that with the query in the GetRequest, the GetRequest's doc comment does say the following which makes this a non-problem:

   // GetResponse is used by the target to respond to a GetRequest from a client.
   // The set of Notifications corresponds to the data values that are requested
   // by the client in the GetRequest.

   (ref)

3. One notification per expanded path.

   notification {
   update: {
   path: "/bgp/neighbors/neighbor[neighbor-address = 1]"
   val: "[{neighbor1}]"
   }
   }
   notification {
   update: {
   path: "/bgp/neighbors/neighbor[neighbor-address = 2]"
   val: "[{neighbor2}]"
   }
   }

   This option seems to violate the doc comment mentioned above, so I don't think this is what the spec intended.

So I believe option 2 is very likely what the spec intended, although I would like to get an authoritative answer from @robshakir.

[robshakir]

Yes, I agree with @wenovus' analysis here -- although there seems to one other option

Primarily, one clarification of option 2. If we have an update that has /bgp/neighbors/neighbor[neighbor-address=1] as the path, then the value should not be a JSON array, but rather a JSON object - i.e., we should get:

{
   "neighbor-address": "1",
   "config": {
        "description": "neighbor 1: peer-42"
   }
}

since this is just a single element of the list. The [s around the value in Wen's example seem to show otherwise :-)

One discussion point is that in the specification today, we allow for a key to be omitted -- e.g., /bgp/neighbors/neighbor/state/foo is listed as an allowed path. This seems to hold with the above approach whereby we'd have separate update messages within the same notification for each specified neighbour, but gives us another discussion point of how /bgp/neighbors/neighbor itself should be treated -- I think it should be equivalent to /bgp/neighbors/neighbor[name=*] -- but the question then arises, do we ever return the JSON array, or is this a series of JSON objects? I'd be supportive of the latter (I don't see any reason to return the array, if one really wanted this, you could just query /interfaces.

We should likely make specification clarifications here.

[0]: note, ygot implements this in the way specified above, here's a test program.

package main

import (
        "fmt"

        log "github.com/golang/glog"

        gpb "github.com/openconfig/gnmi/proto/gnmi"

        oc "github.com/openconfig/ygot/exampleoc"
        "github.com/openconfig/ygot/ygot"
        "github.com/openconfig/ygot/ytypes"
)

func main() {

        d1 := &oc.Device{}
        d1.GetOrCreateInterface("eth0")
        d1.GetOrCreateInterface("eth1")

        s, err := oc.Schema()
        if err != nil {
                log.Exitf("couldn't get schema, %v", err)
        }

        mustPath := func(s string) *gpb.Path {
                p, err := ygot.StringToStructuredPath(s)
                if err != nil {
                        log.Exitf("can't convert to path, %v", err)
                }
                return p
        }

        for _, p := range []string{
                "/interfaces/interface",
                "/interfaces/interface[name=*]",
                "/interfaces/interface/state/oper-status",
        } {
                sp := mustPath(p)
                got, err := ytypes.GetNode(s.RootSchema(), d1, sp, &ytypes.GetHandleWildcards{}, &ytypes.GetPartialKeyMatch{})
                if err != nil {
                        log.Exitf("couldn't get path, %v", err)
                }
                fmt.Println(sp)
                for _, r := range got {
                        ps, _ := ygot.PathToString(r.Path)
                        fmt.Printf("%s->%T\n", ps, r.Data)
                }
                fmt.Printf("---\n\n")
        }
}

[wenovus]

Thanks Rob for the clarification, I made a typo there, I agree that a JSON object should be returned whenever the path is a non-wildcard path.

Per the gNMI path conventions:

To match all entries within a particular list, the key(s) to the list may be omitted, for example to match the oper-status value of all interfaces on a device:

So yes /bgp/neighbors/neighbor is just a shorthand for /bgp/neighbors/neighbor[name=*].

And therefore the behaviour between them should not differ.

[hellt]

thanks @robshakir @wenovus I have one comment around the GetResponse description

// GetResponse is used by the target to respond to a GetRequest from a client.
// The set of Notifications corresponds to the data values that are requested
// by the client in the GetRequest.

here it says The set of Notifications corresponds to the data values that are requested by the client in the GetRequest which (if I read it correctly) implies that we have multiple notifications and not a single one with multiple updates.

Should it then be single notification with a list of updates:

notification {
  update: {
    path: "/bgp/neighbors/neighbor[neighbor-address = 1]"
    val: "{neighbor1}"
  }
  update: {
    path: "/bgp/neighbors/neighbor[neighbor-address = 2]"
    val: "{neighbor2}"
  }
}

or a list of notifications as the description suggests:

notification {
  update: {
    path: "/bgp/neighbors/neighbor[neighbor-address = 1]"
    val: "{neighbor1}"
  }
}
notification {
  update: {
    path: "/bgp/neighbors/neighbor[neighbor-address = 2]"
    val: "{neighbor2}"
  }
}

[robshakir]

Here's where the wider spec is needed (clearly, it's not practical to have the whole spec as the comments in the proto). 3.3.2 in the gNMI spec says:

The target MUST generate a Notification message for each path specified in the client's GetRequest, and hence MUST NOT collapse data from multiple paths into a single Notification within the response.

which means the former rather than the latter, since in this case there is one path being requested from the client and hence one Notification.

The wording in the proto is also correct though, since you may have >1 path being requested in the GetRequest.