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

Feat(eos_config_deploy_cvp)!: Support for dynamic inventories #2395

Conversation

ClausHolbechArista
Copy link
Contributor

@ClausHolbechArista ClausHolbechArista commented Jan 11, 2023

Change Summary

Add capability to read inventory from memory instead of yaml file

Related Issue(s)

Fixes #2063

Component(s) name

arista.avd.eos_config_deploy_cvp

Proposed changes

  • Make inventory argument optional and default to reading from memory
  • Implement action plugin to read from memory using Ansible's InventoryManager object
  • Remove default inventory file value
  • Add optional "avd_inventory_to_container_file" variable to force reading from file.

How to test

  • Updated molecule scenario
  • Updated integration tests

Checklist

User Checklist

  • N/A

Repository Checklist

  • My code has been rebased from devel before I start
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation and documentation have been updated accordingly.
  • I have updated molecule CI testing accordingly. (check the box if not applicable)

@ClausHolbechArista ClausHolbechArista requested a review from a team as a code owner January 11, 2023 22:01
@github-actions github-actions bot added module: inventory_to_container issue related to inventory_to_container module role: eos_config_deploy_cvp issue related to eos_config_deploy_cvp role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated labels Jan 11, 2023
@github-actions github-actions bot added role: build_output_folders issue related to build_output_folders role role: cvp_configlet_upload issue related to cvp_configlet_upload role role: dhcp_provisioner role: eos_cli_config_gen issue related to eos_cli_config_gen role role: eos_config_deploy_eapi issue related to eos_config_deploy_eapi role role: eos_designs issue related to eos_designs role role: eos_snapshot role: eos_validate_state type: code quality CI and development toolset type: documentation Improvements or additions to documentation labels Jan 12, 2023
@ClausHolbechArista ClausHolbechArista marked this pull request as draft January 12, 2023 10:56
@github-actions github-actions bot added the state: conflict PR with conflict label Jan 17, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the state: conflict PR with conflict label Jan 18, 2023
@github-actions
Copy link

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed role: build_output_folders issue related to build_output_folders role role: eos_cli_config_gen issue related to eos_cli_config_gen role role: eos_config_deploy_eapi issue related to eos_config_deploy_eapi role type: documentation Improvements or additions to documentation role: eos_snapshot labels Jan 18, 2023
@github-actions github-actions bot removed type: code quality CI and development toolset role: cvp_configlet_upload issue related to cvp_configlet_upload role role: dhcp_provisioner role: eos_validate_state role: eos_designs issue related to eos_designs role labels Jan 18, 2023
@ClausHolbechArista ClausHolbechArista marked this pull request as ready for review January 23, 2023 08:00
@ClausHolbechArista ClausHolbechArista changed the title Feat(eos_config_deploy_cvp): Add capability to read inventory from memory instead of yaml file Feat(eos_config_deploy_cvp): support for dynamic inventories Jan 25, 2023
@ClausHolbechArista ClausHolbechArista changed the title Feat(eos_config_deploy_cvp): support for dynamic inventories Feat(eos_config_deploy_cvp): Support for dynamic inventories Jan 25, 2023
@JulioPDX
Copy link
Contributor

Testing with ATD (left everything default), I get the following error

TASK [arista.avd.eos_config_deploy_cvp : Generate intended variables] *******************
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: NoneType: None
fatal: [cv_atd1]: FAILED! => {"changed": false, "msg": "arista.avd.inventory_to_container: Group 'ATD_LEAFS' has more than one parent group ({ATD_TENANTS_NETWORKS, ATD_FABRIC, ATD_SERVERS}) below the 'container_root'. Unable to build CloudVision container hierarchy."}

@ClausHolbechArista
Copy link
Contributor Author

ClausHolbechArista commented Jan 27, 2023

Testing with ATD (left everything default), I get the following error

TASK [arista.avd.eos_config_deploy_cvp : Generate intended variables] *******************
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: NoneType: None
fatal: [cv_atd1]: FAILED! => {"changed": false, "msg": "arista.avd.inventory_to_container: Group 'ATD_LEAFS' has more than one parent group ({ATD_TENANTS_NETWORKS, ATD_FABRIC, ATD_SERVERS}) below the 'container_root'. Unable to build CloudVision container hierarchy."}

Thank you for testing. Apparently there is a mistake in that inventory, where the ATD_TEANATS_NETWORKS and ATD_SERVERS are configured inside ATD_FABRIC instead of at the same level. This makes the ATD_LEAFS have three "parents", so not a clean tree structure.

I reproduced this with an integration test, and the current yml parsing simply creates these groups with no devices in them.
How do we want to proceed? @carlbuchmann

  1. Try to keep the current behavior, so simply take the first parent group and ignore the rest? The risk here is that we pick the wrong one, like in your output above, the first one is ATD_TENANTS_NETWORKS. Maybe I can improve the code to get a predictable ordering.

  2. Make this a breaking change highlighting the new stricter behavior in RN and docs

Remember that if someone insists on the current behavior, they can just configure the avd_inventory_to_container_file: "{{ inventory_file }}"

@carlbuchmann
Copy link
Member

  1. Try to keep the current behavior, so simply take the first parent group and ignore the rest? The risk here is that we pick the wrong one, like in your output above, the first one is ATD_TENANTS_NETWORKS. Maybe I can improve the code to get a predictable ordering.
  2. Make this a breaking change highlighting the new stricter behavior in RN and docs

Remember that if someone insists on the current behavior, they can just configure the avd_inventory_to_container_file: "{{ inventory_file }}"

+1 on option # 2 to make this a breaking change. Let's start the porting guide to describe the behavior change and how we can retain existing behavior.

@carlbuchmann carlbuchmann self-requested a review January 27, 2023 15:22
@github-actions github-actions bot added the type: documentation Improvements or additions to documentation label Jan 27, 2023
@ClausHolbechArista ClausHolbechArista changed the title Feat(eos_config_deploy_cvp): Support for dynamic inventories Feat(eos_config_deploy_cvp)!: Support for dynamic inventories Jan 27, 2023
@ClausHolbechArista ClausHolbechArista merged commit 7a68fae into aristanetworks:devel Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: inventory_to_container issue related to inventory_to_container module rn: Feat(eos_config_deploy_cvp)! role: eos_config_deploy_cvp issue related to eos_config_deploy_cvp role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated type: documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for dynamic inventories
3 participants