-
Notifications
You must be signed in to change notification settings - Fork 813
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
Add status subresource to fleet and Gameserverset #575
Add status subresource to fleet and Gameserverset #575
Conversation
Build Succeeded 👏 Build Id: 0ce04f01-27a4-4380-86c3-64c934a75fa4 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:
|
2f3def3
to
2920e68
Compare
Build Succeeded 👏 Build Id: 2fdfe9e1-b192-4bb0-a569-85e3d2fb5e1b 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:
|
install/yaml/install.yaml
Outdated
@@ -48,13 +48,13 @@ rules: | |||
resources: ["customresourcedefinitions"] | |||
verbs: ["get"] | |||
- apiGroups: ["stable.agones.dev"] | |||
resources: ["gameservers", "gameserversets"] | |||
resources: ["gameservers", "gameserversets", "gameserversets/status"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there opportunity here to get more restrictive with our rbac permissions?
I mean - does the controller need to be able to update a gameserverset for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to restrict them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markmandel I have restricted new subresource permissions.
However cannot restrict gameserversets
permissions. In fleets and fleetautoscalers controllers we are using main Resource endpoint gsSetCopy.Spec.Replicas
and Spec.Replicas of the fleet:
https://github.com/GoogleCloudPlatform/agones/blob/0e92c90b9f0da86ab34cae53e74382113f56bf6e/pkg/fleets/controller.go#L268
https://github.com/GoogleCloudPlatform/agones/blob/0e92c90b9f0da86ab34cae53e74382113f56bf6e/pkg/fleetautoscalers/controller.go#L220
install/yaml/install.yaml
Outdated
verbs: ["create", "delete", "get", "list", "update", "watch"] | ||
- apiGroups: ["stable.agones.dev"] | ||
resources: ["gameservers"] | ||
verbs: ["patch"] | ||
- apiGroups: ["stable.agones.dev"] | ||
resources: ["fleets", "fleetallocations", "fleetautoscalers"] | ||
resources: ["fleets", "fleets/status", "fleetallocations", "fleetautoscalers"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above - could we lock this down a little tighter?
2920e68
to
3a5fca4
Compare
Build Succeeded 👏 Build Id: a5b93280-06c3-475c-8cf1-ee457ac4f573 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:
|
3a5fca4
to
bb319d9
Compare
Build Succeeded 👏 Build Id: eb89262a-285b-46bc-82db-5c1f475592e3 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:
|
The small regression after applying this changes:
|
@aLekSer I'm guessing this is because there is no default values for those status items on creation? I'd be tempted to ask #sig-apimachinery to see why 0 values come as blank? (although not quite sure what the reason for this is -- feels like a k8s bug?) I'm wondering if its something we need to worry about it? WDYT? |
I think this is because status fields are not defined in yaml file and the only place Status are changed (inited) is here: |
1642362
to
9f953b1
Compare
Build Failed 😱 Build Id: 897563be-bcc7-4b43-89d2-900e6f3a582d To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: b38b858d-20ca-433d-93cb-6a293f93077b To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
9f953b1
to
b5f8118
Compare
Build Failed 😱 Build Id: 5bfc7075-5e40-4ec1-8750-ecf685743aa9 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Some new error on a test step: Main error was on controller_test.go |
b5f8118
to
b146278
Compare
Build Succeeded 👏 Build Id: 7c2c10f3-ab08-4c36-93af-61e1b5e7c5e4 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:
|
Is the issue that the value is 0, or that status hasn't been set at all? If you scale a fleet up to 5, and then back down to 0, does it go back to being blank? I'm wondering if its because the status hasn't been created yet? |
Hello @markmandel ,
Status were not set at all, but now it is set on creating GSS with aditional step (above).
Now it always set right after create of GSS.
Yes, that was the case, no all should be fine with yesterday fix. |
gsSetCopy.Status.ReadyReplicas = 0 | ||
gsSetCopy.Status.Replicas = 0 | ||
gsSetCopy.Status.AllocatedReplicas = 0 | ||
_, err = gsSets.UpdateStatus(gsSetCopy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an extra step which set default values.
Switch to using UpdateStatus function when it is necessary, add RBAC permissions for "fleets/status" and "gameserversets/status".
b146278
to
efbac45
Compare
Build Succeeded 👏 Build Id: e406f1b9-7287-4201-b82a-9859e797dee8 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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎲
Build Succeeded 👏 Build Id: 38af98c4-be7c-4e7a-8d02-795013154af4 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:
|
Switch to use
UpdateStatus
function for Fleets when it is necessary, add RBACpermissions for "fleets/status" and "gameserversets/status".