CP-308455 VM.sysprep CA-414158 wait for "action" key to disappear #6606
Conversation
ocaml/xapi/vm_sysprep.ml
Outdated
| Client.VBD.eject ~rpc ~session_id ~vbd ; | ||
| Sys.remove iso | ||
| with exn -> | ||
| debug "%s: ejecting CD failed: %s" __FUNCTION__ (Printexc.to_string exn) ; |
There was a problem hiding this comment.
Is warn or error log level more suitable?
| Client.VBD.insert ~rpc ~session_id ~vdi ~vbd ; | ||
| Thread.delay !Xapi_globs.vm_sysprep_wait ; | ||
| match trigger ~domid ~uuid ~timeout with | ||
| match trigger ~rpc ~session_id ~domid ~uuid ~timeout ~vbd ~iso with |
There was a problem hiding this comment.
the result is boolean if .. then .. can be used.
| match trigger ~rpc ~session_id ~domid ~uuid ~timeout ~vbd ~iso with | |
| if not (trigger ~rpc ~session_id ~domid ~uuid ~timeout ~vbd ~iso) then | |
| fail VM_sysprep_timeout |
ocaml/xapi/vm_sysprep.ml
Outdated
| () | ||
| | false -> | ||
| debug "%s: sysprep timeout, ejecting CD" __FUNCTION__ ; | ||
| ( try eject ~rpc ~session_id iso vbd |
There was a problem hiding this comment.
We no longer have this, what if any exception is raised from sysprep, or sysprep itself times out? Shouldn't we try to eject the CD? I think we're only ejecting CDs on the good path now
There was a problem hiding this comment.
You are right; if sysprep does not start or finish within its respective timeout, we would not eject the CD. I'll try to use a finally to ensure this. It feels like this is increasingly delicate.
There was a problem hiding this comment.
I have addressed this - please take another look
There was a problem hiding this comment.
I am planning to squash commits before merging.
There was a problem hiding this comment.
This is ok now (well the same as before). To avoid issues with the CD getting stuck (if the VM doesn't allow ejecting) then it might be useful to see whether we could set some kind of flag to unplug on next boot (setting VBD.is_empty, and clearing the VDI there might work, but not sure what effect that might have on the live VM, since there'd be an inconsitency between what the DB represents and what is live on the VM; although that is not so unusual: we change other fields like VM.platform that way, and they'll take effect on next boot).
Although the VDI itself needs to be cleaned up too, we'd need to mark it as destroy-on-next-boot (we don't have that mechanism currently, we only have reset-on-boot, but that is different, only makes sense for writable VDIs).
Although if the VM refuses the CD eject, and doesn't reboot for a long time then we might still leak the VBD and VDI (e.g. if it refuses eject, and then shuts down instead of rebooting).
There was a problem hiding this comment.
We could (Sys.remove, which I assume is unlink) the file itself, although that'll then result in the VM failing to boot due to a missing VDI.
…pi-project#6604) Change when a CD is ejected because ideally the VM is still running at this point: * wait for sysprep no longer being reported as running * eject Make sure we still eject the CD if we hit a timeout before reaching that point. Signed-off-by: Christian Lindig <[email protected]>
lindig
force-pushed
the
private/christianlin/vm-sysprep
branch
from
ef16d86 to
b014ce1
Compare
3 participants
We have seen failures to eject the CD at the end; catch exceptions when failing to eject the ISO/CD from the VM.