Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add hardware reservation id support #134

Closed
wants to merge 1 commit into from
Closed

add hardware reservation id support #134

wants to merge 1 commit into from

Conversation

jhead-slg
Copy link
Contributor

Fixes #133

@gianarb
Copy link
Contributor

gianarb commented Jun 23, 2020

Hello! Thank you for your PR! I am not sure I would like to expose the possibility to set that parameter from the outside.
It works when using a Machine but It does not look a reliable approach when using a machine template and machine deployment (they work like ReplicaSet for pods) that is probably the thing everyone will use at the end because it manages reconciliation and scaling.

I prefer to hide this capability behind a different field like: strategy=ondemand|reserved|spot and the reservation ID can be retrieved via API, in this way the reconciliation will stay more flexible.

The open question that I have is: what happens when there are not reserved ID available? Should we spin up an on-demand one or wait until a reserved ID becomes available?

@gianarb gianarb self-requested a review June 23, 2020 11:13
@gianarb gianarb self-assigned this Jun 23, 2020
@gianarb gianarb added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 23, 2020
@deitch
Copy link
Contributor

deitch commented Jun 23, 2020

Hey @jhead-slg , thanks for this. We should get this in, so let's figure out the API?

We know two ways to allocate machines in cluster-api:

  • Machine - specify actual specific machines
  • MachineDeployment - template

Multi-master and scaling (both master and worker) only work with MachineDeployment, so we would need a way to support it there.

I think you are addressing the first one, while @gianarb is looking at the second, hence the disconnect.

We could go down the single Machine reservation ID path based on this PR, and then have a separate PR to add support for MachineDeployment.

Actually, I like that. I am going to open an issue to add support for reserved instances for MachineDeployment, while Machine can be tied to a single reservation ID.

We cannot quite merge this PR in because:

  • it needs docs
  • we need to be explicit about failure - to answer your question @gianarb, the way Packet usually works is, if I ask to deploy a machine and specify a reservation ID, and it fails, then the request fails. We should stick to that.
  • we need to add the API code to use it
  • we need to regenerate

One other weakness is that clusterctl cluster > output.yaml won't work, as it doesn't have provisions for optional fields, e.g. if env var RESID is set then add the field, else not. It doesn't even have support for defaults yet.

Still, this is a good path to go down, and we can base it on this.

@gianarb
Copy link
Contributor

gianarb commented Jun 23, 2020

I like the idea to avoid possible but not that scalable approaches if possible.

There is not enough context for me to evaluate if there is a problem that can be solved only treating those reservation IDs as pets. I am all in about this approach if I can see this use case.

Anyway, it is the same, I will be very happy to implement only the workflow that looks scalable and reliable. But overall I won't make any more friction about this topic moving forward, I am just happy to point it out 👍

@gianarb gianarb requested review from deitch and removed request for gianarb June 23, 2020 13:57
@gianarb gianarb closed this in e567596 Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for single device reservation IDs
3 participants