Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds Docker auto-scale functionality to enable scale-to-zero behavior for Docker containers, similar to the existing Kubernetes auto-scale feature. The implementation allows mc-router to automatically start/unpause stopped containers when clients connect and gracefully stop containers after a configurable idle period.
Key changes:
- Refactored
ScalerFuncintoWakerFunc(returns endpoint string) andSleeperFuncto support dynamic endpoint resolution after container startup - Implemented Docker container start/stop/unpause operations with health check waiting
- Added per-container auto-scale label overrides (
mc-router.auto-scale-upandmc-router.auto-scale-down)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| server/routes.go | Refactored type signatures from ScalerFunc to WakerFunc/SleeperFunc; added guard to prevent downscaler activation on empty backends |
| server/k8s.go | Updated K8s integration to wrap scale-up function and return endpoint; updated type signatures |
| server/docker_swarm.go | Updated Docker Swarm signatures to match new types (auto-scale not yet implemented for Swarm) |
| server/docker.go | Implemented full Docker auto-scale with container start/stop/unpause, health checking, and route updates; added global state for container tracking |
| server/connector.go | Updated waker invocation to handle returned endpoint and improved error handling/logging |
| server/server.go | Extended downscaler enablement check to include Docker environments |
| server/api_server.go | Updated API route creation to use new empty function names |
| README.md | Added comprehensive documentation for Docker auto-scale feature with examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This is ready for review. I didn't notice #406 is trying to merge about the same functionality. |
|
Thanks. I was about to ask if you were finished addressing the copilot feedback. Don't worry about overlap with the other PR. Yours has a more focused scope that addresses a common and immediate need. The other PR grew a bit broad, which is partly why it is parked there (and because I keep forgetting about it). |
|
Added a fix that showed default asleep motd on nonexistent routes. |
|
Sorry for the delay with me reviewing this. I've set a goal for myself to look over it today. When I skimmed the changes they looked good, but wanted to take a better look while looking on a computer monitor. |
itzg
left a comment
There was a problem hiding this comment.
Great contribution. Just some nitpicky things.
mcproto/types.go
Outdated
| PacketIdLogin = 0x00 // during StateLogin | ||
| PacketIdLegacyServerListPing = 0xFE | ||
| PacketIdStatusRequest = 0x00 | ||
| PacketIdStatusPing = 0x01 |
There was a problem hiding this comment.
Please call this PacketIdPingRequest to match the docs naming convention:
https://minecraft.wiki/w/Java_Edition_protocol/Packets#Ping_Request_(status)
I wasn't able to find a "status ping" when searching the page as is.
mcproto/types.go
Outdated
|
|
||
| // PingPayload represents the status ping payload (packet 0x01) | ||
| type PingPayload struct { | ||
| Value int64 |
There was a problem hiding this comment.
This field name should be Timestamp, right? According to
https://minecraft.wiki/w/Java_Edition_protocol/Packets#Ping_Request_(status)
mcproto/read.go
Outdated
| case PacketIdStatusPing: | ||
| // read 8-byte long from remainder | ||
| if remainder.Len() >= 8 { | ||
| val := int64(binary.BigEndian.Uint64(remainder.Next(8))) |
There was a problem hiding this comment.
Use the existing ReadLong function in this package
mcproto/write.go
Outdated
| // StatusResponse is a minimal structure for the status JSON | ||
| type StatusResponse struct { | ||
| Version struct { | ||
| Name string `json:"name"` | ||
| Protocol int `json:"protocol"` | ||
| } `json:"version"` | ||
| Players struct { | ||
| Max int `json:"max"` | ||
| Online int `json:"online"` | ||
| Sample []struct { | ||
| Name string `json:"name"` | ||
| ID string `json:"id"` | ||
| } `json:"sample,omitempty"` | ||
| } `json:"players"` | ||
| Description map[string]interface{} `json:"description"` | ||
| Favicon string `json:"favicon,omitempty"` | ||
| EnforcesSecureChat *bool `json:"enforcesSecureChat,omitempty"` | ||
| } |
mcproto/write.go
Outdated
| if err := WriteString(&payload, jsonString); err != nil { | ||
| return err | ||
| } | ||
| pkt := buildPacket(0x00, payload.Bytes()) |
There was a problem hiding this comment.
Declare const for the packet ID 0x00 here
mcproto/write.go
Outdated
| var buf [8]byte | ||
| binary.BigEndian.PutUint64(buf[:], uint64(payload)) | ||
| pl.Write(buf[:]) | ||
| pkt := buildPacket(0x01, pl.Bytes()) |
server/connector.go
Outdated
| logrus.WithFields(logrus.Fields{ | ||
| "client": frontendConn.RemoteAddr(), | ||
| "error": err, | ||
| }).Debug("Predefined status: error reading initial status packet") |
server/docker.go
Outdated
| client *client.Client | ||
| config dockerWatcherConfig | ||
| client *client.Client | ||
| dockerRoutesLock sync.RWMutex |
There was a problem hiding this comment.
I'm wondering why the extra mutex? It looks like the mutex field on L64 isn't even used, so perhaps removing that is the cleaner solution.
There was a problem hiding this comment.
Leftover from initial development... whoops
server/docker.go
Outdated
| } | ||
| if key == DockerRouterLabelAutoScaleUp { | ||
| lowerValue := strings.TrimSpace(strings.ToLower(value)) | ||
| data.autoScaleUp = lowerValue != "" && lowerValue != "0" && lowerValue != "false" && lowerValue != "no" |
There was a problem hiding this comment.
It would be great to create a helper function for this and then use the same at
Lines 287 to 291 in 22ec39b
and below at L385
There was a problem hiding this comment.
Used strconv.ParseBool instead of a new helper function.
|
|
|
Disregard that comment. My testing scenario was flawed since I'm running mc-router outside of docker. |
|
Can you take a look at the unit test failure: https://github.com/itzg/mc-router/actions/runs/20274437941/job/58612175899?pr=488 |
|
Tests were failing because some method signatures changed and were used for running k8s tests. I checked all tests now pass and I have also added some Docker compose examples. |
No description provided.