[cli] Dynamic cli extension via plugins #1186
Conversation
|
retest this please |
1 similar comment
|
retest this please |
|
retest this please |
1 similar comment
|
retest this please |
stepanblyschak
force-pushed
the
cli-ext
branch
from
12c6a9e to
98e8466
Compare
|
This pull request fixes 1 alert when merging ec778be3b6b22d0f2578f89d2d26d5a190b34a6c into d414970 - view on LGTM.com fixed alerts:
|
|
retest this please |
|
This pull request fixes 1 alert when merging 4b90bb79fd95951a30671c8f650ca4bb331c23a0 into 57a0b41 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 634f4afa4e253516656c4f77255da95c7d678ecc into 57a0b41 - view on LGTM.com fixed alerts:
|
Added a mechanism to extend command line interface via plugins. For every program - show, config, clear a new python package called 'plugins' is added. This package is used as a namespace for all plugins python modules. As an example mlnx.py is moved to the plugins package. In the fututre, this mechanism will be used by Application Extension infrastructure. Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@mellanox.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
otherwise, it maybe not displayed in show command list till the execution. use register method as an API between CLI and plugin. Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
stepanblyschak
force-pushed
the
cli-ext
branch
from
634f4af to
701ffef
Compare
|
This pull request fixes 1 alert when merging 701ffef into 22d79f3 - view on LGTM.com fixed alerts:
|
|
Retest this please |
There was a problem hiding this comment.
I added few comments, some of them applicable for all clear/config/show cases.
Just wanted to make a mention that there has to be a similar mechanism that needs to be evolved for all the "YANG" based UI methods, to be able to support dynamic extensions there as well.
config/main.py
Outdated
| # Load plugins and register them | ||
| helper = util_base.UtilHelper() | ||
| for plugin in helper.load_plugins(plugins): | ||
| plugin.register(config) |
There was a problem hiding this comment.
Given the plugins are not likely part of the "original" package, it would be nicer for the system to
- Log the plugins being loaded
- Check with the plugin for compatibility (leaving to plugin how it decides) and log when the plugin reports incompatibility before registering.
There was a problem hiding this comment.
- I've added a log message for plugin import and for plugin registration but with DEBUG log level so it will not appear by default in log otherwise it is going to be printed on every CLI command execution and may pollute the log.
- Thanks for suggestion. Ideally, any compatibility checks should be done on installation time - here CLI auto generation based on YANG will help a lot (phase 2), thus, if package was able to be installed it is compatible.
There is another side of this question, if plugin happens to be incompatible and this is not reflected in version constraints in manifest the whole CLI is broken (possible because plugin will throw something like AttributeError or other kind of exception). I changed the code to catch Exception and log it as error, so the CLI will still be able to work but without new commands or changed commands. This does not catch all incompatibilities possible with this approach but better then nothing.
Given that, a plugin developer can raise Exception with message why he thinks it is not compatible which basically should satisfy your req no 2.
| helper = util_base.UtilHelper() | ||
| for plugin in helper.load_plugins(plugins): | ||
| plugin.register(config) | ||
|
|
There was a problem hiding this comment.
Would there be a provision for unloading/deregistering ?
There was a problem hiding this comment.
There is no need. CLI is short living process that I doubt require unregistering, unloading plugin in the middle.
utilities_common/util_base.py
Outdated
| for _, module_name, ispkg in iter_namespace(plugins_namespace): | ||
| if ispkg: | ||
| continue | ||
| yield importlib.import_module(module_name) |
There was a problem hiding this comment.
Would it be necessary to support an "order" for loading of plugins ? Or is it the expectation for the plugin to be self contained ?
There was a problem hiding this comment.
This might be needed if plugins have cross-dependencies which is against the classical plugin system design where plugins have no cross-dependencies, they only depend on the core.
App.Ext developer is forced to not depend on other plugins here. However, we do not want the infrastructure to put restrictions about CLI design. This shows that this plugin mechanism is not an ideal choice for CLI extensions here.
Again, CLI auto-generation based on YANG modules can help a lot here, since YANG will show cross-dependencies between modules and CLI can be regenerated from scratch on installation/uninstallation/upgrade.
There was a problem hiding this comment.
We should not have any knowledge of CLI in to a generic framework. Autogenerated from YANG is just one form of CLI. Application data model should be able to support different management models.
Consider a OC-YANG based CLI. Here, one SONIC feature may share "YANG" (or data model) with other features. Dependencies would come into play.
I would think that there should be a mechanism that allows dependencies to be checked (with in the management, not at SONIC level). You can say it is plugin's responsibility to check dependencies. But I don't see any model here for a CLI plugin to see what (other cli plugins/infra) is currently available. Can we think of some model, or Can you suggest how this can be achieved ?
There was a problem hiding this comment.
"But I don't see any model here for a CLI plugin to see what (other cli plugins/infra) is currently available"
In your plugin you can do this to list what plugins are available:
pkgutil.iter_modules(show.plugins.__path__, show.plugins.__name__)
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
|
This pull request fixes 1 alert when merging 1d4da48 into 41e62c6 - view on LGTM.com fixed alerts:
|
Signed-off-by: Stepan Blyshchak <stepanb@nvidia.com>
|
This pull request fixes 1 alert when merging d815bb7 into 41e62c6 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 591788b into a50b7a2 - view on LGTM.com fixed alerts:
|
|
retest this please |
|
@jleveque and @bandaru-viswanath comments were addressed. can you please review and approve? once all the checkers are completed i would like to move forward and merge it (as no change effect on existing behaviour) |
|
@bandaru-viswanath kindly reminder, can you please review recent changes? |
|
Hi Liat,
I looked at the changes, and they are fine. However, I have a comment. It
seems we are having an implicit assumption about 'yang based generated
CLI", which is targeted for Phase#2. To me (and I mentioned in the weekly
call as well), one CLI is just one form of the management, there will be
other forms, as well as other CLI models. This infrastructure in the
current PR (application extension) can't assume any specific model. To this
extent, I added a comment to the PR requesting for clarification on a
specific item. Will wait from Stepan.
Thanks
Vissu
…On Mon, Mar 1, 2021 at 12:25 PM liat-grozovik ***@***.***> wrote:
@bandaru-viswanath <https://github.com/bandaru-viswanath> kindly
reminder, can you please review recent changes?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1186 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE7FUDQW3YKTT2XS64HFRMTTBM27FANCNFSM4S3SCQXA>
.
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
|
Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
|
This pull request fixes 1 alert when merging 8732e8c into 6ced42c - view on LGTM.com fixed alerts:
|
|
retest this please |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sonic_platform. This import statement causes unit testing problems as sonic_platform is not installed in UT environment; importing util_base.py in UT code will fail. Since, there is no infrastructure to either mock sonic_platform or install a mocked version of it UT environment I am hiding the import to the method using sonic_platform in util_base.py. Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
|
retest this please |
|
retest this please |
|
This pull request fixes 1 alert when merging 1d188c5 into 0de99c3 - view on LGTM.com fixed alerts:
|
|
retest this please |
|
@bandaru-viswanath Based on our last subgroup meeting, do you have any more comments or it can be merged from your point of view? |
|
@bandaru-viswanath kindly reminder. as all tests passes and it seems that issue raised has been discussed i believe it is time to move forward and merge before the next conflict comes in :-( |
|
@jleveque could you please re-review and approve? |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/AzurePipelines run |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Stepan Blyschak stepanb@nvidia.com
- What I did
Added a mechanism to extend command line interface via plugins.
For every program - show, config, clear a new python package called 'plugins' is added.
This package is used as a namespace for all plugins python modules. As an example mlnx.py is moved to the plugins package.
In the future, this mechanism will be used by Application Extension infrastructure.
- How I did it
- How to verify it
- Previous command output (if the output of a command-line utility has changed)
- New command output (if the output of a command-line utility has changed)