-
Notifications
You must be signed in to change notification settings - Fork 235
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(cv_deploy): Verify streaming status and raise before trying to submit Workspace #4990
base: devel
Are you sure you want to change the base?
Feat(cv_deploy): Verify streaming status and raise before trying to submit Workspace #4990
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-4990
# Activate the virtual environment
source test-avd-pr-4990/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/alexeygorbunov/avd.git@issue_4038_improve_inact_devs_err#subdirectory=python-avd" --force
# Point Ansible collections path to the Python virtual environment
export ANSIBLE_COLLECTIONS_PATH=$VIRTUAL_ENV/ansible_collections
# Install Ansible collection
ansible-galaxy collection install git+https://github.com/alexeygorbunov/avd.git#/ansible_collections/arista/avd/,issue_4038_improve_inact_devs_err --force
# Optional: Install AVD examples
cd test-avd-pr-4990
ansible-playbook arista.avd.install_examples |
We should only warn if requested state for the workspace will not need to submit it. It can be useful to do builds and preview the changes even if devices are not streaming. |
fixed
|
@@ -99,6 +120,23 @@ async def verify_devices_in_cloudvision_inventory( | |||
device.serial_number = found_device_dict_by_hostname[device.hostname].key.device_id | |||
device.system_mac_address = found_device_dict_by_hostname[device.hostname].system_mac_address | |||
|
|||
if verify_streaming: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If feels like we are building more lists and iterating over everything again and again.
Could be just set the streaming flag during the inspection above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean to extend CVDevice
class to add streaming
attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would be my suggestion. But if we decide to just submit no matter what, we don't even need to record this, but just set the force flag or not as given.
verify_streaming: bool = False, | ||
raise_if_inactive: bool = False, | ||
inactive_exception_class: type = CVInactiveDevices, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need all these extra options on this function. If you just gather the streaming state in here, you could move the invocation of the verify to the workspace submission logic where everything would make more sense. It would also mean we would not raise early (before even trying to build the workspace).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, after moving this streaming check over to submission phase - should we still raise (and avoid initiating submission) is we discover inactive devices targeted to configuration update when WS submission is not forced? Or should we just let submission to be initiated and let if fail (although we know in advance that it will fail)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I said let's not try, but what if the device started streaming meanwhile. I think we should just submit and catch the error and provide a nice message suggesting the force option.
|
Change Summary
Verify streaming status of all devices targeted by
cv_deploy
and raise (and update Errors) or update Warnings (depending on theforce
parameter of the Workspace) if any of those inactive devices are targeted by configuration update.Related Issue(s)
Fixes #4038
Component(s) name
arista.avd.cv_deploy
Proposed changes
cv_client.get_inventory_devices
inverify_devices_in_cloudvision_inventory
already fetches actual state of all targeted devices including their streaming status. Add additional attribute_streaming
to describe streaming state ofCVDevice
s.forced
andinactive
devices are targeted for configuration update - raise without wasting compute and time cycles trying to submit Workspace which will 100% fail. Expose details regarding discoveredinactive
devices via result (errors
)forced
andinactive
devices are targeted for configuration update - proceed with Workspace submission but still expose details regarding discoveredinactive
devices via result (warnings
)How to test
cv_deploy
molecule tests targeting CI environmentcv_deploy with cv_submit_workspace_force = true
test:cv_deploy with cv_submit_workspace_force = false
test:Checklist
User Checklist
Repository Checklist