Start SM service after dbsync to avoid startup deadlock

[gthvn1]

The patch fixes an issue that can happens if a VDI operation fails on a SR and keeps the lock. In such case the XAPI can be blocked during the startup. Here is the scenario:

• Xenopsd and XAPI are restarted
• Xenopsd is looking for existing VBDs and check with hypervisor the status. If it needs to be destroyed it pushes a message DP.destroy2 on org.xen.xapi.storage queue.
• XAPI starts, nothing listens on the queue
• At some point a thread is started and handle the message posted by Xenopsd. Plugins are not yet loaded so an error No_storage_plugin_for_sr is returned to Xenopsd. Xenopsd will retry in 5s.
• XAPI continues to boot and sync its database:
  • sync_pbds: it is where SMAPI plugins are setup and now we can handle the message to destroy the VBD
  • And here there are more syncs: sync_pci_devices, sync_pif_params, ...
  • until sync_local_vdi_activations. The deadlock can happen here. If it takes more than 5s to reach the sync of VDIs, Xenopsd has time to post its DP.destroy2. The message is handled by XAPI, the lock for the VDI is taken and the SM plugin is called. We see in production that the operation never returns and so the lock is never released. When XAPI call sync on VDIs, it will sync the VDI that has the problem. And it will wait for the lock preventing the XAPI to start correctly.

So this patch moves the registration of the queue (Storage_access.start) after the synchronisation of the XAPI database. Pending messages remains in the queue and the sync of VDIs asked by XAPI database update process during the start can be done. Of course the problem with the VDI is still there but XAPI can now start.

Tracking-reference: XCPNG-3515

[psafont]

Thanks I can imagine how long this took to investigate. Did you find any comment in the git history that made the actions be oerdered this way? I'm wondering it this might have negative side effects.

This is yet another hidden startup dependency that bytes xapi. The code should really stop being a list of action and become a runnable dependency graph.

[last-genius]

It's hard to know if this reordering broke some other edge case, but we didn't spot any issues in our CI

[gthvn1]

Did you find any comment in the git history that made the actions be oerdered this way?

It was committed 14 years ago... and added after "Initialising SM state". I don't see any particular reason for it other than logical order. I also checked the commit history around that line, and I didn't see any changes indicating that it needs to be there.

[minglumlu]

Do we know why the handling of DP.destroy2 got stuck, although I understand the xapi itself, especially in the startup sequence, will not use the queue to invoke any storage functions hence the change looks safe at this point.
The DP.destory2 not releasing the lock is still a problem.
And I'm not sure about if this would cause some timing issues.

[last-genius]

Do we know why the handling of DP.destroy2 got stuck, although I understand the xapi itself, especially in the startup sequence, will not use the queue to invoke any storage functions hence the change looks safe at this point. The DP.destory2 not releasing the lock is still a problem. And I'm not sure about if this would cause some timing issues.

Guillaume is away - but the DP.destroy2 call in this case got stuck because of some storage issues with this particular VDI (the SR otherwise was responsive). We wanted xapi to be more resilient in this case and be able to start (and allow the user to operate the rest of the VDIs and VMs)