[dloszewski-cni]

Follow-up to apache#14401, as you suggested there — this is the branch I mentioned, rebased onto
fix-stale-detach-volume so the tests come with it.

Your execution-time skip is right, and shouldDetachBeSkipped is kept here as-is. What this
adds is the second half. The skip reads volume.getInstanceId() at the top of
orchestrateDetachVolumeFromVM, but the attachment row is not cleared until after the agent
round-trip returns. In my environment that round-trip ran 2-3 minutes on a busy host, and an
attach that lands inside that window is still cleared by the straggler. So the race is
narrowed rather than closed.

This makes the final clear conditional as well:

• VolumeDao.detachVolume(volumeId, vmId) puts both id and instance_id in the search
  criteria and returns whether a row actually changed. A volume that moved while the detach
  was in flight is left alone. It is the same shape as VolumeDaoImpl.updateState and
  VMInstanceDaoImpl.updateState, which already guard their updates on the state they
  expect to find.
• The caller logs which VM the volume now belongs to, re-reads it, and returns.

What this does and does not close. It closes the database-write half. It does not close
hypervisor-side divergence: by the time the update runs, the DettachCommand has already
succeeded, so on a losing update the disk may be unplugged while the row still says attached.
Not clearing is still strictly better than clearing, and the warning now names the volume and
both VMs so the inconsistency can be found, but I did not want to overstate it.

One thing I deliberately did not decide, because it is your branch. On the losing path the
early return also skips the answer.getContextParam handling — datastoreName to
poolId/poolType, volumePath to path, chainInfo. Skipping revokeAccess,
provideVMInfo and the usage event is clearly right: they would assert an ownership that is
no longer true. But those three context params record state the hypervisor actually changed,
and they are not attachment columns, so skipping them can leave path/chain_info stale for
a volume that now belongs to the other VM. Hoisting them above the conditional update would
fix that, but it widens the change and it is a storage call I would rather you made. Happy to
do it either way.

Tests, on top of the four already in this branch:

• Two at the DAO level. One asserts the update is actually restricted by instance_id and
  that both ids are bound — I wrote the first version of these without that assertion and
  they passed with the guard deleted, so the assertion on the where clause is the point.
  There is also one for the ROOT to DATADISK conversion, which was otherwise uncovered.
• Five at the service level. One of them runs the branch that actually performs the agent
  round-trip, which is the only path the race exists on; the existing tests here all use a
  stopped VM, so they never enter it.
• Two of mine overlap with your two testDetachVolumeSkipped* tests but assert more: no
  agent round-trip, no collectVmDiskStatistics, no findClusterAndHostIdForVm, no detach
  usage event. Drop whichever pair you prefer — your branch, your call.

Verified on this branch: VolumeDaoImplTest 12/12 and VolumeApiServiceImplTest 168/168.
As a control I reverted each half separately and re-ran. Removing the instance_id condition
from the DAO fails the where-clause test; reverting the caller to the unconditional clear
fails three service tests, including the round-trip one.

The attach-side case (occurrence 4 in apache#14214) is still not addressed by either patch, and I
have not root-caused it.