Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for empty request / no prefix #154

Merged
merged 3 commits into from
Apr 28, 2024
Merged

Add support for empty request / no prefix #154

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14911dc
Update client.py
rao-aneesh Mar 26, 2024
3376f0b
Update client.py
rao-aneesh Mar 26, 2024
c183e7a
Update client.py
rao-aneesh Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 43 additions & 21 deletions pygnmi/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ def convert_encoding(self, requested_encoding: str, is_encoding_explicitly_set:

def get(
self,
prefix: str = "",
prefix: str = None,
path: list = None,
target: str = None,
datatype: str = "all",
Expand Down Expand Up @@ -471,12 +471,19 @@ def get(
raise gNMIException("Conversion of gNMI paths to the Protobuf format failed", e)

try:
gnmi_message_request = GetRequest(
prefix=protobuf_prefix,
path=protobuf_paths,
type=pb_datatype,
encoding=pb_encoding,
)
if prefix is None and target is None:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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,
)
Comment on lines +474 to +486
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

debug_gnmi_msg(self.__debug, gnmi_message_request, "gNMI request")

gnmi_message_response = self.__stub.Get(gnmi_message_request, metadata=self.__metadata)
Expand Down Expand Up @@ -589,7 +596,7 @@ def set(
replace: list = None,
update: list = None,
encoding: str = None,
prefix: str = "",
prefix: str = None,
target: str = None,
extension: dict = None,
):
Expand Down Expand Up @@ -673,20 +680,35 @@ def set(
)

if gnmi_extension:
gnmi_message_request = SetRequest(
prefix=protobuf_prefix,
delete=del_protobuf_paths,
update=update_msg,
replace=replace_msg,
extension=[gnmi_extension],
)
if prefix is None and target is None:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I was only staying true to the current coding convention in the codebase.

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],
)
Comment on lines +683 to +697
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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],
)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldnt this be more pythonic ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
gnmi_message_request = SetRequest(
prefix=protobuf_prefix,
delete=del_protobuf_paths,
update=update_msg,
replace=replace_msg,
)
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,
)
Comment on lines +699 to +711
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@rao-aneesh rao-aneesh Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

debug_gnmi_msg(self.__debug, gnmi_message_request, "gNMI request")

gnmi_message_response = self.__stub.Set(gnmi_message_request, metadata=self.__metadata)
Expand Down
Loading