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

Proposal: Split up the api group stable.agones.dev #703

Closed
markmandel opened this issue Apr 12, 2019 · 14 comments · Fixed by #937

Comments

@markmandel
Copy link
Collaborator

commented Apr 12, 2019

This has been rolling around in my head for a while, so wanted to throw it out there.

Basically watching the changes coming to allocation (#682), and how that will have to be in a different group, and then the multicluster work (#698) feels like it should be separated in someway from everything else - I'm wondering if having stable.agones.dev as the core group was a less than ideal decision.

If we made this change, I a sacrificial draft of grouping could be:

core.agones.dev:

  • GameServers
  • Fleets
  • GameServerSets

scaling.agones.dev

  • FleetAutoscaling

allocation.agones.dev

  • GameServerAllocation

multicluster.agones.dev

  • AllocationPolicy (or whatever it ends up being called)

(FleetAllocation would probably have to stay in core, until it goes away, just because you can't mix CRDs and APIServices)

Looking at Kubernetes - they also do something similar.

This is obviously a breaking change - so wanted to run it past everyone, see how people felt.

@markmandel

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 12, 2019

@roberthbailey

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

There is a great comment here from Clayton about what an apigroup should encompass (I'll inline the important bits so people don't get side tracked by that issue):

historically, a Kube API group is intended to roughly encompass a lifecycle. That lifecycle has sometimes been SIG defined (autoscaling) but has also been related to orthogonal features (batch and v2beta1 being created for cronjob specifically, or the split between the various parts of what sig-auth controls). When boundaries can exist between lifecycles, we strongly recommend those be created. Federation v1 is a cautionary tale around coupling too closely between "high level API" (the workloads) and the low level api (the workload APIs on each cluster) - we erred a bit too far on putting everything all together, while federation v2 has focused on a much lower level primitive succeeding.

I think that the lifecycles for the pieces you listed above will likely be separable. The "core" bits will probably stabilize and not change much as we iterate on allocations and especially multicluster support.

Is there historical context for why stable.agones.dev was initially chosen vs just agones.dev? Was the idea to have both stable and unstable api groups? Why different groups instead of just different api versions within the same group?

Note that you can also have apigroups that have names that look nested (because they are different strings). For example, if you run kubectl api-resources -o wide against an installed cluster you will see that there is a group authorization.k8s.io with resources like subjectaccessreviews but that there is also a group rbac.authorization.k8s.io with role and rolebindings defined. The naming structure expresses the relationship - rbac is part of authorization.

Which makes me wonder why use the core prefix at all? What if the core resource types were in an agones.dev apigroup and the other types are put into "subgroups"?

In core k8s, horizontal pod autoscaling is in an apigroup is called autoscaling (which would probably be autoscaling.k8s.io if it was named today) so I think it makes sense to put FleetAutoscaling into autoscaling.agones.dev so that the names align.

@markmandel

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 13, 2019

Thanks for the detailed comments @roberthbailey ! That is useful knowledge!

Is there historical context for why stable.agones.dev was initially chosen vs just agones.dev? Was the idea to have both stable and unstable api groups? Why different groups instead of just different api versions within the same group?

Honestly, I saw that the sample controller had a 3 level group, samplecontroller.k8s.io and I kind of arbitrarily chose stable.agones.dev since I needed to move forward, and there wasn't much literature out there at the time in regards to choosing names 🤷‍♂

Now that we have more experience - i figured it may be time to revisit.

Regarding all your points - I wasn't even aware you could have a two level group, such as agones.dev - that sounds super nice. Also re: scaling vs autoscaling - I couldn't find a reference, so picked a word. I like autoscaling better too 👍 especially if it's inline with k8s.

So to revisit then:

agones.dev:

  • GameServers
  • Fleets
  • GameServerSets
  • FleetAllocations (until they get removed in favour of GameServerAllocations)

autoscaling.agones.dev

  • FleetAutoscaling

allocation.agones.dev

  • GameServerAllocation

multicluster.agones.dev

  • AllocationPolicy (or whatever it ends up being called)

That reads really nicely to me 👍 what does everyone else think?

@roberthbailey

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Given the parenthetical statement about FleetAllocations, would it make sense to put them into the allocation.agones.dev group instead (so that they can be deprecated there leaving the agones.dev group more stable)?

@markmandel

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 15, 2019

Ideally - would totally love to. However, given technical constraints, allocation.agones.dev is controlled by an APIService (so we don't have to store the CRD results, and give us more control over the throughput), whereas FleetAllocations are still controlled by CRDs -- which are two approaches which can't be mixed.

We could move FleetAllocations to be no-storage APIService as well, so that they are in the same group but since they will be going away, ,it doesn't seem worth the effort to me?

@roberthbailey

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

If we update the api group name, we should also update other places that use the same string, such as the prefix used in the SDK for labels and annotations: https://agones.dev/site/docs/guides/client-sdks/#setlabel-key-value

@markmandel

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 23, 2019

Sounds like we have pretty decent consensus on this. I'm going to add this to the next milestone (and essentially the 1.0 roadmap -- as I think we need to complete any CRD changes before we go 1.0 and declare stability.

Please let me know if anyone has objections!

@markmandel markmandel added this to the 0.10.0 milestone Apr 23, 2019

@markmandel markmandel modified the milestones: 0.10.0, 0.11.0 May 7, 2019

markmandel added a commit to markmandel/agones that referenced this issue Jun 13, 2019
Move FleetAutoscaling to autoscaling.agones.dev group
This is a pure refactor, to move FleetAutoscaling to the group outlined
in googleforgames#703, where it will stay.

Did this one first, since it seemed like the easiest one to tackle.
markmandel added a commit to markmandel/agones that referenced this issue Jun 13, 2019
Move FleetAutoscaling to autoscaling.agones.dev group
This is a pure refactor, to move FleetAutoscaling to the group outlined
in googleforgames#703, where it will stay.

Did this one first, since it seemed like the easiest one to tackle.
markmandel added a commit to markmandel/agones that referenced this issue Jun 13, 2019
Move FleetAutoscaling to autoscaling.agones.dev group
This is a pure refactor, to move FleetAutoscaling to the group outlined
in googleforgames#703, where it will stay.

Did this one first, since it seemed like the easiest one to tackle.
markmandel added a commit to markmandel/agones that referenced this issue Jun 13, 2019
Move FleetAutoscaling to autoscaling.agones.dev group
This is a pure refactor, to move FleetAutoscaling to the group outlined
in googleforgames#703, where it will stay.

Did this one first, since it seemed like the easiest one to tackle.
markmandel added a commit to markmandel/agones that referenced this issue Jun 13, 2019
Move FleetAutoscaling to autoscaling.agones.dev group
This is a pure refactor, to move FleetAutoscaling to the group outlined
in googleforgames#703, where it will stay.

Did this one first, since it seemed like the easiest one to tackle.
pooneh-m added a commit that referenced this issue Jun 17, 2019
Move FleetAutoscaling to autoscaling.agones.dev group (#829)
This is a pure refactor, to move FleetAutoscaling to the group outlined
in #703, where it will stay.

Did this one first, since it seemed like the easiest one to tackle.

@markmandel markmandel modified the milestones: 0.11.0, 0.12.0 Jun 18, 2019

@markmandel

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 27, 2019

Update on this - As you can see above, I did FleetAutoscaler, which was the easiest.

There is a PR #856 , to remove FleetAllocation, which will make this easier.

I'm currently focused on #660 - so I'm not working on this at the moment, so if someone wants to jump in, please feel free. Happy to provide helpers for how to make these changes as well. (make gen-crd-client is probably the biggest thing to look at)

The only thing left is moving stable.agones.dev => agones.dev

@roberthbailey

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Another thing we discussed yesterday was promoting the APIs that we are going to commit to keeping stable from v1alpha1 to v1 prior to cutting a 1.0 release. Given that moving stable.agones.dev -> agones.dev will be easier once #856 merges I can start looking at changing the version of the ones that have already been split out first.

@roberthbailey

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

I also noticed that https://github.com/googleforgames/agones/blob/master/site/static/agones.cast will need to be updated when the group name changes since the output shows fleet.stable.agones.dev.

@roberthbailey roberthbailey changed the title Proposal: Move group stable.agones.dev to core.agones.dev ? Proposal: Split up the api group stable.agones.dev Jul 16, 2019

@roberthbailey

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

Remaining work: go through all of the examples and see if they need to have new container images created with the sdk changes.

@markmandel

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 22, 2019

Is there anything left on this? I think this is done and can be closed now?

@roberthbailey

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

See the above comment - I'm going through the examples to make sure that they work and updating container images in agones-images where necessary.

@roberthbailey

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

PRs for updating examples (some of which have already merged):

Outstanding:

Merged:

  • allocator-service: #930
  • autoscaler-webhook: #931
  • cpp-simple: #934
  • nodejs-simple: #945
  • xonotic: #950

I'll close out this issue once the remaining two PRs are merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.