fix(drbd): prevent duplicate TCP ports after toggle-disk operations

[kvaps]

This PR fixes a regression introduced after the TCP port migration to per-node level (commit f754943, May 2025) that causes duplicate TCP port assignments during toggle-disk operations.

Root Cause

The resetStoragePools() method, originally written in 2019 (commit 95cc17d), calls ensureStackDataExists() with an empty LayerPayload. This was correct when TCP ports were stored at RscDfn level but became a regression after the port migration:

1. Empty payload → DrbdRscData created without TCP ports
2. Controller sends Pojo with empty port Set to satellites
3. Satellite's initPorts() uses preferredNewPortsRef from peer resources
4. SatelliteDynamicNumberPool.tryAllocate() always returns true (no-op)
5. Random ports from peer resources get assigned → duplicate TCP ports

Impact

Affected operations:

• Snapshot creation/restore operations
• Manual toggle-disk operations
• Any operation calling resetStoragePools()

Symptoms:

• DRBD resources fail to adjust: "port is also used" errors
• Resources stuck in StandAlone or Connecting states
• Multiple resources on same node using identical TCP ports

Solution

Remove the redundant ensureStackDataExists() call from resetStoragePools(). The calling code (e.g., CtrlRscToggleDiskApiCallHandler:1071) already invokes ensureStackDataExists() with the correct payload immediately after resetStoragePools().

This ensures:

• resetStoragePools() only resets storage pool assignments
• Layer data creation with proper TCP ports happens via caller's ensureStackDataExists()
• No DrbdRscData objects created without TCP port assignments

Fixes

• Closes Error creating rollback entry - Resource SecObjectProtection not found #454 - Duplicate TCP ports after backup/restore operations
• Fixes user reports of resources stuck in StandAlone after node reboots when toggle-disk/backup operations were in progress

Testing

Verified that:

• Toggle-disk operations no longer create resources without TCP ports
• Backup/restore operations complete without TCP port conflicts
• Resources maintain unique TCP ports across toggle-disk cycles

[kvaps]

Updated analysis — the original fix in this PR is incomplete

After extensive debugging on a production cluster, we found that removing the redundant ensureStackDataExists() from resetStoragePools() is correct but doesn't address the root cause of TCP port mismatches.

Root cause

During toggle-disk operations, removeLayerData() deletes DrbdRscData (freeing TCP ports from the number pool), then ensureStackDataExists() creates new DrbdRscData with an empty LayerPayload (no tcpPorts). The controller's initPorts() allocates new ports from the pool — which may differ from the old ports if other resources claimed them in the meantime.

The controller correctly avoids collisions in its own number pool. But if the satellite misses the update (e.g. during controller restart, network issue, or drbdadm adjust failure), it keeps the old ports while peers receive the new ones, causing DRBD connection failures (StandAlone/Connecting).

Evidence from production

• Controller had zero port collisions — all ports were unique
• Satellite .res files had different ports from what the controller assigned
• Example: pvc-d2c5ade3 on plo-csxhk-004 — controller=7187, satellite=7188
• Peer node (gld-csxhk-006) correctly had 7187 in its .res for plo-csxhk-004
• Same pattern confirmed on multiple nodes with 3+ resources affected

Proposed expanded fix

Preserve existing TCP ports during toggle-disk by copying them into the LayerPayload before removeLayerData() deletes them — similar to how copyDrbdNodeIdIfExists() already preserves the node-id.

private void copyDrbdTcpPortsIfExists(Resource rsc, LayerPayload payload) {
    Set<AbsRscLayerObject<Resource>> drbdRscDataSet = LayerRscUtils.getRscDataByLayer(
        getLayerData(apiCtx, rsc), DeviceLayerKind.DRBD);
    if (!drbdRscDataSet.isEmpty()) {
        DrbdRscData<Resource> drbdRscData = (DrbdRscData<Resource>) drbdRscDataSet.iterator().next();
        Collection<TcpPortNumber> tcpPorts = drbdRscData.getTcpPortList();
        if (tcpPorts != null && !tcpPorts.isEmpty()) {
            Set<Integer> portInts = new TreeSet<>();
            for (TcpPortNumber port : tcpPorts) {
                portInts.add(port.value);
            }
            payload.drbdRsc.tcpPorts = portInts;
        }
    }
}

Called from:

1. copyDrbdNodeIdIfExists() — covers both toggle-disk paths (normal and finishOperationInTransaction)
2. needsDeactivate path — shared storage pool case where node-id changes but ports should be preserved

This ensures the same TCP ports are reused when DrbdRscData is recreated, eliminating the window for port mismatch between controller and satellites.

[kvaps]

Pushed the expanded fix (cf28218) that preserves TCP ports during toggle-disk operations.

The new commit adds copyDrbdTcpPortsIfExists() which saves existing TCP ports into the LayerPayload before removeLayerData() deletes them. This is called from:

1. copyDrbdNodeIdIfExists() — covers both normal toggle-disk paths
2. needsDeactivate path — shared storage pool case

Combined with the original commit (removing redundant ensureStackDataExists from resetStoragePools), this prevents TCP port reassignment during toggle-disk operations.

Note: We also discovered a separate issue where the K8s CRD backend doesn't persist tcp_port_list for DrbdRscData entries created after the v1.31.1 migration (e.g. via toggle-disk). The migration correctly populates existing entries, but new entries created afterward are missing the field. This causes the controller to re-allocate ports on restart for those entries. This issue is not addressed in this PR and should be tracked separately.

[ghernadi]

I see how this is redundant with the call in CtrlRscToggleDiskApiCallHandler line 1327 . However here are my thoughts to this:

If anything, it is the call in CtrlRscToggleDiskApiCallHandler line 1327 that is the redundant one, since the resetStoragePools method could technically be called from other places in the future. Removing it as you wanted would add the risk that this future new code/feature forgets to call ensureStackDataExists which will leave a corrupt layer-tree within LINSTOR.

That would mean that the line 1327 of CtrlRscToggleDiskApiCallHandler could be moved into the end of the then-case of if (removeDisk) (i.e. currently between lines 1320 and 1321). However, with the same argument as before, adding some future new branch in the finishOperationInTransaction could also lead to forgetting to call ensureStackDataExists.

So here is what I'd rather suggest:

• Either keep the line 280 in CtrlRscLayerDataFactory to ensure that calling this method cannot be forgotten and instead just add a comment before CtrlRscToggleDiskApiCallHandler line 1327 that we simply accept this redundancy
• Make calling ensureStackDataExists in CtrlRscLayerDataFactory line 280 optional with something like this:

public void resetStoragePools(Resource rscRef)
{
    resetStoragePools(rscRef, true);
}

public void resetStoragePools(Resource rscRef, boolean callEnsureStackDataExistsRef)
{
    ... 
    if (callEnsureStackDataExistsRef)
    {
        ensureStackDataExists(...);
    }
    ...
}

And change the call in CtrlRscToggleDiskApiCallHandler line 1325 to ctrlLayerStackHelper.resetStoragePools(rsc, false);

The latter approach should prevent accidentally forgetting to call this in the future (worst case we add again a redundancy but never miss calling ensureStackDataExists entirely) but also gives the caller enough control to prevent redundancy.

[ghernadi]

I think this call is quite unintuitive. I understand why you added it, but if I see a method that is called copyA and notice that it also copies B sounds strange.

Instead of calling the method here I'd add a new method instead:

private void copyDrbdSettings(Resource rscRef, LayerPayload payloadRef)
{
    copyDrbdNodeIdIfExists(rscRef, payloadRef);
    copyDrbdTcpPortsIfExists(rscRef, payloadRef);
}

and replace the two old calls to copyDrbdNodeIdIfExists with this new copyDrbdSettings method.

[kvaps]

Thanks for the review! Addressed both points:

1. resetStoragePools now has an optional callEnsureStackDataExistsRef parameter (defaults to true), and CtrlRscToggleDiskApiCallHandler passes false to skip the redundant call.

2. Replaced the unintuitive copyDrbdTcpPortsIfExists call inside copyDrbdNodeIdIfExists with a new copyDrbdSettings wrapper that calls both methods. All call sites now use copyDrbdSettings except the needsDeactivate path which only needs copyDrbdTcpPortsIfExists (node-id changes for shared storage).

[kvaps]

We've integrated this change into Cozystack as part of cozystack/cozystack#2331