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: implement setting mtu for virtio network device #42

Merged
merged 6 commits into from
Jan 8, 2024

Conversation

lucasl0st
Copy link
Contributor

@lucasl0st lucasl0st commented Dec 17, 2023

For some scenarios it is required to set the MTU of the proxmox VM network interface, especially when running behind a VPN.
This adds a MTU field to the NetworkDevice struct of the ProxmoxMachine CRD.

See https://git.proxmox.com/?p=qemu-server.git;a=commit;h=61a14cde8d568e552d3deaab2da76b479b8aca7b

Sets the MTU value of a network machine network device:
image

@lucasl0st lucasl0st force-pushed the implement-setting-mtu branch from 7b6b2e7 to 3c60c05 Compare December 17, 2023 19:11
@lucasl0st lucasl0st marked this pull request as ready for review December 17, 2023 19:15
internal/service/vmservice/vm.go Show resolved Hide resolved
api/v1alpha1/proxmoxmachine_types.go Show resolved Hide resolved
api/v1alpha1/proxmoxmachine_types.go Show resolved Hide resolved
@lucasl0st lucasl0st force-pushed the implement-setting-mtu branch from 3c60c05 to 9232d60 Compare December 18, 2023 12:32
internal/service/vmservice/utils_test.go Show resolved Hide resolved
internal/service/vmservice/utils.go Outdated Show resolved Hide resolved
internal/service/vmservice/utils.go Outdated Show resolved Hide resolved
@lucasl0st lucasl0st force-pushed the implement-setting-mtu branch 4 times, most recently from 324e238 to 4b5f506 Compare December 18, 2023 15:45
@pborn-ionos
Copy link
Contributor

We should not default to 1500 here. If a user has different requirements and those are already part of his preconfigured template, we're actively breaking stuff. So i argue if it's not explicitly set in the ProxmoxMachine CR, we should keep existing behaviour and not care about it.

@mcbenjemaa
Copy link
Member

@lucasl0st lucasl0st force-pushed the implement-setting-mtu branch from 4b5f506 to 089d4a8 Compare December 23, 2023 07:16
@lucasl0st lucasl0st force-pushed the implement-setting-mtu branch from 089d4a8 to 3fd27ea Compare December 23, 2023 07:21
@lucasl0st
Copy link
Contributor Author

As requested I removed the default MTU and applied the other suggestions, it should have the original behavior now if no MTU is set

@mcbenjemaa
Copy link
Member

Sorry for the delay, I will test this and get back to you.

@mcbenjemaa
Copy link
Member

mcbenjemaa commented Jan 8, 2024

have you executed make test?

it's failing in my machine

@lucasl0st
Copy link
Contributor Author

have you executed make test?

it's failing in my machine

Yes, it is working on my machine (and it is clearly also working in the Github workflow).
Are you using go 1.20? I ran into some issues with 1.21 before when running the tests

Copy link
Member

@mcbenjemaa mcbenjemaa left a comment

Choose a reason for hiding this comment

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

LGTM

thanks for your contribution

@mcbenjemaa mcbenjemaa merged commit e4a915b into ionos-cloud:main Jan 8, 2024
9 checks passed
@lucasl0st lucasl0st deleted the implement-setting-mtu branch January 8, 2024 15:44
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.

3 participants