static TCPUDP support for the container ports #2505
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SaitejaTamma The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Build Failed 😱 Build Id: a36def6d-5de9-4a92-8aa6-e78abfe0029b To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Failure on linting. Gets us all. Run |
|
Build Succeeded 👏 Build Id: 069509d2-0fb7-4216-ba3e-f24aa8a35d49 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
Build Failed 😱 Build Id: 09f3f40d-fd1c-4342-9ba9-ed65e1a4e7b4 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
pkg/gameservers/controller.go
Outdated
| @@ -474,6 +477,35 @@ func (c *Controller) syncGameServerPortAllocationState(ctx context.Context, gs * | |||
| return gs, nil | |||
| } | |||
|
|
|||
| // Allocate gameserver ports ( containerports ) for dynamic protocols such as TCPUDP | |||
| // TODO: Going forward, this needs to be enhanced for other kinds of dynamic protocols | |||
| func handleTCPUDPPortAllocationforStaticPortPolicy(gs *agonesv1.GameServer) { | |||
There was a problem hiding this comment.
I thought we were going to move this logic into GameServer.Pod(...)?
There was a problem hiding this comment.
The logic for TCPUDP port allocation for static port policy is encapsulated in the method “handleTCPUDPPortAllocationforStaticPortPolicy”. This method is invoked from “syncGameServerPortAllocationState”. Since this is an explicit port allocation action this needs to happen before the event “port allocated” is recorded, “ c.recorder.Event(gs, corev1.EventTypeNormal, string(gs.Status.State), "Port allocated") “
There was a problem hiding this comment.
Got it. That makes a certain amount of sense.
I'm not 100% sure, but is there a way to move this duplication of port logic into some kind of centralised function(s) that are shared between the two implementations? Feels like a lot of duplication.
But I can't see it myself, but I thought I'd ask the question?
pkg/gameservers/controller.go
Outdated
| @@ -474,6 +477,35 @@ func (c *Controller) syncGameServerPortAllocationState(ctx context.Context, gs * | |||
| return gs, nil | |||
| } | |||
|
|
|||
| // Allocate gameserver ports ( containerports ) for dynamic protocols such as TCPUDP | |||
| // TODO: Going forward, this needs to be enhanced for other kinds of dynamic protocols | |||
| func handleTCPUDPPortAllocationforStaticPortPolicy(gs *agonesv1.GameServer) { | |||
There was a problem hiding this comment.
Got it. That makes a certain amount of sense.
I'm not 100% sure, but is there a way to move this duplication of port logic into some kind of centralised function(s) that are shared between the two implementations? Feels like a lot of duplication.
But I can't see it myself, but I thought I'd ask the question?
pkg/gameservers/controller.go
Outdated
| @@ -474,6 +477,35 @@ func (c *Controller) syncGameServerPortAllocationState(ctx context.Context, gs * | |||
| return gs, nil | |||
| } | |||
|
|
|||
| // Allocate gameserver ports ( containerports ) for dynamic protocols such as TCPUDP | |||
| // TODO: Going forward, this needs to be enhanced for other kinds of dynamic protocols | |||
There was a problem hiding this comment.
Not sure I follow this TODO? I feel like the comment should just be something like "Creates duplicate ports for handling TCPUDP protocol with static port policies" - or something simple like that? There isn't dynamic protocols in this?
|
Another thought I had this morning, was should we have an e2e test for this so that we check actual connectivity? The only tricky part is choosing a static port that otherwise is not used, we should have a look to see if there are other e2e tests for static ports? (I'm not sure if there are?) |
|
The closest thing I could find is |
Co-authored-by: Mark Mandel <markmandel@google.com>
|
Build Failed 😱 Build Id: 227490ab-a69e-494f-b8c9-d40e4a78c501 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Failed 😱 Build Id: fccea801-37ea-4c39-a130-3bd9d0b7de2c To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Failed 😱 Build Id: 2aa0fb39-53d0-4be6-a55c-d38930975f8b To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
What type of PR is this?
/kind feature
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #2314
Special notes for your reviewer: