Add support for empty request / no prefix #154
Conversation
|
Hey @rao-aneesh , what issue does it address? I understand your change, but I wonder what problem did you encounter with the existing behaviour? |
|
Hey @akarneliuk, the issue was with master arbitration. The specification states: Hence, sending an empty request with just an extension is necessary. Sending a request with an empty prefix with no paths leads to an error response. |
| if prefix is None and target is None: | ||
| gnmi_message_request = SetRequest( | ||
| delete=del_protobuf_paths, | ||
| update=update_msg, | ||
| replace=replace_msg, | ||
| ) | ||
| else: | ||
| gnmi_message_request = SetRequest( | ||
| prefix=protobuf_prefix, | ||
| delete=del_protobuf_paths, | ||
| update=update_msg, | ||
| replace=replace_msg, | ||
| ) |
There was a problem hiding this comment.
isnt the masterelection is part of only gnmi extension only and not other way around and sending an empty request with gnmi_extension is suppose to happen only.
section3.2.
There was a problem hiding this comment.
It will help to have the flexibility to send an empty SET request. Better than not having it.
Also, as stated in Section 3.4,
A SetRequest specifying an empty set of paths MUST NOT be treated as an error by the target.
Previously, the smallest request that could be generated was:
prefix {
}
Currently, on Cisco Routers, sending a completely empty request does not result in an error. However, having a prefix with no paths does result in a error.
| if prefix is None and target is None: | ||
| gnmi_message_request = SetRequest( | ||
| delete=del_protobuf_paths, | ||
| update=update_msg, | ||
| replace=replace_msg, | ||
| extension=[gnmi_extension], | ||
| ) | ||
| else: | ||
| gnmi_message_request = SetRequest( | ||
| prefix=protobuf_prefix, | ||
| delete=del_protobuf_paths, | ||
| update=update_msg, | ||
| replace=replace_msg, | ||
| extension=[gnmi_extension], | ||
| ) |
There was a problem hiding this comment.
| if prefix is None and target is None: | |
| gnmi_message_request = SetRequest( | |
| delete=del_protobuf_paths, | |
| update=update_msg, | |
| replace=replace_msg, | |
| extension=[gnmi_extension], | |
| ) | |
| else: | |
| gnmi_message_request = SetRequest( | |
| prefix=protobuf_prefix, | |
| delete=del_protobuf_paths, | |
| update=update_msg, | |
| replace=replace_msg, | |
| extension=[gnmi_extension], | |
| ) | |
| if not prefix and target : | |
| gnmi_message_request = SetRequest( | |
| prefix=protobuf_prefix, | |
| delete=del_protobuf_paths, | |
| update=update_msg, | |
| replace=replace_msg, | |
| extension=[gnmi_extension], | |
| ) | |
| else: | |
| gnmi_message_request = SetRequest( | |
| delete=del_protobuf_paths, | |
| update=update_msg, | |
| replace=replace_msg, | |
| extension=[gnmi_extension], | |
| ) |
There was a problem hiding this comment.
Wouldnt this be more pythonic ?
There was a problem hiding this comment.
I am assuming you mean not (prefix and target). That is logically incorrect. The correct condition would be (a or b). But this wouldn't work because we need to specifically check if the value is not None, since an empty string evaluates to False as well.
| if prefix is None and target is None: | ||
| gnmi_message_request = GetRequest( | ||
| path=protobuf_paths, | ||
| type=pb_datatype, | ||
| encoding=pb_encoding, | ||
| ) | ||
| else: | ||
| gnmi_message_request = GetRequest( | ||
| prefix=protobuf_prefix, | ||
| path=protobuf_paths, | ||
| type=pb_datatype, | ||
| encoding=pb_encoding, | ||
| ) |
There was a problem hiding this comment.
the protobuf being empty should be handled by target. What is the error here we are getting ?
It will be great if you help us to point in gNMI spec where it indicates this behavior.
There was a problem hiding this comment.
Due to a recent change on Cisco Routers, an empty prefix block defaults to origin openconfig. If we try to perform a GET with a native model, we get the error
gNMI: invalid YangGetGnmi: rpc error: code = Internal desc = prefix and path origins do not match
Hence, we have to send an origin in the prefix as well as the path. This used to be the case for requests that didn't have prefixes either. We would have to forcibly add prefixes to not get the error. However, with a recent change, if a prefix block is not present, this error isn't shown. To be able to make use of this, this control is needed. #145
| type=pb_datatype, | ||
| encoding=pb_encoding, | ||
| ) | ||
| if prefix is None and target is None: |
There was a problem hiding this comment.
why check for target here ? previous checks are not taking that into account.
Will this introduce any change in the behavior, if yes please document what it would be and check if it is acceptable.
There was a problem hiding this comment.
Previously, the prefix block was present in every request. Now, this change is to allow prefix block to be optional. The 'target' field is present only in the prefix block. Hence, we check for target field here.
| replace=replace_msg, | ||
| extension=[gnmi_extension], | ||
| ) | ||
| if prefix is None and target is None: |
There was a problem hiding this comment.
Something in the lines of below will be a create once and use everywhere mode.
# Define the base arguments for the SetRequest
gnmi_message_request_args = {
'delete': del_protobuf_paths,
'update': update_msg,
'replace': replace_msg,
'extension': [gnmi_extension]
}
# Add the prefix only if it's necessary
if gnmi_extension and (prefix is not None or target is not None):
gnmi_message_request_args['prefix'] = protobuf_prefix
# Create the SetRequest with the specified arguments
gnmi_message_request = SetRequest(**gnmi_message_request_args)There was a problem hiding this comment.
I agree, I was only staying true to the current coding convention in the codebase.
|
@akarneliuk, can you review and merge this please? |
There was previously no way to specify no prefix block before. This adds support for that.
The default value of prefix has been changed to None. To specify an empty prefix, "" must be passed to the function.