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

Fix(eos_config_deploy_cvp): share tags from parent to dependent tasks. #3333

Merged
merged 3 commits into from Nov 16, 2023
Merged

Conversation

arista-matt
Copy link
Contributor

@arista-matt arista-matt commented Nov 9, 2023

Change Summary

share tags from parent to dependent tasks

Related Issue(s)

Fixes #3260

Component(s) name

arista.avd.eos_config_deploy_cvp

Proposed changes

arista.cvp.cv_task_v3 depends upon arista.cvp.cv_container_v3's cvp_container_results, but doesn't share the containers tag. If the containers tag is skipped, arista.cvp.cv_task_v3 will fail due to missing cvp_container_results.

How to test

The following documented/suggested commandansible-playbook playbook.to.deploy.with.cvp.yml --skip-tags "containers" should not fail with the error documented in the attached issue: #3260 However, it's also clear that if --skip-tags provision is used it will similarly fail. So using --skip-tags with either/both provision and containers successfully would indicate this issue has been fixed.

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)

@arista-matt arista-matt requested review from a team as code owners November 9, 2023 16:32
@github-actions github-actions bot added the role: eos_config_deploy_cvp issue related to eos_config_deploy_cvp role label Nov 9, 2023
@carlbuchmann
Copy link
Member

@arista-matt, thanks for providing a fix. Can you provide an example in the PR on how to test and the expected results?

@@ -29,7 +29,7 @@
register: cvp_container_results

- name: "Execute pending tasks on {{ inventory_hostname }}"
tags: [apply]
tags: [apply, containers, provision]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the apply tag is only supposed to be used with --skip-tags and not alone with --tags. Your change here might make sense for your specific workflow, but it does not match everything else in the role, so it will be even harder for someone else to figure out what to do.
If we want to implement the tags for the apply steps as well, we should do it everywhere, but I am a bit worried about the side-effects, so doing this in a minor release is not good.

Example, someone runs with --tags provision today. They get all the changes applied to cloudvision, but no tasks are executed (hitless). If we implement your suggestion, we would suddenly execute those tasks. That is a (dangerous) behavioral change.

Instead we can add some new tags. For this task it could be execute_cv_tasks_for_container_changes and for the one above execute_cv_tasks_for_configlet_changes and below execute_cv_tasks_for_device_changes, giving granular control.
(Better names are welcome, but it needs to be clear - and documented).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ClausHolbechArista - the information you provided here should likely be included in the documentation, making it more prescriptive.

I've reverted the tag-based change for this specific task and tested the variables throughout.

Copy link
Contributor

@ClausHolbechArista ClausHolbechArista left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment. Blocking merge.

@arista-matt
Copy link
Contributor Author

@carlbuchmann, I've updated the information to include how to test.

@ClausHolbechArista ClausHolbechArista requested a review from a team November 13, 2023 09:20
@ClausHolbechArista ClausHolbechArista added the one approval This PR has one approval and is only missing one more. label Nov 13, 2023
Copy link
Contributor

@sugetha24 sugetha24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ClausHolbechArista ClausHolbechArista merged commit 4715378 into aristanetworks:devel Nov 16, 2023
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
one approval This PR has one approval and is only missing one more. rn: Fix(eos_config_deploy_cvp) role: eos_config_deploy_cvp issue related to eos_config_deploy_cvp role
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arista.cvp.cv_task_v3 dependency: arista.cvp.cv_container_v3
4 participants