Staged preview refinement for vpc_pair with additional tests

[sivakasi-cisco]

This PR fixes the "stage now, deploy later" flow for vPC pairs.

Before, re-running to deploy a previously staged pair did nothing (changed: false) and left it stuck pending.

Now the module detects pending work on save (not just deploy), trusts the switch's real status over the controller's optimistic summary, and reports changed: true correctly.

Added 4 integration tests in addition (staged ladder, check-mode, deploy-scope, negative).

[allenrobel]

Code review

Two observations on the sync-state refinement — one comment/dead-code cleanup in query.py, one question about the changed promotion in resources.py. The utils.py OperationType.UPDATE fix looks correct (verified register_action_api_call has a single caller, and the per-call Results scoping means no blast radius).

🤖 Generated with Claude Code

[allenrobel]

Just a comment rather than a review comment, but it would be helpful to reviewers if you describe what this PR does in the PR description, along with a description of the changes and why they are needed.

[sivakasi-cisco]

Just a comment rather than a review comment, but it would be helpful to reviewers if you describe what this PR does in the PR description, along with a description of the changes and why they are needed.

Hi Allen, True. I moved it from draft to open and had missed it

[nikhilsrikrishna]

Could you please confirm whether this is the intended behavior for config_actions.type: switch? Currently, the module appears to deploy every switch in the fabric that is not explicitly in-sync, rather than only the vPC peer switches managed by the current invocation (code). Would it be more appropriate to limit the deployment to the affected vPC peer switches?

[nikhilsrikrishna]

Problem scenario

A vPC pair deletion can be staged using save: false and deploy: false, with the intention of saving and deploying it in a later module invocation.

During the staged deletion, the controller removes the pair from the vPC pair inventory before the switch-side removal is saved or deployed. When the module is subsequently invoked with save: true and deploy: true, it sees that the pair is absent and treats the operation as complete.

The later invocation therefore returns changed: false and does not call either configSave or switchActions/deploy, even though the removal is still pending for the switches.

Steps to reproduce

1. Create and deploy a vPC pair:

   - cisco.nd.nd_manage_vpc_pair:
       fabric_name: "<fabric>"
       state: merged
       config:
         - peer1_switch_id: "<peer1>"
           peer2_switch_id: "<peer2>"
           use_virtual_peer_link: false
       config_actions:
         save: true
         deploy: true
         type: switch

2. Stage its deletion without saving or deploying:

   - cisco.nd.nd_manage_vpc_pair:
       fabric_name: "<fabric>"
       state: deleted
       config:
         - peer1_switch_id: "<peer1>"
           peer2_switch_id: "<peer2>"
       config_actions:
         save: false
         deploy: false
         type: switch

3. After this task:

   • The module reports changed: true.
   • The intended-pair inventory no longer contains the pair.
   • The direct vpcPair endpoints for both peers return {}.
   • Both switches report configSyncStatus: pending.
   • The vPC configuration has not yet been removed.
   • pendingConfig is empty because configSave has not been called.

4. Invoke the same deletion again with save and deploy enabled:

   - cisco.nd.nd_manage_vpc_pair:
       fabric_name: "<fabric>"
       state: deleted
       config:
         - peer1_switch_id: "<peer1>"
           peer2_switch_id: "<peer2>"
       config_actions:
         save: true
         deploy: true
         type: switch

5. The second invocation returns:

   changed: false
   VPC pair <peer1>-<peer2> is already unpaired on the controller.
   No deletion required.
   

   The module trace contains no calls to:

   POST /actions/configSave
   POST /switchActions/deploy
   

Controller intent verification

After the second module invocation returned changed: false, I called POST /actions/configSave directly.

Subsequent pendingConfig responses for both peers contained the staged removal commands:

pendingConfigs:
  - no interface port-channel500
  - no interface Vlan3600
  - no vlan 3600
  - no vpc domain 1
  - no feature vpc
  # plus peer-link member cleanup and interface restoration commands

This confirms that the staged removal intent was still available to the controller even though the pair was no longer present in the pair inventory.

Calling POST /switchActions/deploy directly for the same peers completed the removal. The endpoint returned HTTP 207 with status: success and message: Deployed Successfully for both switches. Final verification showed both peers inSync, empty pendingConfig responses, and empty direct vpcPair responses.

[nikhilsrikrishna]

I noticed that config_actions.save and config_actions.deploy default to true when they are not explicitly provided. Could you please confirm whether this is the expected behavior?

[nikhilsrikrishna]

Impact: Low

Problem scenario

A vPC pair can be saved without being deployed using save: true and deploy: false. Because deployment is disabled, the switches remain pending/outOfSync.

Repeating the identical save-only operation performs another configSave and reports changed: true, even when there is no declarative pair difference and the pending configuration is unchanged.

Steps to reproduce

1. Create a controller-only pair:

- cisco.nd.nd_manage_vpc_pair:
    fabric_name: "<fabric>"
    state: merged
    config:
      - peer1_switch_id: "<peer1>"
        peer2_switch_id: "<peer2>"
        use_virtual_peer_link: false
    config_actions:
      save: false
      deploy: false
      type: switch

2. Save the pending pair without deploying:

- cisco.nd.nd_manage_vpc_pair:
    fabric_name: "<fabric>"
    state: merged
    config:
      - peer1_switch_id: "<peer1>"
        peer2_switch_id: "<peer2>"
        use_virtual_peer_link: false
    config_actions:
      save: true
      deploy: false
      type: switch

3. Repeat the exact task from step 2.

Observed behavior

The repeated save-only invocation returns:

changed: true
class_diff:
  created: []
  updated: []
  deleted: []
deployment_changed: true
deployment_needed: true
deployment_paths:
  - /api/v1/manage/fabrics/<fabric>/actions/configSave

The API trace for the repeated invocation contains:

POST /api/v1/manage/fabrics/<fabric>/actions/configSave
{"type":"switch"}

HTTP 200
{"status":"Config save is completed"}

It contains no vPC-pair PUT and no switchActions/deploy request.

Direct pendingConfig reads for both peers returned the same command lists before and after the repeated operation. The pair identity and configuration were also unchanged.

The repeated task is therefore reported as changed: true solely because the still-undeployed pair triggers another configSave, whose successful result is promoted to the module-level changed value. This makes repeated save: true, deploy: false operations non-idempotent.

[sivakasi-cisco]

Could you please confirm whether this is the intended behavior for config_actions.type: switch? Currently, the module appears to deploy every switch in the fabric that is not explicitly in-sync, rather than only the vPC peer switches managed by the current invocation (code). Would it be more appropriate to limit the deployment to the affected vPC peer switches?

Hi Nikhil, thank you for the comments. There is one more PR where the config_actions are explictly handled for vpc pair here. Just to avoid repetitive changes, I have handled the default deploy to false in that.

https://github.com/CiscoDevNet/ansible-nd/pull/513/changes

[sivakasi-cisco]

@nikhilsrikrishna , Thank you for the comments.

The vPC‑pair module lets to stage a change now and apply it later, but the few bugs made that "do it later" step quietly do nothing - thank you for pointing.

"Deploy later" now actually deploys. Before, if we staged a pair (or a deletion) and re‑ran later to push it, the module looked at the controller, decided "nothing to do," reported no change, and left the switches stuck half‑done. Now it notices the still‑pending work and finishes the job.

The module now trusts the switches' real status, so it doesn't call something "done" too early. Saving twice no longer lies about changing things. Before, running a save‑only step again and again always said "changed," even when nothing was actually different. Now a repeat save‑only correctly reports no change — which is what automation expect in case of idempotency.

Now, the "stage now, apply later" flow for vPC pairs behaves right, it completes pending work, reports change status accurately, and is safe to re‑run

[sivakasi-cisco]

Comments addressed and tested

[nikhilsrikrishna]

Thanks, Sivakami, for addressing the earlier comments and explaining the changes. The latest changes address the staged-deletion issue we discussed. However, while reviewing the implementation, I noticed a possible additional edge case when the fabric contains multiple independent vPC pairs.

Consider two pairs:

• A/B — the pair being deleted.
• C/D — an unrelated pair that remains configured.

First, A/B is deleted with save: false and deploy: false. As observed in the original reproduction, A/B disappears from /vpcPairs, while switches A and B remain pending/out-of-sync because the removal has not been saved or deployed. C/D remains in /vpcPairs.

The deletion of A/B is then invoked again with save: true and deploy: true.

On this later invocation, /vpcPairs returns C/D. Because have is non-empty, the deleted-state query returns immediately (query.py):

elif have:
    return _set_lightweight_context(have)

This happens without checking whether C/D matches the requested A/B pair and prevents the fallback that reconstructs an already-absent requested pair from being reached.

The state machine consequently receives C/D as the existing pair but is asked to delete A/B. Since A/B cannot be found, the operation is recorded as no_change (resources.py).

As a result, custom_vpc_delete() and the pending-member recovery helper are not called. No pending or out-of-sync signal is recorded, so configSave and switchActions/deploy are skipped despite deploy: true.

One possible approach would be for the deleted-state query to use the requested pair identifiers as its source of truth:

• Retain requested pairs that are still present in /vpcPairs.
• Reconstruct requested pairs that are already absent so the recovery check can run.
• Exclude unrelated pairs such as C/D from deletion reconciliation.

The existing _filter_vpc_pairs_by_requested_config() helper (query.py) may be reusable or generalizable for this.

Could you please check whether this multi-pair staged-deletion scenario needs to be handled?