Refactor common network shared code into package #76

privateip · 2017-09-21T15:30:33Z

Date: 2017/9/21

Status: New
Proposal type: network
Targeted Release: 2.5
Associated PR: n/a
Estimated time to implement: 2 weeks

Motivation

The rapid pace of module development for network modules has amassed a significant amount of technical debt related to the network shared code found in ansible/module_utils. This proposal is designed to provide details on creating a stable network package in ansible/module_utils/network and move the current shared network libraries into the new package. The ansible.module_utils.network package will only contain shared code that is common for all network modules to implement and will be maintained in a stable way moving forward.

[edit]
Amending this proposal to include a note of clarification. The only code that will live in the newly proposed package would be things that are truly platform agnostic and shareable across any and all network modules. This means there should be no platform specific code in this package

Problems

What problems exist that this proposal will solve?

Current network shared modules are not stable in their implementation
Limited or no unit tests exist for shared functions
Changes from release to release break functionality

What problems exist that this proposal will solve?

Stabilize the current network shared libraries
Consolidate all of the network shared code under a single package (ansible.module_utils.network)
Provide unit tests for shared code
Document how the network shared libraries are to be used

Solution proposal

In order to solve the problems mentioned above, this proposal will refactor a number of shared functions that currently reside in module_utils. The goal for the refactoring will be to introduce changes only where necessary to complete the refactor. All of the current functions in module_utils will remain and be deprecated per community process so as to provide backwards compatibility.

In order to solve the current problem, the following is proposed:

Create ansible.module_utils.network
Refactor module_utils/netcfg.py into module_utils/network/config.py
- remove CustomNetworkConfig
Refactor module_utils/network_common.py into module_utils/network/common.py
- remove Entity, EntityCollection, ComplexDict, ComplexList (all can be replaced by subspecs)
Create module_utils/network/parsing.py
- Combine shared code from the following modules
  - module_utils/netcli.py
  - plugins/filters/network.py (no change in function this is just moving code around)
  - [edit] relicense code from filter to module_utils moving from GPL to BSD
Deprecate the following:
- module_utils/netcfg.py
- module_utils/network_common.py
- module_utils/network.py
- module_utils/netcli.py
Update core modules with new package imports
Add unit test cases for all modules in module_utils/network
Provide developer documentation and inline comments for module_utils/network
[edit] Update porting guide here (create developers section under networking)

Performing this work will help to stabilize the common code base used for developing network modules and add the missing test cases. Once completed new changes introduced into ansible.module_utils.network will be done to maintain backwards compatibility, keep the package stable and well documented.

Dependencies (optional)

Testing (optional)

See comments in proposal around testing

Documentation (optional)

See comments in proposal around documentation

Anything else?

The text was updated successfully, but these errors were encountered:

dagwieers · 2017-09-21T17:12:56Z

Looking good. I assume that you mean

Refactor module_utils/netcfg.py into module_utils/network/config.py
Refactor module_utils/network_common.py into module_utils/network/common.py

Do we expect that e.g. ACI modules will also move their common library from module_utils/aci.py to module_utils/network/aci.py ?

gundalow · 2017-09-21T17:16:48Z

@dagwieers

Do we expect that e.g. ACI modules will also move their common library from module_utils/aci.py to module_utils/network/aci.py?

Answer: No. The new new directory is for code shared by multiple platforms

privateip · 2017-09-21T17:31:06Z

@dagwieers

Looking good. I assume that you mean
Refactor module_utils/netcfg.py into module_utils/network/config.py
Refactor module_utils/network_common.py into module_utils/network/common.py

Thanks, corrected in the proposal

Do we expect that e.g. ACI modules will also move their common library from module_utils/aci.py to module_utils/network/aci.py ?

Only if there are things that are truly common across anything network related. For instance, one thing that might common is functions for manipulating IP address information.

gundalow · 2017-09-21T17:38:27Z

+1 to proposal

Does 2 weeks include time to write unit tests?
Current coverage for the libraries is fairly low, so we will need to write a lot of new unit tests https://codecov.io/gh/ansible/ansible/tree/devel/lib/ansible/module_utils
Is this something we plan to do early during 2.5 development to allow partners to update their code to use these newer stable functions?
Is there any non-network specific code that is currently in these files that need to be moved else where (not basic.py), thinking about common filters

privateip · 2017-09-21T17:38:31Z

I have updated the proposal to include a comment about what type of code should reside in ansible.module_utils.network.

[edit]
Amending this proposal to include a note of clarification. The only code that will live in the newly proposed package would be things that are truly platform agnostic and shareable across any and all network modules. This means there should be no platform specific code in this package

privateip · 2017-09-21T17:41:23Z

Does 2 weeks include time to write unit tests?
Current coverage for the libraries is fairly low, so we will need to write a lot of new unit tests https://codecov.io/gh/ansible/ansible/tree/devel/lib/ansible/module_utils

Yes

Is this something we plan to do early during 2.5 development to allow partners to update their code to use these newer stable functions?

Yes, I don't anticipate much push back in getting this approved so the implementation can start very early.

Is there any non-network specific code that is currently in these files that need to be moved else where (not basic.py), thinking about common filters

Its a fair ask. I will go investigate and respond here

gundalow · 2017-09-21T19:22:06Z

Will also need documenting in https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/porting_guide_2.5.rst (create developers section under networking)

privateip · 2017-09-21T19:39:00Z

Will also need documenting in https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/porting_guide_2.5.rst (create developers section under networking)

Proposal updated with comment

abadger · 2017-09-21T20:41:33Z

Code moved from plugins/filters/network.py will need to be relicensed.

privateip · 2017-09-21T21:28:09Z

[edit] relicense code from filter to module_utils moving from GPL to BSD

Proposal updated with comment about re-licensing

itdependsnetworks · 2017-09-22T12:53:59Z

This seems like a good long term move, but in terms of priorities where does this fit? To me there are more pressing issues.

dagwieers · 2017-09-22T13:16:02Z

Ken, let's collect a list of stuff that people think is important on the Wiki at: https://github.com/ansible/community/wiki/Network:Core-community-plan

Having an overview of what people think is needed will help in structuring possible areas of attention and is helpful even for new or existing work. For the Windows Working Group this really helped solve some misconceptions, find areas that need improvement (docs, community), and bring in new ideas.

That said, people are always free to work on what they like or need themselves. It helps if we have a consensus of what we want, and how to get there. And the more people joining that conversation, the better the result.

dagwieers · 2017-09-22T13:21:39Z

For Windows we started off using Etherpad, which is very good for collaborating on content, but very bad for synthesizing that information. So we ended up moving everything into the Wiki. Maybe we could do the same for the Network WG ?

The Github Wiki is very (very, very) bad at concurrent access so probably Etherpad is best to start from.

itdependsnetworks · 2017-09-22T13:42:16Z

Agreed, I think @gundalow was collecting all of our initial comments. (Correct me if I am wrong)

dagwieers · 2017-09-22T14:06:35Z

I thought that was more about how the Network WG could be improved.

gundalow · 2017-09-23T13:05:22Z

@itdependsnetworks I was mainly collecting feedback around the Working Group process, no about specific development tasks.

privateip · 2017-09-27T16:11:48Z

~~approved to be implemented in ansible networking irc meeting~~

dagwieers · 2017-09-27T16:36:58Z

So, there's an argument wrt. if module_utils/network/ is to be used for all network-related libraries or only the generic libraries. Since the name is quite generic "network" and we have a lot of vendor-specific libraries in module_utils/ it is probably wise to have a similar structure in module_utils/ as we already have established in modules/ which means avi.py would live in module_utils/network/.

privateip · 2017-09-27T16:41:55Z

Holding this for one more week before finalizing to address @dagwieers comments. Original proposal is to create a package that does not include platform specific code but only common shared code for network modules.

Idea to change the package name to one of the following:

ansible.module_utils.network_common
ansible.module_utils.network.common
ansible.module_utils.network.shared

Amendment on the table to create a structure that more reflects the modules/ layout in module_utils/. Discussion to be added to core irc meeting agenda and hashed out there

privateip · 2017-10-04T15:48:51Z

After having some one off individual discussions, proposal has been updated to move forward with the following packaging:

ansible.module_utils.network.common - command shared functions
ansible.module_utils.network.{{ platform }} - where platform is platform specific shared functions

dagwieers · 2017-10-04T16:05:14Z

@privateip Perfect as that will work much better with how ansibot is designed. With this scheme it will automatic label PRs with files in these directories.

gundalow · 2017-10-04T16:30:51Z

AGREED in Network IRC meeting Wednesday 4th Oct

caphrim007 · 2017-11-30T21:45:16Z

how does the proposal accommodate cases where the following occurs.

bigip_* and bigiq_* share code amongst themselves (classes, functions, constants). One does not want to copy the module code from one to the other

Would any of the following be true? (examples are directories unless otherwise specified). Which of the following would be preferred

Case 1
ansible.module_utils.network.bigiq (specific to bigiq)
ansible.module_utils.network.bigip (specific to bigip)
ansible.module_utils.network.f5 (common for all F5 products)

Case 2
ansible.module_utils.network.f5.bigiq (specific to bigiq)
ansible.module_utils.network.f5.bigip (specific to bigip)
ansible.module_utils.network.f5.common (common for all F5 products)

Case 3
ansible.module_utils.network.bigiq (specific to bigiq)
ansible.module_utils.network.bigip (specific to bigip)
ansible.module_utils.network.f5.common (a file, common for all F5 products)

Case 4
ansible.module_utils.network.bigiq (specific to bigiq)
ansible.module_utils.network.bigip (specific to bigip)
ansible.module_utils.network.f5_utils (a file, common for all F5 products)

privateip · 2017-11-30T21:51:19Z

Case 2 would be the preferred, imho

…e (part-1) As per proposal ansible#76 refactor common network shared and platform specific code into sub-package. ansible/proposals#76 * ansible.module_utils.network.common - command shared functions * ansible.module_utils.network.{{ platform }} - where platform is platform specific shared functions

…e (part-1) As per proposal ansible#76 refactor common network shared and platform specific code into sub-package. ansible/proposals#76 * ansible.module_utils.network.common - command shared functions * ansible.module_utils.network.{{ platform }} - where platform is platform specific shared functions * Fix review comments

…33452) * Refactor common network shared and platform specific code into package (part-1) As per proposal #76 refactor common network shared and platform specific code into sub-package. ansible/proposals#76 * ansible.module_utils.network.common - command shared functions * ansible.module_utils.network.{{ platform }} - where platform is platform specific shared functions * Fix review comments * Fix review comments

* As per proposal ansible/proposals#76 deprecate use of Entity, EntityCollection, ComplexDict, ComplexList and use subspec instead. * Refactor ios modules * Refactor eos modules * Refactor vyos modules * Refactor nxos modules * Refactor iosxr modules * Add support for key in suboptions handling

…#33575) * Deprecate Entity, EntityCollection and use subspec in network modules * As per proposal ansible/proposals#76 deprecate use of Entity, EntityCollection, ComplexDict, ComplexList and use subspec instead. * Refactor ios modules * Refactor eos modules * Refactor vyos modules * Refactor nxos modules * Refactor iosxr modules * Add support for key in suboptions handling * Fix CI issues

gundalow · 2018-01-26T14:16:44Z

Code is complete (thanks @ganeshrn)
Only thing left before closing this is to add some details to https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/porting_guide_2.5.rst#networking

gundalow · 2018-01-26T14:17:15Z

For reference, new packages are live and can be found in https://github.com/ansible/ansible/tree/devel/lib/ansible/module_utils/network

ganeshrn · 2018-04-12T05:19:09Z

Done as part of PR ansible/ansible#33452

privateip mentioned this issue Sep 21, 2017

Network Working Group Meeting Agenda 2 ansible/community#247

Closed

privateip added the Agreed label Sep 27, 2017

privateip removed the Agreed label Sep 27, 2017

gundalow added the Agreed label Oct 4, 2017

privateip mentioned this issue Oct 4, 2017

refactor network common code into package [COMMON_LIB] ansible/ansible#31323

Closed

gundalow mentioned this issue Dec 4, 2017

ec2_group fix CIDR with host bits set - fixes #25403 ansible/ansible#29605

Merged

ganeshrn mentioned this issue Dec 5, 2017

Deprecate Entity, EntityCollection and use subspec in network modules ansible/ansible#33575

Merged

ganeshrn closed this as completed Apr 12, 2018

gundalow mentioned this issue May 14, 2018

Memset: initial module, integration tests, module_utils and supporting documentation. ansible/ansible#40071

Merged

bcoca added the Completed label Apr 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor common network shared code into package #76

Refactor common network shared code into package #76

privateip commented Sep 21, 2017 •

edited

Loading

dagwieers commented Sep 21, 2017

gundalow commented Sep 21, 2017 •

edited

Loading

privateip commented Sep 21, 2017

gundalow commented Sep 21, 2017

privateip commented Sep 21, 2017

privateip commented Sep 21, 2017

gundalow commented Sep 21, 2017

privateip commented Sep 21, 2017

abadger commented Sep 21, 2017

privateip commented Sep 21, 2017

itdependsnetworks commented Sep 22, 2017

dagwieers commented Sep 22, 2017 •

edited

Loading

dagwieers commented Sep 22, 2017 •

edited

Loading

itdependsnetworks commented Sep 22, 2017

dagwieers commented Sep 22, 2017

gundalow commented Sep 23, 2017

privateip commented Sep 27, 2017 •

edited

Loading

dagwieers commented Sep 27, 2017

privateip commented Sep 27, 2017 •

edited

Loading

privateip commented Oct 4, 2017

dagwieers commented Oct 4, 2017

gundalow commented Oct 4, 2017

caphrim007 commented Nov 30, 2017 •

edited

Loading

privateip commented Nov 30, 2017

gundalow commented Jan 26, 2018

gundalow commented Jan 26, 2018

ganeshrn commented Apr 12, 2018

Refactor common network shared code into package #76

Refactor common network shared code into package #76

Comments

privateip commented Sep 21, 2017 • edited Loading

Motivation

Problems

Solution proposal

Dependencies (optional)

Testing (optional)

Documentation (optional)

Anything else?

dagwieers commented Sep 21, 2017

gundalow commented Sep 21, 2017 • edited Loading

privateip commented Sep 21, 2017

gundalow commented Sep 21, 2017

privateip commented Sep 21, 2017

privateip commented Sep 21, 2017

gundalow commented Sep 21, 2017

privateip commented Sep 21, 2017

abadger commented Sep 21, 2017

privateip commented Sep 21, 2017

itdependsnetworks commented Sep 22, 2017

dagwieers commented Sep 22, 2017 • edited Loading

dagwieers commented Sep 22, 2017 • edited Loading

itdependsnetworks commented Sep 22, 2017

dagwieers commented Sep 22, 2017

gundalow commented Sep 23, 2017

privateip commented Sep 27, 2017 • edited Loading

dagwieers commented Sep 27, 2017

privateip commented Sep 27, 2017 • edited Loading

privateip commented Oct 4, 2017

dagwieers commented Oct 4, 2017

gundalow commented Oct 4, 2017

caphrim007 commented Nov 30, 2017 • edited Loading

privateip commented Nov 30, 2017

gundalow commented Jan 26, 2018

gundalow commented Jan 26, 2018

ganeshrn commented Apr 12, 2018

privateip commented Sep 21, 2017 •

edited

Loading

gundalow commented Sep 21, 2017 •

edited

Loading

dagwieers commented Sep 22, 2017 •

edited

Loading

dagwieers commented Sep 22, 2017 •

edited

Loading

privateip commented Sep 27, 2017 •

edited

Loading

privateip commented Sep 27, 2017 •

edited

Loading

caphrim007 commented Nov 30, 2017 •

edited

Loading