Add functionality to tag Root Volume

[Pidu2]

Summary

• Add rootVolumeTags field to Machine Spec which allows to set tags on the CloudScale VM Root Volume
• There are no breaking changes

Checklist

• Categorize the PR by setting a good title and adding one of the labels:
  bug, enhancement, documentation, change, breaking, dependency
  as they show up in the changelog
• Update tests.

[tiagodcc]

There is a side effect associated with this design. If Create() fails after creating the server but before tagging the root volume, the following happens:

• The Create method returns an error.
• The controller retries.
• It calls Exists(), which now returns true (because the server was created).
• The controller calls Update() instead of Create().
• The Update() method does not contain the logic to tag the root volume.
  In this case, the machine becomes "Ready", but the root volume tags are missing.

I think that these thoughts are relevant for the following issue and that they need to be addressed there instead: #45

[bastjan]

Thank you for the PR @Pidu2 and @tiagodcc. I will review it next week.

[bastjan]

Thanks for the PR 😊

Code looks good. I'll check with Cloudscale how we reliably detect root volumes.

[bastjan]

I think that these thoughts are relevant for the following issue and that they need to be addressed there instead: #45

I'm not entirely sure if I agree.

Server groups are a "floating" object. Their lifecycle is completely outside of the lifecycle of a server. The next created machine will just fill up the first server group with capacity. Idempotency is just not very important there.

This fix could be to snapshot the labels at server creation time (as metadata appended to the server?) and then apply them in the update method if required.

[tiagodcc]

Server groups are a "floating" object. Their lifecycle is completely outside of the lifecycle of a server. The next created machine will just fill up the first server group with capacity. Idempotency is just not very important there.

I can see your point here and agree that idempotency is not very important in this case.

However, a server group can only contain 4 servers at max. For this reason, this MachineAPI provider correctly creates new server groups in case a group is full. From my point of view, this server group should have the same lifecycle as the servers associated with it. In a very unlikely scenario, where one scales down multiple instances, server groups that aren't actively needed either won't be deleted or must be deleted manually.

This fix could be to snapshot the labels at server creation time (as metadata appended to the server?) and then apply them in the update method if required.

Have I understood it correctly that you want to save the root volume tags in the Machine object and then reapply them in the Update method?
This sounds like a reasonable fix in that case.

[bastjan]

Have I understood it correctly that you want to save the root volume tags in the Machine object and then reapply them in the Update method? This sounds like a reasonable fix in that case.

Yes. I'm just not yet sure where to save the tags. Probably we could just use the spec field. Those tags would then be the first and only parameter that is updated by the controller, which can be seen as inconsistent.

Another possibility would be to just attach the tags as metadata to the cloudscale server and sync them this way. Or add them to the status field, which might end up being a weirdly named field tho.

My preference is just syncing the spec field.

[bastjan]

As we're not using root volume tags in production i'd be fine merging the PR, creating a release, and create a follow up issue specifically for the possible root volume tag inconsistency. Or (I or you) can try to come up with a fix in this PR. What do you think?

[Pidu2]

IMO using spec is fine. In that case we should probably update both tags (server and root) to keep it consistent at least for tagging. When looking at the AWS Controller, they actually seem to do a similar thing (only update tags on running instance): https://github.com/openshift/machine-api-provider-aws/blob/main/pkg/actuators/machine/reconciler.go#L195.

As we will not be needing the functionality immediately, we can include a fix within this PR and wait before doing another release.

I will be on vacation after tomorrow - let's see if I can come up with a fix before that. Potentially Tiago has some time next week, otherwise we can do it next year. :)

[bastjan]

we should probably update both tags (server and root)

I agree that would be my favourite.

[Pidu2]

• Added a function that builds the server tags to keep this consistent between Update and Create
• Added functionality to set tags for server and root volume during Update
• Adjusted Update Test-Case to reflect changes

[simu]

LGTM 🚀

[bastjan]

Released in v0.4.0.