-
Notifications
You must be signed in to change notification settings - Fork 799
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 Sidecar log level parameter to GS specification #1007
Add Sidecar log level parameter to GS specification #1007
Conversation
What is left: documentation update and UT. |
Build Failed 😱 Build Id: 0d2740fa-f270-417d-9191-c6db48c68592 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build failed because I should have performed
|
a2301e2
to
0402062
Compare
Build Succeeded 👏 Build Id: 8723c226-c6d1-4a63-9c26-21833df2e505 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:
|
pkg/apis/agones/v1/gameserver.go
Outdated
@@ -135,6 +135,8 @@ type GameServerSpec struct { | |||
Health Health `json:"health,omitempty"` | |||
// Scheduling strategy. Defaults to "Packed". | |||
Scheduling apis.SchedulingStrategy `json:"scheduling,omitempty"` | |||
// LogLevel specifies the log level for gameserver sidecar. | |||
LogLevel string `json:"loglevel,omitempty"` |
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.
Random thought: Is "loglevel" too generic? Considering it's just for the sdk sidecar - should we pick a name that's more specific? sdk-server-loglevel
(seems long), not sure what the right answer is here, figured I would ask the question first.
Just wondering if the name might be confusing.
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.
yes, you are right, that would suit better.
f96fe14
to
d9987ac
Compare
Build Failed 😱 Build Id: 1c8ebc65-0e9b-4acb-8802-a3d4997cdf23 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: ff00d479-51e0-4d7f-b23d-ef6e6e874236 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:
|
Flaky E2E test:
|
@@ -49,6 +49,8 @@ spec: | |||
# Minimum consecutive failures for the health probe to be considered failed after having succeeded. | |||
# Defaults to 3. Minimum value is 1 | |||
failureThreshold: 3 | |||
# SdkServer Log Level configuration. Could be any of panic, fatal, error, warn, warning, info, debug and trace. Defaults to info |
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.
we should feature
wrap these items.
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.
Done
install/yaml/install.yaml
Outdated
@@ -860,6 +882,17 @@ spec: | |||
type: integer | |||
minimum: 1 | |||
maximum: 65535 | |||
loglevel: |
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.
Pretty sure this validation also needs to be changed, with the rename?
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.
yes, that's true
d9987ac
to
8c570b0
Compare
Build Succeeded 👏 Build Id: 916af54a-7c22-486c-91e7-78312ceb0c55 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:
|
@@ -92,6 +92,17 @@ properties: | |||
type: integer | |||
minimum: 1 | |||
maximum: 65535 | |||
sdk-server-loglevel: | |||
type: string | |||
enum: |
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 don't think we should expose so many log levels. This seems to be externalizing the fact that we are using logrus and would make it nearly impossible to change our logging library in the future. I feel like there are really three categories that people want:
- Default (what we have today, which in basically the info level)
- Errors and above
- Debugging - everything
What if we abstracted the log levels from logrus and just offered something smaller like those three? Alternatively, we could give people a way to specifically disable the ping logs since those are the ones that cause the most noise in the log file.
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.
Yes, less level would be more concise. I will do the change cause that would actually give more space for the changes in the future.
f5247f7
to
1d3ef31
Compare
Build Succeeded 👏 Build Id: b0632155-c2e8-47fb-86e2-f91d56737d23 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 Succeeded 👏 Build Id: 7a085cc1-0600-4f5b-ba38-802783b8a935 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:
|
1d3ef31
to
458876a
Compare
Build Succeeded 👏 Build Id: d0d73236-6774-4ffd-b06f-22b26ae6feaf 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.
This looks good to me. Since this adds a configuration option to GameServer
which we can't break once its out there, @roberthbailey you good to give me an also +1 that's we're both happy with this PR ?
458876a
to
1c126f7
Compare
/assign @aLekSer |
Build Succeeded 👏 Build Id: 3678dba6-96b2-4238-b041-81027322a405 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: d87c24ed-d156-49ec-a95a-e7bb9e576411 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
I have merged master to the branch. |
Build Succeeded 👏 Build Id: 7f74c6d7-1178-4b09-9fc4-0bbfdd359d26 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:
|
d55e6ae
to
c4ee99c
Compare
Updated publishVersion to a new one: Also I have found an issue with PR is now ready for review (and merge). |
c4ee99c
to
ffd2890
Compare
Build failed for some strange reason - no status comment was generated here on
|
Build Succeeded 👏 Build Id: 619e3d42-f1ba-4483-88e5-ed3d1f426774 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.
I did another pass and had a few more comments for you.
pkg/apis/agones/v1/gameserver.go
Outdated
// Template describes the Pod that will be created for the GameServer | ||
Template corev1.PodTemplateSpec `json:"template"` | ||
} | ||
|
||
// Logging contains log levels for Agones system containers | ||
type Logging struct { | ||
// SdkServer is a log level for SDK server (sidecar) logs |
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.
Please add Defaults to "Info"
at the end of this comment.
@@ -205,17 +212,19 @@ func TestGameServerApplyDefaults(t *testing.T) { | |||
Protocol: corev1.ProtocolTCP, | |||
}, | |||
}, | |||
Health: Health{Disabled: true}, | |||
Health: Health{Disabled: true}, | |||
Logging: Logging{SdkServer: "Debug"}, |
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.
rather than piggyback on an existing test case ("convert from legacy single port to multiple" to test applying a custom log level, please add a new test case that specifically tests setting the log level.
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.
OK, adding a separate test
@@ -57,6 +57,11 @@ spec: | |||
health: | |||
initialDelaySeconds: 30 | |||
periodSeconds: 60 | |||
{{% feature publishVersion="1.0.0" %}} |
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.
This should now be 1.1.0
a4156b8
to
b0b913e
Compare
Build Failed 😱 Build Id: 838b17fc-c2cb-4970-a589-7685c6b8b927 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Applied comments and added |
Build Failed 😱 Build Id: 41a79350-09d9-4c67-a281-2231e8cdeb9f To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
b0b913e
to
3e4c972
Compare
Build Succeeded 👏 Build Id: 1dae75b1-a06d-4fe4-9539-cedca78bdd42 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:
|
3e4c972
to
51ea158
Compare
Add ability to specify logrus log level in yaml configuration and apply this log level on creating the GameServer.
51ea158
to
3b2365a
Compare
Build Succeeded 👏 Build Id: e93355dd-0519-42b2-a1a6-cc9b3cd7c4a6 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 Succeeded 👏 Build Id: f0e5420e-e806-4bb7-be59-98c10c281c98 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:
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aLekSer, roberthbailey 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 |
Add ability to specify
logrus
log level in yaml configuration and apply this log level on GameServer creation step.Closes #971 .