Add allocator service example and documentation #353
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Build Failed 😱 Build Id: ac691d89-3c36-4ad9-8069-46ef6fa969ab Build Logs |
|
Build Failed 😱 Build Id: 83c46baf-0361-4f69-bb7e-7cef6676a244 Build Logs |
|
Build Failed 😱 Build Id: 08f678a3-c0e4-4c39-99cf-3db72aca272b Build Logs |
|
Build Failed 😱 Build Id: 651fc6a5-a4d3-4a06-b7ac-fc5abf7e3936 Build Logs |
|
Build Failed 😱 Build Id: d195f5ab-14f1-4a05-8589-72d13880cfba Build Logs |
1 similar comment
|
Build Failed 😱 Build Id: d195f5ab-14f1-4a05-8589-72d13880cfba Build Logs |
|
Build Failed 😱 Build Id: 729097cf-8792-45d9-8d52-4c0f610d1c3f Build Logs |
|
Any idea what I need to change to resolve this build error? |
|
Looks like the e2e tests where being a bit flakey, cleaned out the test cluster, and hit re-run for you. |
|
Build Succeeded 👏 Build Id: 53ea0682-b303-4c35-a02b-77001066c59e The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
|
Thanks! |
|
Sorry for the delay in reviewing this - traveling at the moment. Will try and get to this soon! |
markmandel
reviewed
Sep 14, 2018
There was a problem hiding this comment.
Hey! First of all, thanks so much for going through all this effort!!!
I added lost of comments within, but I think it basically boils down to that I think we could strip this example back by a lot, and it will make the concepts that we would are trying convey much easier to understand without the extra things around it, but the core of it is great, and we should definitely have this as an example.
| @@ -0,0 +1,16 @@ | |||
| # Create a single service Ingress resource | |||
There was a problem hiding this comment.
Should we keep this simple, and just use a Service with type: LoadBalancer -- Remove the need for an Ingress?
Just trying to focus on exactly what we want the example to focus on, without anything extra complex. WDYT?
There was a problem hiding this comment.
I thought about using only a Load Balancer, but went with the Ingress because it seems easier to use TLS. It also seems easier to piont a subdomain at the ip, and allows for more flexibility in the future.
I think the focus should be on how to safely allocate a gameserver from a fleet. I'll try and edit out as much complexity as possible.
There was a problem hiding this comment.
I was thinking about this more - and I'm going to come back to this.
I feel strongly we should remove the Ingress piece here, it's too complicated, could intimidate new users, and doesn't add anything to the example.
I feel like this example is meant to show how to do a fleet allocation through the Kubernetes API. The rest is just implementation details - so the simpler the service the better. So remove Ingress, and remove the SSL. (I could even advocate for removal of the token auth -- but would leave a note about how this service is totally open, and point to article on locking down services).
We could definitely add to the "Next Steps" section, a link to existing documentation on how to setup ingress and/or a ssl cert, but I don't think we need this here.
In practice I feel like a user is going to integrate with a matchmaker, and not setup a direct API call like we have here anyway. And setting up a matchmaker is outside the scope of this project.
| @@ -0,0 +1,11 @@ | |||
| FROM alpine:3.7 | |||
There was a problem hiding this comment.
We could use a multi stage docker build to make creating this make easier. WDYT?
There was a problem hiding this comment.
I think that would be great and will make those changes.
| secretName: allocatorw3secret | ||
| containers: | ||
| - name: fleet-allocator | ||
| image: index.docker.io/trota/allocator-service:simple-udp-0.5.0-03f4866-5 |
There was a problem hiding this comment.
Do we have to provide an image here? I think the code is actually biggest part of this. We could just leave it as {insert image name here} ? with clear instructions on how to build it? (Especially if we make below multi-stage)
There was a problem hiding this comment.
Yes, providing instructions on how to build it would definitely be better. I imagine most of the audience will be comfortable hosting their own image in their own repository.
I'll make these changes.
examples/allocator-service/main.go
Outdated
|
|
||
| // Main will set up an http server, fetch the ip and port of the allocated | ||
| // gameserver set, and return json a string of address:port | ||
| func main() { |
There was a problem hiding this comment.
I would put this first in the file - it sets the context, and you then head into each handler from there. Makes understanding what is happening in here easier. WDYT?
There was a problem hiding this comment.
Sounds great. For some reason, maybe Python, I'm always leaving main until last!
I'll reorder the file so that main is first.
docs/create_allocator_service.md
Outdated
| @@ -0,0 +1,435 @@ | |||
| # Quickstart Create an Allocator Service | |||
|
|
|||
| This quickstart describes one way to implement a service which will allocate a Game Server on demand using the Agones API. This allows a game client or matchmaker service to call the allocator service in order to allocate a Game Server and retrieve its IP address and port number. The game client can then connect directly to the Game Server, so that the minimized latency that is in the Agones design is preserved. | |||
There was a problem hiding this comment.
Personally, i think there are chunks of this that are overkill - I don't think we need to explain Kubernetes - but we can totally link to references that may be useful to understand concepts they may not be familiar with.
That being said - love and appreciate the user empathy in including it, but I think users may find this overwhelming.
There was a problem hiding this comment.
I'll reduce the amount of text, and focus on linking to documentation over explanation.
Do you have any data on the user base? Having a clearer understanding of the audience would make writing more targeted documentation easier. I've been imagining the audience to be the same as that for kubernetes or gke documention, and am trying to replicate the level of detail I see in their docs.
There was a problem hiding this comment.
I think that's valid - it's for developers for sure - so expect technical expertise.
I think we can assume some K8s knowledge, but should link to docs so that people who aren't that familiar have a reference to go look at.
docs/create_allocator_service.md
Outdated
|
|
||
|
|
||
| ### 11. Allocate a Game Server | ||
| The service uses Basic Auth to provide some security as to who can allocate gameserver resources. A generated key is in main.go that you can change. |
There was a problem hiding this comment.
Just a thought - why is creating this service better than just calling the K8s API directly? It has security built in, etc?
Maybe we need a context statement somewhere, that this code would ideally be part of a much larger system, such as a matchmaker.
There was a problem hiding this comment.
I think the service approach is preferable to calling k8s directly because it creates a separation of concerns, helps protect the game server from being compromised, and abstracts some game server logic from the code that gets deployed with the game client. Also, being able to refer to a subdomain.domain.ext when creating connections between client and server provides flexibility.
Yes, describing how this service would work as one component in a larger system would be great. Once horizontal scaling is sorted out, I mean the automatic creation of Ready game servers, it will make more sense. I will add a sentence about context, and hopefully we can write a separate document in the future that is a complete guide to managing the allocation of gameservers while preserving n gameservers in a ready state.
docs/create_allocator_service.md
Outdated
| ``` | ||
|
|
||
|
|
||
| ### 13. Create an A Record |
There was a problem hiding this comment.
As per your comments - let's not worry about this.
There was a problem hiding this comment.
Ok I will edit this out.
docs/create_allocator_service.md
Outdated
| Now https://allocator-service.yourdomain.ext should return the IP address and port of the allocated Game Server. | ||
|
|
||
|
|
||
| ### 14. Customize |
There was a problem hiding this comment.
We could do this as a "next steps" section, i.e.
Things you could do next
- Change the fleetname
- Add a SSL certificate
- Change the HTTP password
I don't think we need to explain how Docker works either. This would also be simpler witha multi-stage build
There was a problem hiding this comment.
That's a nice way of compressing those last several steps. I'll make a next steps section while purging the detailed content.
docs/create_allocator_service.md
Outdated
|
|
||
| The [Gin Web Framework](https://github.com/gin-gonic/gin/) | ||
|
|
||
| The Kubernetes documentation on [Services](https://kubernetes.io/docs/concepts/services-networking/service/) |
There was a problem hiding this comment.
We could link this above, as we introduce each of these concepts.
There was a problem hiding this comment.
Sounds good. I'll add inline links while reducing explanatory text.
Thanks for the review!
markmandel
added
kind/documentation
|
Just a auggestion. Right now this service doesn't add very much value imo because everything can already be done directly with K8s. How about adding some multi-cluster support? For example define a cluster tree and an allocation strategy - balance allocation between clusters or allocate in order (first in cluster 1, when that is full use cluster 2 and so on) ? On top of this, you could also chose the cluster based on client location... |
|
@victor-prodan that almost feels like a feature - I think this is meant to be a simple example of how you can write code against the K8s api to "do a thing" - nothing more than that. Do you agree @slartibaartfast ? |
|
Yes, this is a "do a thing" example to expand the documentation. As interesting as addressing multi cluster support and defining an allocation strategy would be, I think that would be out of scope. Maybe those could be documented in a separate, similar way in the future? For this PR, I am planning on reducing complexity and culling unnecessary steps from the text. |
|
Build Failed 😱 Build Id: ee08ee32-33e2-4819-a2c3-94d0bd88a0c5 Build Logs |
|
Build Failed 😱 Build Id: 975730ab-56bd-49ba-85bc-f12d64ce6ea5 Build Logs |
|
Should I use a different logger? |
|
The lint step is failing, you can repro this on your side by running |
|
Check the bottom of the logs: Need to run goimports over the code. |
|
Build Succeeded 👏 Build Id: c7c1bc13-9d4f-43b5-9115-214d727bb35c The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
|
Thanks for the tips guys. I will keep them in mind in the future! |
|
Sorry I've been spotty in my review of this. Having a quick pass through, this looks good to go 👍 - only one more thing. Please rebase this down to a single commit, and I'll hit the approve button! 💃 |
|
No problem - I appreciate the review. I'll rebase it down to a single commit soon. Today I've been adding a few lines to check the GameServerSet for a ready GameServer before creating a FleetAllocation if that is ok. |
|
Curious to know why you need to interact with the GameServerSet - honestly, i never envisioned that a user would really need to know about GameServerSets - it's more of an implementation detail. What's the need there? Is there information on a GSS that isn't available on a Fleet? |
|
I was looping through the fleets game server set list, counting the number of gsSet.Status.ReadyReplicas. Now I see that I can just get that from FleetStatus.ReadyReplicas, and will use that value instead. |
|
We need the clone/fork so that the reader has a context within which to work. |
|
@slartibaartfast but it's not actually used in the example. Why add the extra step? It's definitely not a prerequisite, as it's not actually used. I'm trying to remove barriers to entry here for new developers - make things as simple as possible for them. |
|
@markmandel the only reason pull/fork seems necessary to me is that the reader edits allocator-service.yaml to point to the image they created in the first step. Other than that, all of the command text could be edited to reference hosted files rather than local ones. |
|
@slartibaartfast we could also just grab the raw download straight from Github rather than force a full clone of the repo for a couple of files. |
|
@markmandel I'll remove the clone/fork requirement and edit the text to reference hosted rather than local files, and have the user download and edit allocator-service.yaml after they build and push their image in step 1. |
markmandel
reviewed
Oct 2, 2018
docs/create_allocator_service.md
Outdated
| @@ -0,0 +1,335 @@ | |||
| # Quickstart Create an Allocator Service | |||
There was a problem hiding this comment.
I'm feeling that this isn't actually a quickstart - it should be more of a guide - "How to Create A Fleet Allocation via the Kubernetes API"
Because this is actually what we are showing here, the service aspect is more of an implementation detail in my mind - what we really want to show is "how do you create a fleet allocation through the API".
WDYT?
There was a problem hiding this comment.
Yes, I had the same question regarding is it a guide or a quickstart several weeks ago. Looking at other documents in this repo in a general way, Quickstarts seem to leave the reader with something tangible, whereas Guides seem to be more descriptions of things that exist.
If showing code that creates a fleet allocation is the goal, then all you would need is something like the go snippet in access_api.md. Lines 159 - 182 of main.go in this pr could suffice. While that would be more helpful than no code snippet at all, I think it would leave some readers wondering what to do with the snippet.
The service aspect is definitely an implementation detail, but one that seems necessary if the user is to easily see their code snippet in action. It moves us from a review of how one could theoretically perform a fleet allocation programmatically to actually doing so. It may also generate some useful discussion about calling the API.
I think having a working example of how to call the API is useful. If a reader finds this to be too complicated, they are free to consider alternative solutions, having gained a bit of knowledge of one way to use the API. Reading through the slack channel, it's a question that comes up and having some docs that address the idea seems helpful to me.
| @@ -1,11 +1,12 @@ | |||
| # Getting Started | |||
| # Quickstart Edit a Game Server | |||
There was a problem hiding this comment.
In the future - please keep edits like this in separate PRs. Something like this would be quick to push through, but this PR is being held up by other things.
There was a problem hiding this comment.
Definitely. I will do so in the future.
| @@ -0,0 +1,16 @@ | |||
| # Create a single service Ingress resource | |||
There was a problem hiding this comment.
I was thinking about this more - and I'm going to come back to this.
I feel strongly we should remove the Ingress piece here, it's too complicated, could intimidate new users, and doesn't add anything to the example.
I feel like this example is meant to show how to do a fleet allocation through the Kubernetes API. The rest is just implementation details - so the simpler the service the better. So remove Ingress, and remove the SSL. (I could even advocate for removal of the token auth -- but would leave a note about how this service is totally open, and point to article on locking down services).
We could definitely add to the "Next Steps" section, a link to existing documentation on how to setup ingress and/or a ssl cert, but I don't think we need this here.
In practice I feel like a user is going to integrate with a matchmaker, and not setup a direct API call like we have here anyway. And setting up a matchmaker is outside the scope of this project.
|
What if we add Tutorials to the types of documentation available? They could address topics which are more implementation oriented, such as this service. Other tutorials could address things like integrating with specific game engines, or how to use Agones with Open Match. |
|
SGTM! I like the approach. Let's do that! |
|
@markmandel @slartibaartfast Thanks, guys for this service. I have been trying to get this running but troubleshooting an issue. Any help is greatly appreciated. What I have noticed here is service throwing following error (HTTP panic) when trying to allocate a GS. After trying to pin point where the issue is, I think it's happening in line 195 of allcoate(). I log the contents of newFleetAllocation variable to confirm. It's returning a JSON message from GS that has status field empty causing http panic in the code.
When I tried with the CURL command for directly accessing API, I get the correct JSON message. Response: I am trying to figure out what may be causing status {) block to empty when tried form the go code. |
|
@santhh Thanks for giving the service a try. I'll be working on it this weekend and will try and reproduce this behaviour. |
|
@slartibaartfast Thank you! I am running in GKE. I created the image locally and pushed it to GCR. I ran into some compile issue for package dependency but used glide to solve it as described Some other things I noticed while I troubleshoot:
|
|
@santhh what Agones version are you using? There was an bug in 0.4 with this behavior fixed by @markmandel.... |
|
@victor-prodan I cloned from master but updated yaml file for 0.4 image as there was no image available for 0.5 or 0.5 rc. Is there a snapshot i can use? |
|
@santhh Yes, in this case you can use the images built for PRs. Example: |
|
@victor-prodan @slartibaartfast @markmandel Thanks guys! it worked. I will do some load testing and let you guys know how it goes with fleet auto scaler |
|
Build Failed 😱 Build Id: 9f4797cd-f0ee-4e60-bedc-a5028576b1af Build Logs |
|
Looks like a flaky test. Restarting. |
|
Build Failed 😱 Build Id: 773f38e5-30cd-4786-a50b-28e6e14da566 Build Logs |
|
Thanks for the restart. Please let me know if there is something I need to change on my end. There aren't many changes in this push. I rewrote some of the text so that it is more focused on using the API as the goal, and added more comments to main.go so that is hopefully more Tutorial style. |
|
Build Failed 😱 Build Id: 62504532-c2b7-43b7-9967-232355f49075 Build Logs |
|
Build Failed 😱 Build Id: f464f7b2-c05f-4c2e-beab-720f909f99df Build Logs |
|
For the previous two build failures, the first seems to fail while running fleetautoscaler test while the second looks like it had a timeout while trying to push the controller image. |
|
Build Succeeded 👏 Build Id: b1bde8b3-091b-44bd-8b8e-19513f3e708b The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a service that allocates a game server from a fleet using the Agones API. It then returns the IP address and port of the allocated game server. I cleaned up the notes I took while making it, and maybe they might provide some useful documentation for people who are looking to programmatically allocate game servers.
Some things I'm not sure about:
Please let me know if this would be useful, and if so, what changes should be made.