-
Notifications
You must be signed in to change notification settings - Fork 857
Log Chain ID in Events When Applying ChainPolicy in FleetAutoscaler #4131
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
Conversation
@indexjoseph fyi |
Below is an example of the relevant event output:
|
Build Failed 😭 Build Id: 49128080-b1a1-49fb-86b0-0ef305887429 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😭 Build Id: 8c857142-ea5f-496f-ab18-168358f4ed0e Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
/gcbrun |
Build Failed 😭 Build Id: b0da549d-cefb-4b56-ba9d-b0b91a2ff116 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
I think this all looks good except for one thing - it seems like the current functionality we're logging on every sync cycle. I'm not sure if it matters that much but I think we should only log when there's a change within the chain. For instance, we'd log when the |
I'm also curious if this bug can be consolidated with #3955. |
You want to stop logging the default state. right? |
Build Succeeded 🥳 Build Id: 875a1333-609a-4c35-b15c-d34208dac51e 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:
|
Not necessarily, I'm pointing out that the current implementation logs the applied policy and fleet size on every synchronization cycle of the FleetAutoscaler. This results in repetitive event entries, even when there's no actual change in the applied policy or the fleet's size. My suggestion is to modify the logging behavior so that it only occurs when the FleetAutoscaler switches from applying one policy to another (e.g., from 'default' to 'weekend'). Although I'm unsure if this is possible without adding logic to hold state, as of right now there's no way to compare the previous policy within a chain against the current policy and determine whether a new log entry is needed, it's up to you if you think it's necessary. |
Build Failed 😭 Build Id: 51940118-42fc-4173-9090-f79f0d634ad3 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
@indexjoseph I added a
|
/gcbrun |
Build Failed 😭 Build Id: 33faf044-500e-40ea-a8f9-9ee8107a6924 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😭 Build Id: a1dbfb6e-ac25-4330-8bb3-b1e515370288 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😭 Build Id: 34084e68-3cd6-4a26-a0a7-d20a65fb7a89 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 🥳 Build Id: f48d9bb5-7330-4b04-8caa-a89bf80a44e9 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:
|
Yep, LGTM! |
Build Succeeded 🥳 Build Id: 65e690b7-f645-4e19-bed9-f788c13751bc 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 left some questions
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 a unit or e2e test that checks that the last applied policy works as expected? And throws handles errors as expected?
// WithLastAppliedPolicy sets the LastAppliedPolicy field in the declarative configuration to the given value | ||
// and returns the receiver, so that objects can be built by chaining "With" function invocations. | ||
// If called multiple times, the LastAppliedPolicy field is set to the value of the last call. |
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.
The first sentence of this comment is confusing. Could you reword?
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 can't edit this file. please correct me if i'm wrong
agones/pkg/client/applyconfiguration/autoscaling/v1/fleetautoscalerstatus.go
Lines 15 to 17 in 533ff3f
// This code was autogenerated. Do not edit directly. | |
// Code generated by applyconfiguration-gen. DO NOT EDIT. |
Yes, I added unit and e2e tests to verify LastAppliedPolicy and error handling. |
Build Failed 😭 Build Id: 021e7380-1dac-4f66-a4aa-4f55999e1da2 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
/gcbrun |
Build Failed 😭 Build Id: 36aa6bc7-1611-45c3-80a6-9626df52343b Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 🥳 Build Id: a873a5c6-a422-4c16-9822-4a72c2aa4579 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:
|
@@ -1301,6 +1301,20 @@ func TestCounterAutoscalerAllocatedMultipleNamespaces(t *testing.T) { | |||
return fas.Status.CurrentReplicas > 0 | |||
}) | |||
|
|||
// Wait for LastAppliedPolicy to be set to CounterPolicyType for fasA | |||
framework.WaitForFleetAutoScalerCondition(t, fasA, func(log *logrus.Entry, fas *autoscalingv1.FleetAutoscaler) bool { |
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.
Right now this is under the test TestCounterAutoscalerAllocatedMultipleNamespaces
. Could you move either to its own test, or under TestChainAutoscaler
?
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 added couple of testcase for TestChainAutoscaler
and TestAutoscalerWebhook
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.
lgtm, i believe Ivy requested changes
Build Succeeded 🥳 Build Id: 70386160-7a67-44f0-91bc-251ae2eaf588 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.
LGTM
What type of PR is this?
/kind cleanup
What this PR does / Why we need it:
Added logging of Chain ID to the event message when a ChainPolicy is applied in the FleetAutoscaler controller.
Which issue(s) this PR fixes:
Closes #3953
Special notes for your reviewer: