Feat(eos_designs): Preview: Containerlab Digital Twin - #6846
Conversation
|
Review docs on Read the Docs To test this pull request: # Create virtual environment for this testing below the current directory
python -m venv test-avd-pr-6846
# Activate the virtual environment
source test-avd-pr-6846/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/ankudinov/avd.git@clab-digital-twin#subdirectory=python-avd" --force
# Point Ansible collections path to the Python virtual environment
export ANSIBLE_COLLECTIONS_PATH=$VIRTUAL_ENV/ansible_collections
# Install Ansible collection
python -m ansible.cli.galaxy collection install git+https://github.com/ankudinov/avd.git#/ansible_collections/arista/avd/,clab-digital-twin --force
cd test-avd-pr-6846
# Run your playbook using `python -m ansible.cli.playbook path/to/playbook.yml ...`You can also test this PR using AVD playground:
|
github-actions
Bot
added
role: eos_cli_config_gen
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #6846 +/- ##
==========================================
+ Coverage 92.59% 92.65% +0.05%
==========================================
Files 781 781
Lines 42574 42662 +88
Branches 10300 10314 +14
==========================================
+ Hits 39422 39529 +107
+ Misses 1927 1913 -14
+ Partials 1225 1220 -5
🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughContainerlab Digital Twin support is added across schemas, topology generation, documentation serialization, startup-config output, Molecule scenarios, and validation fixtures. Existing Digital Twin topology artifacts and generated device documentation are also updated. ChangesContainerlab Digital Twin
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
for more information, see https://pre-commit.ci
…into clab-digital-twin
|
need to the new scenarios to |
done |
| kinds={ | ||
| default_kind: ContainerlabKind( | ||
| enforce_startup_config=True, | ||
| image="arista/ceos:latest", |
There was a problem hiding this comment.
We hardcode this assuming that all users will import ceos image naming it arista/ceos. Should we provide a flexibility to override this if needed (maybe they want a different version or they named it differently)?
For ACT we provide the following key which we later take into account during topology generaiton:
digital_twin.fabric.act_os_version
<node_type_keys.key>.defaults.digital_twin.act_os_version
There was a problem hiding this comment.
I agree with this. Especially when users want to implement this functionality internally, we are now relying on the end user to add an additional workflow to replace arista/ceos:latest, with whatever is used internally.
There was a problem hiding this comment.
Agree in principle. However let's move this to another PR. I'd prefer to have a discussion around joint ACT/cLab schema first before blindly adding containerlab_image on top of act_os_version as yet another knob. To avoid cases like fabric: {}
But absolutely has to be configurable in the future.
|
|
||
| sorted_avd_facts = sorted(fabric_documentation_facts.avd_facts.items()) | ||
|
|
||
| unsupported_devices = [device for device, facts in sorted_avd_facts if not facts.mgmt_ip or facts.mgmt_ip == "dhcp"] |
There was a problem hiding this comment.
Devices may have inband mgmt or use DHCP on OOB MGMT interface. This may bee very restrictive to raise in this case.
I would suggest to use digital_twin.mgmt_ip (if defined) for Clab and fallback to regular mgmt_ip.
These override keys are currently exposed at the following levels:
"<node_type_keys.key>.defaults.digital_twin.mgmt_ip"
"<node_type_keys.key>.node_groups.[].nodes.[].digital_twin.mgmt_ip"
"<node_type_keys.key>.node_groups.[].digital_twin.mgmt_ip"
"<node_type_keys.key>.nodes.[].digital_twin.mgmt_ip"
"device_profiles.[].digital_twin.mgmt_ip"
"devices.[].digital_twin.mgmt_ip"
There was a problem hiding this comment.
Logic handling calculation of the oob_mgmt_ip should be extended to include containerlab DT case (right now it only reacts on ACT mode)
There was a problem hiding this comment.
This should solve the problem with regular mgmt_ips sitting in the different subnets (currently this would always lead to raising)
There was a problem hiding this comment.
Let's not explode the scope of this PR. Overriding mgmt IP is certainly important, however I'd love to add basics first. It will also take a bit more than address override. Currently cLabs only supports single subnet for management. srl-labs/containerlab#3237 can help. But I have to craft and test a working environment before adding any code. If you already have one - share.
|
|
||
| return ContainerlabDigitalTwin( | ||
| name=f"{fabric_documentation_facts.fabric_name}, Containerlab Digital Twin", | ||
| prefix="", |
There was a problem hiding this comment.
Should we expose this to the user as well? Name overlapping for prod switches is probably not possible (so running Clab DT for multiple prod fabrics on the same host should be ok) but if user tries to generate containerlab topos for our example fabrics and run them on the same host then there will be container name collision
There was a problem hiding this comment.
No. The reason is - the prefix will change lab hostnames and we don't want to do that for the current implementation. This may change though once we have the full story with mgmt ip override, etc. Let's keep it clean and simple at the start - same mgmt IP, same hostname.
| return ContainerlabDigitalTwin( | ||
| name=f"{fabric_documentation_facts.fabric_name}, Containerlab Digital Twin", | ||
| prefix="", | ||
| mgmt=ContainerlabMgmt(network="custom_mgmt", ipv4_subnet=str(next(iter(unique_mgmt_networks)))), |
There was a problem hiding this comment.
network="custom_mgmt" - wouldn't this lead to the deployment failure if we try to run multiple clab topologies on the same host? If definition of the bridge matches - we risk to have ceos mgmt IP collisions. If ipv4-subnets differ for different topologies - there we'll probably face deployment error.
We should probably prepend fabric name to the bridge name, or even make this configurable by the user?
There was a problem hiding this comment.
no. we should not aim to support all possible deployment cases and expect clean environment every time. This can be discussed later, but will not be implemented in this PR
| data = ContainerlabDigitalTwin( | ||
| mgmt=ContainerlabMgmt(network="clab-mgmt", ipv4_subnet="172.16.1.0/24"), | ||
| topology=ContainerlabTopology( | ||
| nodes={1: ContainerlabNode(mgmt_ipv4="172.16.1.101")}, | ||
| kinds=(ContainerlabKind(enforce_startup_config=True, image="ceos:latest"),), | ||
| endpoint_lists=[("leaf1:eth1", "spine1:eth1")], |
There was a problem hiding this comment.
Pylance is not happy about reusing the same class names that we import from pyavd
Argument of type "ContainerlabMgmt" cannot be assigned to parameter "mgmt" of type "ContainerlabMgmt" in function "__init__"
"avd.ansible_collections.arista.avd.tests.unit.action.test_eos_designs_documentation.ContainerlabMgmt" is not assignable to "pyavd.api.fabric_documentation.ContainerlabMgmt"
Maybe we should either use actual production clab classes for this test or rename locally-redefined so that the don't overlap with imported ones?
| content["topology"]["nodes"] = { | ||
| node_name: { | ||
| "mgmt-ipv4": node_settings["mgmt-ipv4"], | ||
| "startup-config": f"intended/configs/{node_name}.cfg", |
There was a problem hiding this comment.
If we are using hardcoded path to the startup config, why not to set this when we instantiate dataclasses (meaning ContainerlabNode would just have another field startup_config that we'll set together with mgmt_ipv4). Not sure what is the benefit of manipulating this here in plugin instead of setting it in the beginning
There was a problem hiding this comment.
Also, this path hardcodes to intended/configs/{node_name}.cfg assuming that output_dir_name is always intended. But if it is not the case - path under starup-config in topology file may become unreachable
| description OOB_MANAGEMENT | ||
| no shutdown | ||
| vrf MGMT | ||
| ip address 172.16.1.101/24 |
There was a problem hiding this comment.
This Digital twin will likely be running on-prem (meaning original OOB MGMT subnet would likely be routable from the clab runner) and nodes will be reusing prod hostnames, mgmt IPs, credentials, etc. This may create a risk of pushing something unexpected to the prod.
| links=tuple(links), | ||
| ), | ||
| interface_mapping={ | ||
| "ManagementIntf": {"eth0": "Management1"}, |
There was a problem hiding this comment.
If node_config.mgmt_interface is set to Ma0 (or anything other than Ma1) then generated EOS config will have Ma0 (and not Ma1 expected by Clab and mapped to Linux eth0) which i guess may lead to mgmt connectivity issues. We probably need a special section under shared_utils.mgmt_interface to enforce OOB MA to be Ma1
There was a problem hiding this comment.
Yes, I though about it and taking management from node settings. However what if every node will have it's own custom mgmt interface? In that case we have to build int map for every node. This is not a big problem, but adds complexity and int map was added only to merge the PR a bit quicker and support "older" cEOSlab versions. Long term we want to use INTF_MAP_ETH0 env var which is coming very soon and set it to node mgmt interface.
| @@ -0,0 +1,41 @@ | |||
| --- | |||
There was a problem hiding this comment.
Would pointing language server to the schema definition be helpful with making sure all generated key are schema-compliant?
# yaml-language-server: $schema=https://raw.githubusercontent.com/srl-labs/containerlab/main/schemas/clab.schema.json
There was a problem hiding this comment.
why do we have a digital_twin folder inside a digital_twin folder
There was a problem hiding this comment.
Outer digital_twin references the name of the scenario/fabric that we picked (it could have been anything else overall). Inner digital_twin is a dedicated folder containing digital_twin artifacts (at lest for ACT mode) but is also tunable (via output_dir_name and documentation_dir_name vars)
|
Any thought to create generic names that could be used across DT environments. For example instead of act_username, we would have digital_twin_usename, or have a heirarchy of |
We certainly have to brainstorm a unified DT schema. Different PR though. |
|
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
6 participants
Change Summary
Preview: Containerlab Digital Twin
Related Issue(s)
Fixes #3379
Component(s) name
arista.avd.eos_designsHow to test
use AVD playground to generate Containerlab digital twing
Checklist
Repository Checklist
Summary by CodeRabbit
changedreporting for Containerlab startup-config output generation.