This could very well use a harmony id from harmony_types. I like the id format, it is unique, relatively short and contains a timestamp.
Why do we need that? It is yet another dependency? Can't we pass the cloud init any other way when creating the vm? Do we need that for cloud init as I'm assuming or something else?
This is tricky and is actually an architectural problem in harmony. Ensure ready is executed eagerly on a topology, but this topology won't necessarily be running a kvm related workload this run and/or might be doing the kvm setup in an earlier task before calling the kvm dependent scores. I am pretty sure we have a ROADMAP entry on that topic of topology initialization dependency. Add a reference here to the roadmap entry with a TODO so we capture this use case when we get to this work.
This feels straight up wrong. KvmVmScore (sounds generic) depending on CloudInit (specific) is a crime against good architecture and naming.
Use args vec, more readable. Also true for all other Command::new calls.
feels like ansible should have a purpose built module here too. Don't be lazy and call shell commands. Calling shell commands through ansible is completely useless as they're not truly idempotent in the way that file and package installations are.
I have some doubts about this trait.
This comment does not feeld correct. Ensure_package is distribution agnostic. For now we choose to support only debian as this is our first concrete target, but this may change soon and the encapsulation is correct, choosing the correct tool based on the distribution is this function's burden. We might move that to the topology and separate the topologies more granularly between debian an rhel and others but at this moment I think this would be wrong.
I'd like to see here a proper rust struct for the file module config built as proper rust code and simply serialized to the ansible module format instead of a bunch of json! invocations feeling very fragile. We should research if there already exist ansible rust crates. I doubt we will find mature and solid ones but it's worth looking we could be surprised.
The file spec is probably the correct one, all that is lacking is a function to serialize it directly to ansible file module json.
I was not convinced that we should use ansible for the file copy but here for the systemd unit I am sure that we want to use the ansible builtin systemd module https://docs.ansible.com/projects/ansible/latest/collections/ansible/builtin/systemd_module.html
Halfway through the review, many small things and a few bigger things to fix. Overall not terrible. But take the time to step back, understand clearly the code review and revisit the entire p-r with the comments in mind and improve it.
Also many things this does require sudo (like creating a user) which might end up in cloud init for a reason or another, I think it could be misleading to have a single trait with implementations all over the place from cloud init to calling an ansible module to install a package, etc.
This is ugly. Use a long string with format! or askama templates like we do in other places.
Are there alternatives to systemd unit files to make sure it restarts on reboot? What about running the agent itself as a podman container? It would probably have to be privileged but that would reduce the configuration burden on the host and centralize our logic around podman instead of spreading it to systemd.