-
-
Notifications
You must be signed in to change notification settings - Fork 138
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
Conversation
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... |
Hi @bpg, it's a container problem not vm. The template I have used is from project documentation.
Without disk block terrafrom shows this error used environment
|
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)...) | ||
// } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bpg It works!, Thanks
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 :)
853bc7c
to
ad2c618
Compare
There was a problem hiding this 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.
There was a problem hiding this 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 - 🚢 !
Community Note
Relates OR Closes #0000