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

Allocation endpoint: Deprecate metaPatch for metadata #2042

Closed
markmandel opened this issue Apr 2, 2021 · 4 comments · Fixed by #2070
Closed

Allocation endpoint: Deprecate metaPatch for metadata #2042

markmandel opened this issue Apr 2, 2021 · 4 comments · Fixed by #2070
Labels
area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc good first issue These are great first issues. If you are looking for a place to start, start here! help wanted We would love help on these issues. Please come help us! kind/cleanup Refactoring code, fixing up documentation, etc
Milestone

Comments

@markmandel
Copy link
Member

Context:
https://github.com/googleforgames/agones/blob/release-1.13.0/proto/allocation/allocation.proto#L53

We have a concept disconnect currently between a GameServerAllocation and the Allocation Service, in that when passing labels or annotations on a GameServerAllocation it is done with a metadata element, and the Allocation Service it is done with a metaPatch.

I've seen a few users get caught up by this (and we have docs for it now too).

I would advocate we deprecate (but keep the functionality) of metaPatch in the gRPC Allocation service proto and replace it with metadata to avoid a change in concept name all the way from GameServerAllocation ➡️ Allocation Endpoint

Probably something like:

message AllocationRequest {
  // The k8s namespace that is hosting the targeted fleet of gameservers to be allocated
  string namespace = 1;

  // If specified, multi-cluster policies are applied. Otherwise, allocation will happen locally.
  MultiClusterSetting multiClusterSetting = 2;

  // The required allocation. Defaults to all GameServers.
  LabelSelector requiredGameServerSelector = 3;

  // The ordered list of preferred allocations out of the `required` set.
  // If the first selector is not matched, the selection attempts the second selector, and so on.
  repeated LabelSelector preferredGameServerSelectors = 4;

  // Scheduling strategy. Defaults to "Packed".
  SchedulingStrategy scheduling = 5;
  enum SchedulingStrategy {
    Packed = 0;
    Distributed = 1;
  }

  // Deprecated: Please use metadata instead.
  MetaPatch metaPatch = 6;

  // metadata is optional custom metadata that is added to the game server at
  // allocation You can use this to tell the server necessary session data
  MetaPatch metadata = 7;
}

And then I believe we could handle the conversion of both in the converter library.

@markmandel markmandel added kind/cleanup Refactoring code, fixing up documentation, etc area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc labels Apr 2, 2021
@markmandel
Copy link
Member Author

@pooneh-m you originally wrote this, WDYT? Just trying to simplify things, but also not break backward compatibility.

Apologies I didn't see this in my initial review 😞

@markmandel markmandel added good first issue These are great first issues. If you are looking for a place to start, start here! help wanted We would love help on these issues. Please come help us! labels Apr 2, 2021
@pooneh-m
Copy link
Contributor

pooneh-m commented Apr 4, 2021

Thanks for the catch. Sounds good on deprecation and replacement, if renaming avoids confusion and not adding to it for the the behavior difference across the two fields.

@lambertwang
Copy link
Contributor

Do we also want to deprecate and rename the metapatch field in GameServerAllocationSpec?

@markmandel
Copy link
Member Author

OIC where that came from now. The json spec is json:"metadata -- so I probably don't think we need to beak the data structure GameServerAllocationSpec at this time, but I can see how that would also be confusing if you were building that data structure from scratch. Unless people think strongly otherwise? (maybe in a separate ticket?)

I wonder what I was thinking at the time when I did that 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc good first issue These are great first issues. If you are looking for a place to start, start here! help wanted We would love help on these issues. Please come help us! kind/cleanup Refactoring code, fixing up documentation, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants