feat: Default volumes and images when versions set

[Callisto13]

When users only set the KernelVersion and OsVersion fields, we fill in the other fields required by flintlock based on those values.

For example, if given

spec:
  template:
    spec:
      kernelVersion: 5.10.77
      osVersion: 1.23.5

We fill in the rest with:

spec:
  template:
    spec:
      rootVolume:
        id: root
        image: ghcr.io/weaveworks-liquidmetal/capmvm-kubernetes:1.23.5
      kernel:
        filename: "boot/vmlinux"
        image: ghcr.io/weaveworks-liquidmetal/kernel-bin:5.10.77
      additionalVolumes:
      - id: modules
        image: ghcr.io/weaveworks-liquidmetal/kernel-modules:5.10.77
        mountPoint: /lib/modules/5.10.77

convertToFlintlockAPI has also been refactored to make it easier to read, and tests have been added.

Part of liquidmetal-dev/cluster-api-provider-microvm#226

Follows #2

[Callisto13]

@richardcase what do you think of this idea?

Connected PRs

1. feat: Add kernelVersion and osVersion to microvm #2
2. Then this pr
3. feat: Add short options for kernel and os versions cluster-api-provider-microvm#255

[yitsushi]

I love all the little pieces of this PR. The tests, the github action, the simplifications or newVM... all of it.

[Callisto13]

😊 very sweet review, BUT i have just seen one thing that is wrong, and one thing which I could probably name better 😅

[Callisto13]

Actually... I think I have changed my mind about having this feature.

The kernel and os paths get harder to predict the more variation we add. For example, there is a firecracker-kernel-bin and a cloudhypervisor-kernel-bin depending on which provider you use. Yes we could use the eventual spec.provider key which I am about to add, but the default for that is actually controlled on the host machine so we would not be able to set one at all at this level, so some people would not be able to use this feature. Also there are other things like the kernel bin location, which depending on how we do ARM images (see liquidmetal-dev/image-builder#57), may end up somewhere else and also be impossible to default correctly.

Also the validation to make sure various fields are set correctly exists somewhere else, which just seems brittle to me and something we are likely to forget or be confused about later.

Honestly this feature was just a 'nice to have', and now feels more like a 'pain in the ass to have'. Most users likely will not be harmed by having to specify things in yaml and it is probably better to have less magic going on anyway.

I think I want to keep all the refactoring I did and remove the new bits, wdyt @yitsushi ?

[yitsushi]

I think I want to keep all the refactoring I did and remove the new bits.

That's what I wanted to suggest too after I read the title of the notification: "Actually... I think I have changed my mind about having this feature."

Yes we could use the eventual spec.provider key which I am about to add, but the default for that is actually controlled on the host machine so we would not be able to set one at all at this level, so some people would not be able to use this feature.

That's an interesting topic, this issue exists without this patch, someone tries to use firecracker images for cloudhypervisor. I know that's user error, but it's getting more and more complicated. I don't know a solution to that, but would be nice if we can add some constraints, for example Flintlock services report back what provider they are configured to use, and accept specs for that platform only and reject everything else (just a quick example/idea without thinking too much).

[Callisto13]

Closing.

Revert needed here: #6