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

feat: add support for network_device MTU #176

Merged
merged 4 commits into from
Dec 13, 2022
Merged

feat: add support for network_device MTU #176

merged 4 commits into from
Dec 13, 2022

Conversation

abdo-farag
Copy link
Contributor

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #0000

@bpg
Copy link
Owner

bpg commented Dec 8, 2022

Hey @abdo-farag thanks for contributing this! I'll take a closer look at the code later this week.

The disk problem you've mentioned in the comment is interesting though, don't remember seeing this before. I don't think we can keep that if condition commented, as it may break some other use cases...
Would you mind sharing your VM template so I can take a closer look?

@abdo-farag
Copy link
Contributor Author

Hi @bpg, it's a container problem not vm. The template I have used is from project documentation.

resource "proxmox_virtual_environment_container" "ubuntu_container" {
  description = "Managed by Terraform"
  node_name = "pve"
  vm_id     = 1234

  disk {
    datastore_id    = "local"
  }

  
  initialization {
    hostname = "ubuntu-container"

    ip_config {
      ipv4 {
        address = "dhcp"
      }
    }

    user_account {
      keys     = [trimspace(tls_private_key.ubuntu_container_key.public_key_openssh)]
      password = random_password.ubuntu_container_password.result
    }
  }

  network_interface {
    bridge = "vmbr1"
    name = "veth0"
    mtu  = 9000
  }
  
  operating_system {
    template_file_id = proxmox_virtual_environment_file.ubuntu_container_template.id
    type             = "ubuntu"
  }
}

resource "proxmox_virtual_environment_file" "ubuntu_container_template" {
  content_type = "vztmpl"
  datastore_id = "local"
  node_name    = "pve"

  source_file {
    path = "http://download.proxmox.com/images/system/ubuntu-20.04-standard_20.04-1_amd64.tar.gz"
  }
}

resource "random_password" "ubuntu_container_password" {
  length           = 16
  override_special = "_%@"
  special          = true
}

resource "tls_private_key" "ubuntu_container_key" {
  algorithm = "RSA"
  rsa_bits  = 2048
}

output "ubuntu_container_password" {
  value     = random_password.ubuntu_container_password.result
  sensitive = true
}

output "ubuntu_container_private_key" {
  value     = tls_private_key.ubuntu_container_key.private_key_pem
  sensitive = true
}

output "ubuntu_container_public_key" {
  value = tls_private_key.ubuntu_container_key.public_key_openssh
}

Without disk block terrafrom shows this error
Error: received an HTTP 500 response - Reason: storage 'local-lvm' does not exist
with disk block and 'local' as datastore_id terrafrom shows this error.
│ Error: Invalid address to set: []string{"datastore_id"}

used environment

PVE version 7.3-3
Terraform v1.3.4

@bpg
Copy link
Owner

bpg commented Dec 10, 2022

Thanks for the details, I've created a separate ticket for this error #179, a fix is on the way.

// disk[mkResourceVirtualEnvironmentContainerDiskDatastoreID] != dvResourceVirtualEnvironmentContainerDiskDatastoreID {
// err := d.Set(mkResourceVirtualEnvironmentContainerDiskDatastoreID, []interface{}{disk})
// diags = append(diags, diag.FromErr(err)...)
// }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abdo-farag I pushed the fix, could you rebase and uncomment this block to see if it works for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bpg It works!, Thanks

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bpg It works!, Thanks

Great! Could you please put the SSD work in a separate PR, and we'll focus here on MTU?
Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bpg I have created new branch for ssd feature is it better so? I don't know how to split them :)

Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @abdo-farag! The code looks great, it seems to be working fine in my tests as well. I've pushed a small commit to add mtu arguments to the examples.

Could you please add the new MTU parameters to the docs? virtual_environment_container.md and virtual_environment_container.md to complete the PR? It is also worth to mention that MTU can't be greater than the bridge MTU of the PVE.

@bpg bpg changed the title add support for network_device MTU feat: add support for network_device MTU Dec 11, 2022
Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @abdo-farag, I've updated the docs. Also changed default MTU from 1500 -> 0 as I don't think 1500 can be universally applied. The VM / LXC MTU can't be larger than the bridge MTU, and that one potentially can be lower than 1500 in some cases.
Other than that - 🚢 !

@bpg bpg merged commit 3c02cb1 into bpg:main Dec 13, 2022
@ghost ghost mentioned this pull request Dec 13, 2022
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.

2 participants