Repository navigation
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Controller-only validation for head The suite covers forward and reverse fallback, remembered passwords, one-cycle termination before the starting pair repeats, duplicate and missing alternatives, new-option resets, censored/opaque connection failures, key authentication, ordinary remote-command failure, and state restoration. Fourteen cases use the real command/SFTP/SCP/piped-transfer builders with only SSH process execution mocked; both normal and no_log contexts return from IPv6 to IPv4, and the command target/control path is rebuilt. No DUT, SONiC image or physical deployment was used. CI and physical integration results are not claimed complete. No backport was requested. |
There was a problem hiding this comment.
🟡 Changes recommended
Per-task set_options calls currently discard the remembered successful candidate.
1 open finding
What changed in this PR
Refactors SSH connection handling to perform bounded rotation across endpoint/password combinations.
Changes:
- Adds retry rotation for commands, uploads, and downloads.
- Adds controller-only regression coverage.
- Documents retry behavior and test invocation.
| File | Description |
|---|---|
ansible/plugins/connection/multi_passwd_ssh.py |
Implements endpoint and credential rotation. |
tests/common/unit_tests/connections/unit_test_multi_passwd_ssh.py |
Tests retry ordering, state restoration, and transfer paths. |
tests/common/unit_tests/README.md |
Documents behavior and test execution. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
7e83577 to
33a36f9
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Current source validation: head All 35 controller-only regression cases passed with Ansible core 2.20.9. Coverage includes unchanged-options cursor preservation, active endpoint consistency, the real reset path targeting the correct check/stop socket, and 14 real command/SFTP/SCP/piped-builder cases. Applicable repository pre-commit checks passed and the committed tree equals the tested tree. Both initial Copilot findings and the two redundant-binding CodeQL comments are implemented and resolved. No DUT/image validation was performed; current-head CI is not claimed complete. No backport requested. |
There was a problem hiding this comment.
🟡 Changes recommended
Reapplying unchanged options leaves the remembered endpoint inconsistent, causing connection resets to target the wrong endpoint.
1 open finding
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Keep supplied endpoints separate from the active connection and try each distinct endpoint/password pair at most once per operation. Retain a successful pair across unchanged options, keep reset targeting consistent, and preserve censored connection failures without parsing stderr. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6df9e99c-2f5d-44d2-9e64-25271fa13595 Signed-off-by: Xichen Lin <lukelin0907@gmail.com>
33a36f9 to
fcfe6da
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
🟡 Changes recommended
The exhausted-retry exception hides the final actionable SSH failure diagnostic.
1 open finding
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Remove the committed regression module and its README section as requested. Retain the validation harness and results in the session workspace; the plugin implementation is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6df9e99c-2f5d-44d2-9e64-25271fa13595 Signed-off-by: Xichen Lin <lukelin0907@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Restart from the original plugin, preserving its password policy, comments and low-level hooks. Remember the primary before IPv6 fallback, try the other supplied endpoint once, and prevent nested piped transfers from repeating the cycle. Keep validation outside the PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6df9e99c-2f5d-44d2-9e64-25271fa13595 Signed-off-by: Xichen Lin <lukelin0907@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Xichen96
changed the title
|
Minimal restart v2.0: Private controller validation:16 real-plugin command scenarios and12 SFTP/SCP/piped upload/download combinations passed with Ansible2.20.9 and SSH execution simulated. Checks cover reverse fallback, stopping after both endpoints fail, bytes/string host restoration, existing password/error handling and no_log behavior. A piped-transfer duplicate cycle was reproduced and the small recursion guard removes it. AST comparison verifies the original low-level entry points and password policy, excluding only the added recursion flag bookkeeping. Repository pre-commit passed; committed tree equals the tested candidate. This is not a retry-on-every-error or winning-password-cache change: existing timeout/no-route/no_log and authentication guards remain. No DUT/image validation or current-head CI pass is claimed. No backport requested. |
There was a problem hiding this comment.
🟡 Changes recommended
Endpoint state can retry the same failed host, and file-transfer path handling can raise TypeError.
2 open findings
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| if (isinstance(ssh_args[idx], bytes) and ssh_args[idx].decode() == self.host) or \ | ||
| ssh_args[idx] == self.host: |
| # First, try with the current host (generally IPv4) with multi-password | ||
| return _conn_with_multi_pwd(self, *args, **kwargs) | ||
| except AnsibleConnectionFailure as e: | ||
| orig_host = self._play_context.remote_addr |
Description of PR
Summary:
Add bounded reverse fallback to the original
multi_passwd_sshimplementation.Keep its password loop, low-level retry hooks, error policy and explanatory comments.
Task: https://msazure.visualstudio.com/One/_workitems/edit/40065307
Parent PBI: https://msazure.visualstudio.com/One/_workitems/edit/40056625
Currently, a successful fallback leaves the connection on the alternate endpoint.
If that endpoint subsequently fails, the plugin does not try the original
primary. When both initial attempts fail, restoring the primary setting is not
another connection attempt.
Scope: only the shared connection plugin, covering command execution and both
upload and download operations. Controller-only regression checks are retained
outside the PR; no test module or test README changes are included. No particular
physical pytest testcase is claimed as fixed.
This is independent of the metadata/template changes in
#28367 and contains none of those
changes. Both PRs touch the connection plugin: the small
no_logfallbackcondition is retained here as well. Reconcile that
overlap when merging; the metadata PR's separate release-delivery requirements
are not a request to backport this refactor.
Type of change
Back port request
Tracking issue/work item for backport/cherry-pick request (GitHub issue or Microsoft ADO): No backport requested.
Failure type: pre-existing one-way transport retry limitation.
Tested branch
Test result
Review version: v2.0, head
f4142ad4c3a4d58b1a4ee9d1bbeb713401118b1a.Restarted from the original public-master implementation. Baseline -> v2.0:
one file, +24/-12. Rejected v1.1 -> v2.0: +131/-73, mostly restoring original
code and comments. No tests or README changes are included.
Current minimal-patch evidence: #28445 (comment)
Historical rejected-rewrite evidence: #28445 (comment)
Earlier preparation-head evidence: #28445 (comment)
Controller-only source validation of the minimal replacement, based on public master
5cbfdf6d1bd26cea0fedccc7feff2850900d6f6e, using Ansible core 2.20.9:16 command scenarios and 12 upload/download combinations passed, together with
repository pre-commit checks. The checks and results are retained outside the PR.
No SONiC image or DUT was used; PR CI and physical validation are not claimed complete.
Approach
What is the motivation for this PR?
Transport selection should work in either direction without losing the supplied
primary after a successful alternate connection. Fix that gap without replacing
the existing password-retry implementation or broadening its error policy.
How did you do it?
Use the existing single retry; if it also fails, restore the starting endpoint
and raise the error without attempting the starting endpoint again.
_conn_with_multi_pwdpolicy and_run/_file_transport_commandoverrides. Do not add candidate lists, a successful-password cursor, new public-operation overrides or
set_optionshandling.because an earlier fallback can leave a string host argument.
_runfrom starting a second retry cycle.Password order, password restoration, the existing hash behavior and error
filtering remain unchanged. Endpoint fallback still requires timeout/no-route
or
no_log, with the existing authentication and same-host guards. This is nota retry-on-every-error expansion. Ansible's own configured reconnect retries
remain independent of the single alternate-endpoint attempt.
How did you verify/test it?
Private checks use the actual plugin and Ansible retry layer with SSH execution
simulated. They cover forward/reverse fallback, stopping after both endpoints
fail, host-argument restoration, unchanged password order and authentication
guards, censored failures, and ordinary command errors. Upload/download checks
cover SFTP, SCP and piped transfers with and without
no_log. The piped-transfercheck reproduced a repeated cycle before the recursion guard and verifies the
final code attempts only the current and alternate endpoints.
Any platform specific information?
No platform-specific behavior or new runtime dependencies.
Supported testbed topology if it's a new test case?
Not a new DUT test case. The shared connection plugin is topology-independent;
the private regression checks are controller-only.
Documentation
Original comments are retained; only directional wording was adjusted where
fallback is now bidirectional. No test documentation changes are included.