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.

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 iosxr grpc plugin and grpc_nw_get module #52857

Open
wants to merge 4 commits into
base: devel
from

Conversation

@ganeshrn
Copy link
Member

ganeshrn commented Feb 22, 2019

SUMMARY
  • Add iosxrv 9k gRPC plugin implementation
  • Add grpc_nw_get module to fetch configuration
    and state data from the network device with gRPC enabled
  • Add integration test for grpc_nw_get module

Related to

  1. #52823

  2. #38820

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/module_utils/network/grpc/init.py
lib/ansible/module_utils/network/grpc/grpc.py
lib/ansible/modules/network/grpc/init.py
lib/ansible/modules/network/grpc/grpc_nw_get.py
lib/ansible/plugins/grpc/init.py
lib/ansible/plugins/grpc/iosxr.py
lib/ansible/plugins/grpc/iosxr_pb/init.py
lib/ansible/plugins/grpc/iosxr_pb/ems_grpc_pb2.py

ADDITIONAL INFORMATION

Add iosxr gRPC plugin and grpc_nw_get module
*  Add iosxrv 9k gRPC plugin implementation
*  Add `grpc_nw_get` module to fetch configuration
   and state data from network device with gRPC enabled
*  Add integration test for `grpc_nw_get` module

@ganeshrn ganeshrn changed the title Add iosxr gRPC plugin and grpc_nw_get module Add iosxr grpc plugin and grpc_nw_get module Feb 22, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 22, 2019

@ganeshrn, just so you are aware we have a dedicated Working Group for network.
You can find other people interested in this in #ansible-network on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ganeshrn ganeshrn force-pushed the ganeshrn:iosxrv_grpc_support branch from 1dbd629 to 97b4e66 Feb 23, 2019

@ansible ansible deleted a comment from ansibot Feb 23, 2019

@ansibot ansibot removed the needs_revision label Feb 23, 2019

@ansibot ansibot added the core_review label Feb 23, 2019

@Qalthos
Copy link
Contributor

Qalthos left a comment

My biggest concern here is whether we need a new grpc plugin type at all. It's hard to tell with just one implementation, but it takes no options and mostly takes direction from variables in self._connection. If the location of the pb file could be inferred from network_os (or whatever else), then I don't see a compelling reason to have a grpc plugin, though admittedly I have no idea how any of this translates to another platform (if at all)

@@ -104,7 +104,7 @@ def __getitem__(self, y):
DEFAULT_SUBSET = None
DEFAULT_SU_PASS = None
# FIXME: expand to other plugins, but never doc fragments
CONFIGURABLE_PLUGINS = ('become', 'cache', 'callback', 'cliconf', 'connection', 'httpapi', 'inventory', 'lookup', 'shell')
CONFIGURABLE_PLUGINS = ('become', 'cache', 'callback', 'cliconf', 'connection', 'httpapi', 'inventory', 'lookup', 'shell', 'grpc')

This comment has been minimized.

@Qalthos

Qalthos Feb 26, 2019

Contributor

It doesn't look like grpc is actually configurable.

This comment has been minimized.

@ganeshrn

ganeshrn Feb 26, 2019

Author Member

ansible-doc CI test fails if the plugin is not added as a configurable plugin that's the reason it is added here and basic configuration document is added in plugin

def channel(self):
return self._connection._channel

def get_config(self, section=None):

This comment has been minimized.

@Qalthos

Qalthos Feb 26, 2019

Contributor
Suggested change
def get_config(self, section=None):
@abstractmethod
def get_config(self, section=None):

This comment has been minimized.

@ganeshrn

ganeshrn Feb 26, 2019

Author Member

The grpc protocol does not define any specific api's or RPC as in the case of netconf and restconf. The gRPC api's support for any platform is defined in the protobuf files, that's the reason these methods are not defined as abstractmethod as some of the platform may not have the need to implement these api's

This comment has been minimized.

@Qalthos

Qalthos Mar 4, 2019

Contributor

I guess if these methods aren't guaranteed to exist, then why present them in the base class at all? What is the benefit of having this method exist but be unimplemented on an implementation that doesn't use it?

Show resolved Hide resolved lib/ansible/plugins/grpc/__init__.py
Show resolved Hide resolved lib/ansible/plugins/grpc/__init__.py
@samccann
Copy link
Contributor

samccann left a comment

A few nit docs comments.

I also created #52997 so we can add grpc into the platform options table. It requires a bit of doc cleanup so it all fits so I'll take that issue and request your review when I'm done.

DEFAULT_GRPC_PLUGIN_PATH:
name: GRPC Plugins Path
default: ~/.ansible/plugins/grpc:/usr/share/ansible/plugins/grpc
description: Colon separated paths in which Ansible will search for GRPC Plugins.

This comment has been minimized.

@samccann

samccann Feb 26, 2019

Contributor
Suggested change
description: Colon separated paths in which Ansible will search for GRPC Plugins.
description: Colon separated paths in which Ansible will search for gRPC plugins.
Show resolved Hide resolved lib/ansible/config/base.yml Outdated
Show resolved Hide resolved lib/ansible/modules/network/grpc/grpc_nw_get.py Outdated
Show resolved Hide resolved lib/ansible/modules/network/grpc/grpc_nw_get.py Outdated
Show resolved Hide resolved lib/ansible/modules/network/grpc/grpc_nw_get.py Outdated
Show resolved Hide resolved lib/ansible/modules/network/grpc/grpc_nw_get.py Outdated
Show resolved Hide resolved lib/ansible/plugins/grpc/iosxr.py Outdated
Show resolved Hide resolved lib/ansible/plugins/grpc/iosxr.py Outdated
Show resolved Hide resolved test/integration/targets/grpc_nw_get/tests/iosxr/basic.yaml Outdated
Show resolved Hide resolved test/integration/targets/grpc_nw_get/tests/iosxr/basic.yaml Outdated
@ganeshrn

This comment has been minimized.

Copy link
Member Author

ganeshrn commented Feb 26, 2019

@Qalthos IMO gRPC plugin is required because the implementation varies based on the capabilities defined in the python bindings generated from protocol buffers and varies based on platform. Also, the protocol buffer is added in the individual plugin folder instead of mdoule_utils because the grpc modules does not directly consume these python bindings. IMO only the code that can be reused in modules should be placed under module_utils. Also the stub object (created from python binding) requires the gRPC channel object to pass to it https://github.com/ansible/ansible/pull/52857/files#diff-640ddcbe8e941027a8af1ebf0841390bR30

@ganeshrn ganeshrn requested a review from Qalthos Feb 27, 2019

@dagwieers dagwieers added the cisco label Feb 27, 2019

@samccann
Copy link
Contributor

samccann left a comment

docs portion LGTM

Fix plugin documentability
This should avoid the ansible-doc error
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 4, 2019

@ansibot ansibot added the test label Mar 4, 2019

@Qalthos Qalthos dismissed their stale review Mar 4, 2019

Points taken

@ansibot ansibot added core_review and removed needs_revision labels Mar 4, 2019

@nitzmahone

This comment has been minimized.

Copy link
Member

nitzmahone commented Mar 4, 2019

Uhh, adding yet more new plugin types is probably something that deserves at least a modicum of discussion with the core team, especially when there is only one implementation (how do you know you've got the abstraction correct with a single implemented use case?). This definitely needs some discussion before being merged; we're trying really hard to stem the tide of new plugin types and API surface areas that we'll have to support for many years, so this decision can't come lightly.

@ansibot ansibot added the stale_ci label Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.