search

cross-platform-actions / action

Cross-platform GitHub action
MIT License
140 stars 19 forks source link

Configurability of `cache` mode for disks with qemu #67

Closed olifre closed 11 months ago

olifre commented 1 year ago

For most CI use cases, using unsafe instead of writeback is very reasonable for a significant performance gain. It seems reasonable whenever the file systems on the actual disk images do not need to stay intact in case of unexpected hardware (or kernel) failure, which is a reasonable assumption for CI (if it fails, it fails, no salvaging from the broken disk images will be done usually).

Behaviour is effectively similar to ignoring fsync() system calls, which e.g. eatmydata does which is commonly used on Debian systems e.g. for package builds to gain performance.

So my proposal would be to allow to configure the cache mode for qemu VMs, currently, writeback is hardcoded (which of course is a reasonable default, since it works well for any usecase): https://github.com/cross-platform-actions/action/blob/de0e1f18a909bf9562ca13842e70a71b8b0c8938/src/qemu_vm.ts#L60C1-L64

jacob-carlborg commented 1 year ago

Seems reasonable. Is there a reason to use writeback for this action at all, or can we just hardcode unsafe instead? Is there a risk to cause CI failures what would otherwise not occur?

olifre commented 1 year ago

Pondering about this a bit, I don't really see any reason for this action to use writeback. If the kernel, the hypervisor or the underlying hardware have a problem, CI fails anyways, and unsafe is safe as long as none of these break apart :wink: .

jacob-carlborg commented 1 year ago

Then I suggest we just change the hardcoded value to unsafe. This will make the API and the implementation of the action simpler than if it was configurable. Does that sound good?

olifre commented 1 year ago

Yes, that sounds good to me :+1:.

olifre commented 1 year ago

One more thought: Of course, if GitHub already uses unsafe (or whatever is equivalent in the underlying virtualization they use), the gains might be negligible. But it is still better IMHO to choose unsafe here, since this will work independent of GitHub's infrastructure and also for self-hosted runners (in case someone uses this action with self-hosted runners).

manxorist commented 1 year ago

I agree, I think the default can be unsafe, and writeback can be provided as an option. The only use case I can imagine that could conflict with unsafe caching would be kernel (module/driver) development that expects kernel crashes of the OS inside the VM, and wants to analyze collected data in a following step. However, I am not sure if cross-platform-actions would be even suitable for that use case as the OS image (and thus kernel) is already provided.