-
Notifications
You must be signed in to change notification settings - Fork 59
Enable game server ip address allocation for macvlan driver. #49
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
Conversation
Issue: For macvlan driver default docker config assign first available ip. Game servers get ip assigned in order of container creation. Solution proposal: Wings support only one docker network for each game server. To assign correct IP default allocation could be used. Allocations doesn't limit IPs at all, admin could set desired game server container ip and port by default allocation.
|
This seems fine to me. I don't think it would work yet as our config loading order still first loads the default struct |
|
I can test if it will work with pelican. |
I've been running it for more than a month now without any noteworthy issue. |
The question is does it still work for people that does not use macvlan. |
In short: the proposed change applies only to wings that set The current implementation does not work for an interface of type My proposed fix makes the containers assigned to It is worth remembering that the use of Edit: |
The config logic should fil in all fields that are empty and can not be empty |
|
@madpeteguy Would you happen to have a pelican build with this fix? Was using your pterodactyl build on unraid but want to move over to pelican & was bummed to find out pelican defaults to the default first available ip same as pterodactyl. Would love to see this officially merged into pelican whenever / if possible. Edit: Made my own build here https://hub.docker.com/r/crunch41/pelican-wing hope that's alright, Have only tested on unraid but looks like it works as it should now with specified ip addresses being assigned. Only wish you could change the docker container names now haha |
|
@Crunch41 Sorry, I did not have time to migrate to Pelican yet. |
|
@Crunch41 Thank you for making this image available with the pull changes, and a big thanks to @madpeteguy for making it possible! I’m currently using this build on Unraid after also encountering issues with the way the default egg/container is created and how IPs are assigned sequentially on a macvlan network. So far, everything has worked great on a fresh install. However, when migrating from an existing install to this image, I noticed SQL database errors when deleting existing port allocations and attempting to remove allocations. I’ll do further testing and provide more details to help investigate. |
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.
Pull Request Overview
This PR cherry-picks support for assigning a specific IP from the default allocation when using the macvlan Docker driver, ensuring game servers receive the intended address.
- Imported Docker’s
networktypes and added logic to detectmacvlandriver. - Constructed a
NetworkingConfigusing the server’s default allocation IP and gateway. - Updated both
server/install.goandenvironment/docker/container.goto pass the new config intoContainerCreate.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| server/install.go | Added macvlan network config generation and passed it into the container creation call. |
| environment/docker/container.go | Mirrored macvlan networking logic in the environment’s Create method for consistency. |
Comments suppressed due to low confidence (2)
server/install.go:464
- [nitpick] Consider renaming
defaultMappingto something more descriptive likeipAllocationto improve readability.
var defaultMapping = ip.Server.Config().Allocations.DefaultMapping
environment/docker/container.go:264
- There are no tests covering the newly added macvlan network configuration path; consider adding unit tests to verify correct behavior.
var netConf *network.NetworkingConfig = nil //In case when no networking config is needed set nil
| }() | ||
|
|
||
| r, err := ip.client.ContainerCreate(ctx, conf, hostConf, nil, nil, ip.Server.ID()+"_installer") | ||
| var netConf *network.NetworkingConfig = nil //In case when no networking config is needed set nil |
Copilot
AI
Jun 26, 2025
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.
You can simplify the nil initialization by declaring var netConf *network.NetworkingConfig without explicitly assigning nil.
| var netConf *network.NetworkingConfig = nil //In case when no networking config is needed set nil | |
| var netConf *network.NetworkingConfig // In case when no networking config is needed, defaults to nil |
| var serverNetConfig = config.Get().Docker.Network | ||
| if "macvlan" == serverNetConfig.Driver { //Generate networking config for macvlan driver | ||
| var defaultMapping = ip.Server.Config().Allocations.DefaultMapping | ||
| ip.Server.Log().Debug("Set macvlan " + serverNetConfig.Name + " IP to " + defaultMapping.Ip) |
Copilot
AI
Jun 26, 2025
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.
[nitpick] Use formatted logging (e.g., Debugf) instead of string concatenation for better readability and performance.
| ip.Server.Log().Debug("Set macvlan " + serverNetConfig.Name + " IP to " + defaultMapping.Ip) | |
| ip.Server.Log().Debugf("Set macvlan %s IP to %s", serverNetConfig.Name, defaultMapping.Ip) |
parkervcp
left a comment
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.
LGTM
This is pterodactyl/wings#203 cherrypicked onto current main branch.
Tagging @madpeteguy for notice.