Add the ability to spin up Hetzner servers from custom snapshots

[marvinpinto]

This comes in handy when using tools such as Packer to generate customized images.

I have a working docker image built off this feature and I've not run into issues thus far. The image below is linked from my autoscaler fork.

docker pull ghcr.io/marvinpinto/autoscaler:ba42c328

[k8s-ci-robot]

Welcome @marvinpinto!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[mwielgus]

cc: @LKaemmerling

[LKaemmerling]

Hey @marvinpinto,

thank you for your contribution. I would suggest using the ID of the image instead of the description. The description is not unique and can be changed really fast. The ID is stable.

Another point:
You changed the vendored hcloud-go, this should be avoided because those changes will be overwritten when the hcloud-go is updated here. I would suggest you implement your feature in the manager using the hcloud-go List/All method for images within the Manager https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/cloudprovider/hetzner/hetzner_manager.go#L47. This could also be done via labels (& label selectors)

hcloud-go All Method with Filtering: https://pkg.go.dev/github.com/hetznercloud/hcloud-go/hcloud#ImageClient.AllWithOpts

[marvinpinto]

Hey @LKaemmerling, thank you for taking time to review this!

You changed the vendored hcloud-go, this should be avoided because those changes will be overwritten when the hcloud-go is updated here. I would suggest you implement your feature in the manager using the hcloud-go List/All method for images within the Manager

Understood, I will move all logic out of hcloud-go 👍

I would suggest using the ID of the image instead of the description. The description is not unique and can be changed really fast. The ID is stable.

The original lookup for an image was done purely using the image name, e.g. "ubuntu-20.04". This of course works well for pre-baked images but not so well for custom snapshots because it seems that snapshots don't get a name attribute, I verified this in the API.

In the lookup function I modified, it first attempts to look up an image by its ID (e.g. if someone supplies an int), then if it doesn't find anything it attempts a lookup using the name attribute (e.g. "ubuntu-20.04"), and finally if it still doesn't find anything it attempts a lookup using the description attribute. I kept it this way so it would not be a breaking change.

In the final case, it returns the most recent (custom snapshot) image, which is useful in scenarios where these images are churned regularly and one only wishes to use the newest.

Do you still have concerns about using the description attribute?

[LKaemmerling]

Hey @LKaemmerling, thank you for taking time to review this!

You changed the vendored hcloud-go, this should be avoided because those changes will be overwritten when the hcloud-go is updated here. I would suggest you implement your feature in the manager using the hcloud-go List/All method for images within the Manager

Understood, I will move all logic out of hcloud-go 👍

I would suggest using the ID of the image instead of the description. The description is not unique and can be changed really fast. The ID is stable.

The original lookup for an image was done purely using the image name, e.g. "ubuntu-20.04". This of course works well for pre-baked images but not so well for custom snapshots because it seems that snapshots don't get a name attribute, I verified this in the API.

In the lookup function I modified, it first attempts to look up an image by its ID (e.g. if someone supplies an int), then if it doesn't find anything it attempts a lookup using the name attribute (e.g. "ubuntu-20.04"), and finally if it still doesn't find anything it attempts a lookup using the description attribute. I kept it this way so it would not be a breaking change.

In the final case, it returns the most recent (custom snapshot) image, which is useful in scenarios where these images are churned regularly and one only wishes to use the newest.

Do you still have concerns about using the description attribute?

The description is basically just a description (this sentence should get an Oscar!). You should not query against it as it may change easily. For your use case I would suggest using labels on the snapshot and then a label selector as a way to query all images with this selector. Ideally this should be only one image, but if there are more images with a specific label you can use the most recent one.

So as I said using the description is not recommended.

[LKaemmerling]

What needs to be taken into account as well is that every request you send against the API counts towards your Ratelimit. So you shouldn't to this on every node creation (because this might be a problem when you need a lot of nodes in a short time)

[marvinpinto]

For your use case I would suggest using labels on the snapshot and then a label selector as a way to query all images with this selector

Sounds good to me 👍

What needs to be taken into account as well is that every request you send against the API counts towards your Ratelimit

I was thinking the logic to retrieve the image ID could go somewhere in this block. Which I believe would only run on startup and use the "found" image ID until the autoscaler is restarted.

Does this sound reasonable to you, or would you prefer I implement caching + periodic updating (possibly via the Refresh mechanism)?

[LKaemmerling]

Does this sound reasonable to you, or would you prefer I implement caching + periodic updating (possibly via the Refresh mechanism)?

Personally from a hcloud provider perspective I'm fine with both 😀 but I using the on startup is fine, as the images shouldn't change that often

[marvinpinto]

I've rebased this PR against master and updated it implementing all of @LKaemmerling's suggestions. Let me know if I missed anything!

My updated docker image is available at:

docker pull ghcr.io/marvinpinto/autoscaler:d234dc55

[LKaemmerling]

I've rebased this PR against master and updated it implementing all of @LKaemmerling's suggestions. Let me know if I missed anything!

My updated docker image is available at:

docker pull ghcr.io/marvinpinto/autoscaler:d234dc55

Hey @marvinpinto,

thank you this looks good, I just have a few smaller minor things but than from my side everything is ready :) Good job!

[marvinpinto]

Thank you again for taking the time @LKaemmerling, really appreciate it 😊

[LKaemmerling]

Hey @marvinpinto,

this looks good to me! Great job :)

[mwielgus]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LKaemmerling, marvinpinto, mwielgus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• cluster-autoscaler/OWNERS [mwielgus]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment