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

Update legacy facilities and servers #70

Merged
merged 2 commits into from
Apr 22, 2021
Merged

Update legacy facilities and servers #70

merged 2 commits into from
Apr 22, 2021

Conversation

scottilee
Copy link
Contributor

@scottilee scottilee commented Apr 17, 2021

SIG onprem from Kubeflow is using this repo to deploy Equinix Metal servers and Kubernetes. I've found a couple issues when using the code from this repo on a new Equinix Metal account with no hardware reservations or access to ARM servers:

  • "ewr1" is only available through a hardware reservation. To accommodate user accounts without hardware reservations, it would be better to use something like "dc13" instead.
╷
│ Error: Error reserving IP address block: POST https://api.equinix.com/metal/v1/projects/d2bb6d34-9d18-4934-aaa4-a70c144d0864/ips: 422 ewr1 is not a valid facility 
│ 
│   on kubernetes-controller-pool.tf line 31, in resource "metal_reserved_ip_block" "kubernetes":
│   31: resource "metal_reserved_ip_block" "kubernetes" {
  • "c1.small.x86" is a legacy device that uses RAID1. Updating to "c3.small.x86", should work better for a wider range of users.

  • ARM servers are not available by default. To accommodate user accounts without access to ARM servers I would recommend removing all the Terraform code related to ARM or setting the default count to 0 as it will throw an error about ARM not being available. If you do plan on using ARM use "c1.large.arm" instead as that is more up-to-date.

╷
│ Error: You are not allowed to provision c1.large.arm servers
│
│   on modules/node_pool/main.tf line 25, in resource "metal_device" "arm_node":
│   25: resource "metal_device" "arm_node" {
│
╵
╷
│ Error: You are not allowed to provision c1.large.arm servers
│
│   on modules/node_pool/main.tf line 25, in resource "metal_device" "arm_node":
│   25: resource "metal_device" "arm_node" {
│
╵
╷
│ Error: You are not allowed to provision c1.large.arm servers
│
│   on modules/node_pool/main.tf line 25, in resource "metal_device" "arm_node":
│   25: resource "metal_device" "arm_node" {
│
╵

Also, I noticed all of the GPU server types are legacy. What the future plans are for GPU server types?

Originally kubeflow-onprem#1

@jmarhee jmarhee self-requested a review April 17, 2021 17:46
@displague
Copy link
Member

@scottilee these recommendations sound very reasonable and we've been meaning to update our modules with plans and facilities that are generally available.

We recently released metro support, do you think that would be suitable in your environment?
equinix/terraform-provider-metal#55

Are you using the project as a TF module, are there output variables you could benefit from?

@scottilee
Copy link
Contributor Author

We recently released metro support, do you think that would be suitable in your environment?
equinix/terraform-provider-metal#55

"metro" would work but I think updating the default variable for "facility" still makes sense unless you remove it and use just "metro".

Are you using the project as a TF module, are there output variables you could benefit from?

I'm not sure I understand this question but I am using the project as-is for the most part. The output variables seemed fine to me and were helpful as they are now.

@scottilee
Copy link
Contributor Author

If you already have changes coming to fix these things you can close this PR. Otherwise, I'm happy to work with you to update this PR into a state that can be merged.

variables.tf Outdated Show resolved Hide resolved
@displague displague linked an issue Apr 18, 2021 that may be closed by this pull request
@displague
Copy link
Member

displague commented Apr 18, 2021

If you already have changes coming to fix these things you can close this PR. Otherwise, I'm happy to work with you to update this PR into a state that can be merged.

I think this PR is a great start (possible all we need to change) for #71.

Are you using the project as a TF module, are there output variables you could benefit from?

I'm not sure I understand this question but I am using the project as-is for the most part. The output variables seemed fine to me and were helpful as they are now.

See #65. Your fork could instead be a new project that consumes this project as a module. Your project could itself be published as a Terraform module. (with conventional naming for published modules, that might be: terraform-metal-kubeflow).

For example, the EM Anthos on vSphere module consumes the EM vSphere module: https://github.com/equinix/terraform-metal-anthos-on-vsphere/blob/main/01-create-vsphere.tf.

We'd be glad to work with you on incorporating any options you need to make this project/module fit.

@displague
Copy link
Member

"metro" would work but I think updating the default variable for "facility" still makes sense unless you remove it and use just "metro".

I've created #72 to keep track of the metros changes, which would be done in a different PR.

@scottilee
Copy link
Contributor Author

@displague Thanks for the review. I addressed your comment. Let me know if there's anything else.

@displague displague merged commit 1af8fad into equinix:main Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update examples and defaults to use modern slugs
3 participants