gnmi-1.19 fixes#5612
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces stability and reliability improvements to the controller card switchover test suite. By refining how gNMI data is retrieved and ensuring that critical system components are ready and properly configured before operations occur, the changes reduce flakiness in the testing environment. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Pull Request Functional Test Report for #5612 / ee9377fVirtual Devices
Hardware Devices
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the controller card switchover test by adding standby controller card readiness checks, configuring routing policies and basic interfaces, and refactoring the verification of configured elements using raw gNMI calls. Feedback focuses on improving the robustness of the raw gNMI JSON parsing by querying the parent /interfaces container instead of the list directly, and using GetOrCreateStatement instead of AppendNewStatement to avoid ignoring errors when defining routing policy statements.
| // 2. Construct the exact path matching: /interfaces/interface | ||
| getRequest := &gpb.GetRequest{ | ||
| Type: gpb.GetRequest_CONFIG, | ||
| Encoding: gpb.Encoding_JSON_IETF, | ||
| Path: []*gpb.Path{ | ||
| { | ||
| Elem: []*gpb.PathElem{ | ||
| {Name: "interfaces"}, | ||
| {Name: "interface"}, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| ctxTimeout, cancel := context.WithTimeout(context.Background(), 20*time.Second) | ||
| defer cancel() | ||
|
|
||
| // 3. Execute the raw gNMI call | ||
| getResponse, err := gnmiClient.Get(ctxTimeout, getRequest) | ||
| if err != nil { | ||
| t.Fatalf("Raw gNMI Get failed: %v", err) | ||
| } | ||
|
|
||
| numInterfaces := 0 | ||
| // 4. Parse the raw JSON using a generic map wrapper | ||
| if len(getResponse.GetNotification()) > 0 && len(getResponse.GetNotification()[0].GetUpdate()) > 0 { | ||
| jsonVal := getResponse.GetNotification()[0].GetUpdate()[0].GetVal().GetJsonIetfVal() | ||
|
|
||
| // Parse into a dynamic list of maps representing the interfaces list array | ||
| var rawData []map[string]interface{} | ||
| if err := json.Unmarshal(jsonVal, &rawData); err != nil { | ||
| t.Fatalf("Failed to parse raw JSON payload: %v", err) | ||
| } | ||
|
|
||
| // The length of the array is exactly how many interface objects are inside the JSON | ||
| numInterfaces = len(rawData) | ||
| } else { | ||
| t.Fatalf("No interface configuration elements returned from device state database.") | ||
| } |
There was a problem hiding this comment.
Querying the list path /interfaces/interface directly via raw gNMI Get and unmarshalling it into a slice of maps ([]map[string]interface{}) is fragile. Depending on the gNMI server implementation, a list query might return multiple updates (one per list key) or a wrapped JSON object instead of a direct JSON array, which will cause json.Unmarshal to fail and crash the test.
To make this robust and vendor-independent, query the parent container /interfaces (which is guaranteed to be returned as a single JSON object) and dynamically extract the interface list, handling potential namespace prefixes.
// 2. Construct the exact path matching: /interfaces
getRequest := &gpb.GetRequest{
Type: gpb.GetRequest_CONFIG,
Encoding: gpb.Encoding_JSON_IETF,
Path: []*gpb.Path{
{
Elem: []*gpb.PathElem{
{Name: "interfaces"},
},
},
},
}
ctxTimeout, cancel := context.WithTimeout(context.Background(), 20*time.Second)
defer cancel()
// 3. Execute the raw gNMI call
getResponse, err := gnmiClient.Get(ctxTimeout, getRequest)
if err != nil {
t.Fatalf("Raw gNMI Get failed: %v", err)
}
numInterfaces := 0
// 4. Parse the raw JSON using a generic map wrapper
if len(getResponse.GetNotification()) > 0 && len(getResponse.GetNotification()[0].GetUpdate()) > 0 {
jsonVal := getResponse.GetNotification()[0].GetUpdate()[0].GetVal().GetJsonIetfVal()
// Parse into a dynamic map representing the interfaces container
var rawData map[string]interface{}
if err := json.Unmarshal(jsonVal, &rawData); err != nil {
t.Fatalf("Failed to parse raw JSON payload: %v", err)
}
// Extract the interface list, handling potential namespace prefixes
interfacesVal, ok := rawData["interface"]
if !ok {
interfacesVal, ok = rawData["openconfig-interfaces:interface"]
}
if ok {
if interfacesList, ok := interfacesVal.([]interface{}); ok {
numInterfaces = len(interfacesList)
} else {
t.Fatalf("Interface list in JSON is not an array")
}
} else {
t.Fatalf("No interface list found in JSON payload")
}
} else {
t.Fatalf("No interface configuration elements returned from device state database.")
}| d := &oc.Root{} | ||
| rp := d.GetOrCreateRoutingPolicy() | ||
| pdef := rp.GetOrCreatePolicyDefinition(acceptRoutePolicy) | ||
| stmt, _ := pdef.AppendNewStatement("20") |
There was a problem hiding this comment.
Using AppendNewStatement with a hardcoded key and ignoring the returned error with _ is discouraged. Since the statement key is explicitly known ("20"), it is cleaner and more idiomatic to use GetOrCreateStatement which does not return an error.
stmt := pdef.GetOrCreateStatement("20")2 participants
Made these changes :
1.Removed wild card issue in gnmi get
2.Added a check for standby CC ready before switchover
3.Created interface and routing policy configs before attemping the get and set