Fix apswitch template rendering - #25830
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@pdam-arista - Reviewed the PR. The fix looks correct to me: CI is green, and the 202605 backport request is linked to #25829 with failure details. No blockers from my side. @auspham could you also review this change? |
auspham
left a comment
There was a problem hiding this comment.
Thanks for the fix - the diagnosis and approach are correct. ansible.template.trust_as_template is exactly the public API added in ansible-core 2.19 for the template trust-model inversion, and wrapping the disk-read template before self._templar.template() is the right, minimal fix for the unrendered-Jinja → "No PFC storm detected" failure (#25829).
One blocking concern before merge: the import is unguarded and hard-breaks on ansible-core < 2.19.
from ansible.template import trust_as_templatetrust_as_template only exists in ansible-core 2.19+, so on any older core this from ... import ... raises ImportError at plugin load time. apswitch.py is the action plugin for all Arista fanout operations (not just PFCWD), so this would break every play that uses it on older ansible-core - a much wider blast radius than the bug being fixed. Green CI only exercises the CI image (2.19+); it does not cover the older cores this repo still carries compatibility code for.
This also diverges from the repo convention of staying version-tolerant rather than hard-breaking, e.g.:
ansible/plugins/callback/yaml.py-if isinstance(AnsibleDumper, type) and hasattr(...)(guards the 2.20 change)ansible/plugins/connection/multi_passwd_ssh.py-try: import importlib ... except ImportError: import imptests/common/helpers/parallel.py/tests/common/devices/base.py- runtime detection for 2.18 vs 2.19+
Could you guard the import with an identity fallback? On <2.19 strings are trusted/templated by default, so the passthrough preserves the old behavior:
try:
from ansible.template import trust_as_template
except ImportError: # ansible-core < 2.19: strings are trusted/templated by default
def trust_as_template(data):
return dataIf the project has definitively set ansible-core 2.19+ as the minimum, this becomes optional - but I could not find a hard version floor, and the surrounding code still supports older cores, so I would guard it to be safe.
Nit (non-blocking, pre-existing): the f = open(...) / f.close() around the changed line would read cleaner as a with open(...) as f: block.
With the latest Ansible behavior, arbitrary strings passed to Templar are not rendered as templates unless they are marked trusted. ansible/plugins/action/apswitch.py reads the EOS PFC storm template from disk and passes the raw file content to Templar. Since the content was not trusted, Jinja variables were not rendered before sending the command to the fanout. As a result, raw Jinja content was sent to EOS and pfc_gen_brcm_xgs.py was not launched. This caused pfcwd tests to fail with no PFC storm detected on the selected DUT port. Trust the template content before passing it to Templar so the EOS fanout command is rendered correctly. Signed-off-by: Pratik Dam <pdam@arista.com>
3f70445 to
df91711
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
auspham
left a comment
There was a problem hiding this comment.
Update looks good - thanks for the quick turnaround. Both points from the previous review are fully addressed:
- The import is now guarded with an identity fallback (
try: from ansible.template import trust_as_template / except ImportError: def trust_as_template(data): return data), so apswitch keeps working on ansible-core < 2.19 (where strings are trusted/templated by default) as well as 2.19+. No leftover unguarded import. - The file read is now a
with open(...)block.
The runtime behavior on 2.19+ is unchanged from the earlier commit that already passed the full suite, and the lint/static-analysis gates are green on the new commit. Approving. (Heavy KVM CI is still running - merge should wait for it to go green.)
|
Thanks @auspham for the review and approval. |
|
Hi @StormLiangMS can this merged now ? |
|
This PR has backport request for branch(es): 202605. ---Powered by SONiC BuildBot
|
|
Cherry-pick PR to 202605: #26021 |
<!-- Please make sure you've read and understood our contributing guidelines; https://github.com/sonic-net/SONiC/blob/gh-pages/CONTRIBUTING.md Please provide following information to help code review process a bit easier: --> ### Description of PR <!-- - Please include a summary of the change and which issue is fixed. - Please also include relevant motivation and context. Where should reviewer start? background context? - List any dependencies that are required for this change. --> Summary: Fixes sonic-net#25829 <!-- If you request a backport/cherry-pick below, link the GitHub issue or ADO work item here (for example, "Fixes #<issue>" or "ADO: <work item URL>"). --> ### Type of change <!-- - Fill x for your type of change. - e.g. - [x] Bug fix --> - [x] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [ ] Test case improvement ### Back port request <!-- Only check a release or feature branch when the PR links a GitHub issue or ADO work item above. The linked tracker should explain the failure in detail, including whether it is a day-one issue or a regression, the affected branch/image/platform/test, and why this branch needs the fix. Backport or cherry-pick requests without a linked issue/work item may not be favored. --> - [ ] 202311 - [ ] 202405 - [ ] 202411 - [ ] 202505 - [ ] 202511 - [ ] 202512 - [x] 202605 Tracking issue/work item for backport/cherry-pick request: Failure type: <!-- day-one issue / regression / other --> ### Approach #### What is the motivation for this PR? With latest Ansible ( 2.19+) behavior , arbitrary strings passed to `Templar` are not rendered as templates unless they are marked trusted. Refer: https://docs.ansible.com/projects/ansible/latest/porting_guides/porting_guide_core_2.19.html#template-trust-model-inversion Under `Template trust model inversion`, Ansible documents the new behavior: Only strings marked as loaded from a trusted source are eligible to be rendered as templates. The same section also states that custom plugins sourcing template strings must use public APIs to apply trust where appropriate. That is the case here: `ansible/plugins/action/apswitch.py` is a custom action plugin that reads the EOS PFC storm template from disk and passes the raw string to `Templar`. With latest Ansible, that string is left unrendered unless it is wrapped with `trust_as_template()`. `apswitch.py` reads the EOS PFC storm template from disk and passes the raw file content to `Templar`. Since the content was not trusted, Jinja variables were not rendered before sending the command to the fanout. As a result, raw Jinja content was sent to EOS and `pfc_gen_brcm_xgs.py` was not launched. This caused PFCWD tests to fail with no PFC storm detected on the selected DUT port. #### How did you do it? Imported `trust_as_template` from `ansible.template` and wrapped the file content read from the EOS fanout template before passing it to `self._templar.template()`. #### How did you verify/test it? ##### Before the fix: - pfc_gen_brcm_xgs.py was not started on the fanout. - PFCWD tests failed with No port matching EthernetXX detected storm. ##### After the fix: - pfc_gen_brcm_xgs.py started correctly on the fanout. - Example passing test: #### Any platform specific information? No #### Supported testbed topology if it's a new test case? No ### Documentation <!-- (If it's a new feature, new test case) Did you update documentation/Wiki relevant to your implementation? Link to the wiki page? --> N/A Signed-off-by: Pratik Dam <pdam@arista.com> Signed-off-by: selldinesh <dinesh.sellappan@keysight.com>
<!-- Please make sure you've read and understood our contributing guidelines; https://github.com/sonic-net/SONiC/blob/gh-pages/CONTRIBUTING.md Please provide following information to help code review process a bit easier: --> ### Description of PR <!-- - Please include a summary of the change and which issue is fixed. - Please also include relevant motivation and context. Where should reviewer start? background context? - List any dependencies that are required for this change. --> Summary: Fixes sonic-net#25829 <!-- If you request a backport/cherry-pick below, link the GitHub issue or ADO work item here (for example, "Fixes #<issue>" or "ADO: <work item URL>"). --> ### Type of change <!-- - Fill x for your type of change. - e.g. - [x] Bug fix --> - [x] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [ ] Test case improvement ### Back port request <!-- Only check a release or feature branch when the PR links a GitHub issue or ADO work item above. The linked tracker should explain the failure in detail, including whether it is a day-one issue or a regression, the affected branch/image/platform/test, and why this branch needs the fix. Backport or cherry-pick requests without a linked issue/work item may not be favored. --> - [ ] 202311 - [ ] 202405 - [ ] 202411 - [ ] 202505 - [ ] 202511 - [ ] 202512 - [x] 202605 Tracking issue/work item for backport/cherry-pick request: Failure type: <!-- day-one issue / regression / other --> ### Approach #### What is the motivation for this PR? With latest Ansible ( 2.19+) behavior , arbitrary strings passed to `Templar` are not rendered as templates unless they are marked trusted. Refer: https://docs.ansible.com/projects/ansible/latest/porting_guides/porting_guide_core_2.19.html#template-trust-model-inversion Under `Template trust model inversion`, Ansible documents the new behavior: Only strings marked as loaded from a trusted source are eligible to be rendered as templates. The same section also states that custom plugins sourcing template strings must use public APIs to apply trust where appropriate. That is the case here: `ansible/plugins/action/apswitch.py` is a custom action plugin that reads the EOS PFC storm template from disk and passes the raw string to `Templar`. With latest Ansible, that string is left unrendered unless it is wrapped with `trust_as_template()`. `apswitch.py` reads the EOS PFC storm template from disk and passes the raw file content to `Templar`. Since the content was not trusted, Jinja variables were not rendered before sending the command to the fanout. As a result, raw Jinja content was sent to EOS and `pfc_gen_brcm_xgs.py` was not launched. This caused PFCWD tests to fail with no PFC storm detected on the selected DUT port. #### How did you do it? Imported `trust_as_template` from `ansible.template` and wrapped the file content read from the EOS fanout template before passing it to `self._templar.template()`. #### How did you verify/test it? ##### Before the fix: - pfc_gen_brcm_xgs.py was not started on the fanout. - PFCWD tests failed with No port matching EthernetXX detected storm. ##### After the fix: - pfc_gen_brcm_xgs.py started correctly on the fanout. - Example passing test: #### Any platform specific information? No #### Supported testbed topology if it's a new test case? No ### Documentation <!-- (If it's a new feature, new test case) Did you update documentation/Wiki relevant to your implementation? Link to the wiki page? --> N/A Signed-off-by: Pratik Dam <pdam@arista.com> Signed-off-by: selldinesh <dinesh.sellappan@keysight.com>
<!-- Please make sure you've read and understood our contributing guidelines; https://github.com/sonic-net/SONiC/blob/gh-pages/CONTRIBUTING.md Please provide following information to help code review process a bit easier: --> ### Description of PR <!-- - Please include a summary of the change and which issue is fixed. - Please also include relevant motivation and context. Where should reviewer start? background context? - List any dependencies that are required for this change. --> Summary: Fixes sonic-net#25829 <!-- If you request a backport/cherry-pick below, link the GitHub issue or ADO work item here (for example, "Fixes #<issue>" or "ADO: <work item URL>"). --> ### Type of change <!-- - Fill x for your type of change. - e.g. - [x] Bug fix --> - [x] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [ ] Test case improvement ### Back port request <!-- Only check a release or feature branch when the PR links a GitHub issue or ADO work item above. The linked tracker should explain the failure in detail, including whether it is a day-one issue or a regression, the affected branch/image/platform/test, and why this branch needs the fix. Backport or cherry-pick requests without a linked issue/work item may not be favored. --> - [ ] 202311 - [ ] 202405 - [ ] 202411 - [ ] 202505 - [ ] 202511 - [ ] 202512 - [x] 202605 Tracking issue/work item for backport/cherry-pick request: Failure type: <!-- day-one issue / regression / other --> ### Approach #### What is the motivation for this PR? With latest Ansible ( 2.19+) behavior , arbitrary strings passed to `Templar` are not rendered as templates unless they are marked trusted. Refer: https://docs.ansible.com/projects/ansible/latest/porting_guides/porting_guide_core_2.19.html#template-trust-model-inversion Under `Template trust model inversion`, Ansible documents the new behavior: Only strings marked as loaded from a trusted source are eligible to be rendered as templates. The same section also states that custom plugins sourcing template strings must use public APIs to apply trust where appropriate. That is the case here: `ansible/plugins/action/apswitch.py` is a custom action plugin that reads the EOS PFC storm template from disk and passes the raw string to `Templar`. With latest Ansible, that string is left unrendered unless it is wrapped with `trust_as_template()`. `apswitch.py` reads the EOS PFC storm template from disk and passes the raw file content to `Templar`. Since the content was not trusted, Jinja variables were not rendered before sending the command to the fanout. As a result, raw Jinja content was sent to EOS and `pfc_gen_brcm_xgs.py` was not launched. This caused PFCWD tests to fail with no PFC storm detected on the selected DUT port. #### How did you do it? Imported `trust_as_template` from `ansible.template` and wrapped the file content read from the EOS fanout template before passing it to `self._templar.template()`. #### How did you verify/test it? ##### Before the fix: - pfc_gen_brcm_xgs.py was not started on the fanout. - PFCWD tests failed with No port matching EthernetXX detected storm. ##### After the fix: - pfc_gen_brcm_xgs.py started correctly on the fanout. - Example passing test: #### Any platform specific information? No #### Supported testbed topology if it's a new test case? No ### Documentation <!-- (If it's a new feature, new test case) Did you update documentation/Wiki relevant to your implementation? Link to the wiki page? --> N/A Signed-off-by: Pratik Dam <pdam@arista.com> Signed-off-by: ssithaia-ebay <ssithaian@ebay.com>
|
@vmittal-msft can we please get a request for 202511 and msft-202601 on here too? The breaking change this PR fixes exists on both branches and causes pfcwd mgmt tests to fail because of the ansible bump up |
|
This PR has backport request label(s) for branch(es): 202605, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202605: <test result>) in the Test result section as well in your PR description. ---Powered by SONiC BuildBot
|
|
The label |
|
Cherry-pick PR to 202511: #27008 |
|
This PR has backport request label(s) for branch(es): 202511,msft-202601, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202511: <test result>) in the Test result section as well in your PR description. ---Powered by SONiC BuildBot
|
<!-- Please make sure you've read and understood our contributing guidelines; https://github.com/sonic-net/SONiC/blob/gh-pages/CONTRIBUTING.md Please provide following information to help code review process a bit easier: --> ### Description of PR <!-- - Please include a summary of the change and which issue is fixed. - Please also include relevant motivation and context. Where should reviewer start? background context? - List any dependencies that are required for this change. --> Summary: Fixes sonic-net#25829 <!-- If you request a backport/cherry-pick below, link the GitHub issue or ADO work item here (for example, "Fixes #<issue>" or "ADO: <work item URL>"). --> ### Type of change <!-- - Fill x for your type of change. - e.g. - [x] Bug fix --> - [x] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [ ] Test case improvement ### Back port request <!-- Only check a release or feature branch when the PR links a GitHub issue or ADO work item above. The linked tracker should explain the failure in detail, including whether it is a day-one issue or a regression, the affected branch/image/platform/test, and why this branch needs the fix. Backport or cherry-pick requests without a linked issue/work item may not be favored. --> - [ ] 202311 - [ ] 202405 - [ ] 202411 - [ ] 202505 - [ ] 202511 - [ ] 202512 - [x] 202605 Tracking issue/work item for backport/cherry-pick request: Failure type: <!-- day-one issue / regression / other --> ### Approach #### What is the motivation for this PR? With latest Ansible ( 2.19+) behavior , arbitrary strings passed to `Templar` are not rendered as templates unless they are marked trusted. Refer: https://docs.ansible.com/projects/ansible/latest/porting_guides/porting_guide_core_2.19.html#template-trust-model-inversion Under `Template trust model inversion`, Ansible documents the new behavior: Only strings marked as loaded from a trusted source are eligible to be rendered as templates. The same section also states that custom plugins sourcing template strings must use public APIs to apply trust where appropriate. That is the case here: `ansible/plugins/action/apswitch.py` is a custom action plugin that reads the EOS PFC storm template from disk and passes the raw string to `Templar`. With latest Ansible, that string is left unrendered unless it is wrapped with `trust_as_template()`. `apswitch.py` reads the EOS PFC storm template from disk and passes the raw file content to `Templar`. Since the content was not trusted, Jinja variables were not rendered before sending the command to the fanout. As a result, raw Jinja content was sent to EOS and `pfc_gen_brcm_xgs.py` was not launched. This caused PFCWD tests to fail with no PFC storm detected on the selected DUT port. #### How did you do it? Imported `trust_as_template` from `ansible.template` and wrapped the file content read from the EOS fanout template before passing it to `self._templar.template()`. #### How did you verify/test it? ##### Before the fix: - pfc_gen_brcm_xgs.py was not started on the fanout. - PFCWD tests failed with No port matching EthernetXX detected storm. ##### After the fix: - pfc_gen_brcm_xgs.py started correctly on the fanout. - Example passing test: #### Any platform specific information? No #### Supported testbed topology if it's a new test case? No ### Documentation <!-- (If it's a new feature, new test case) Did you update documentation/Wiki relevant to your implementation? Link to the wiki page? --> N/A Signed-off-by: Pratik Dam <pdam@arista.com> Signed-off-by: Raghavendran Ramanathan <rraghav@cisco.com>
Description of PR
Summary:
Fixes #25829
Type of change
Back port request
Tracking issue/work item for backport/cherry-pick request:
Failure type:
Approach
What is the motivation for this PR?
With latest Ansible ( 2.19+) behavior , arbitrary strings passed to
Templararenot rendered as templates unless they are marked trusted.
Refer:
https://docs.ansible.com/projects/ansible/latest/porting_guides/porting_guide_core_2.19.html#template-trust-model-inversion
Under
Template trust model inversion, Ansible documents the new behavior:Only strings marked as loaded from a trusted source are eligible to be
rendered as templates.
The same section also states that custom plugins sourcing template strings
must use public APIs to apply trust where appropriate. That is the case here:
ansible/plugins/action/apswitch.pyis a custom action plugin that reads theEOS PFC storm template from disk and passes the raw string to
Templar.With latest Ansible, that string is left unrendered unless it is wrapped with
trust_as_template().apswitch.pyreads the EOS PFC storm template from disk and passes the rawfile content to
Templar. Since the content was not trusted, Jinja variableswere not rendered before sending the command to the fanout. As a result, raw
Jinja content was sent to EOS and
pfc_gen_brcm_xgs.pywas not launched.This caused PFCWD tests to fail with no PFC storm detected on the selected
DUT port.
How did you do it?
Imported
trust_as_templatefromansible.templateand wrapped the filecontent read from the EOS fanout template before passing it to
self._templar.template().How did you verify/test it?
Before the fix:
After the fix:
Any platform specific information?
No
Supported testbed topology if it's a new test case?
No
Documentation
N/A