-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Refactored fallback logic to only patch status when the fallback is e… #5659
base: main
Are you sure you want to change the base?
Conversation
…nabled Signed-off-by: Bharath Guvvala <bharath.raghavendra@gmail.com>
Signed-off-by: Bharath Guvvala <bharath.raghavendra@gmail.com>
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! Could you update the changelog linking the fixed issue? I think that improvements can be the better fit considering that this will improve the performance.
@zroubalik @dttung2905 PTAL if you have time :)
/run-e2e |
@bharathguvvala , could you check the failing CI checks? I think that unit tests probably need to be updated as they expect the call to |
Signed-off-by: Bharath Guvvala <bharath.raghavendra@gmail.com>
Signed-off-by: Bharath Guvvala <bharath.raghavendra@gmail.com>
pkg/fallback/fallback_test.go
Outdated
@@ -67,7 +67,7 @@ var _ = Describe("fallback", func() { | |||
primeGetMetrics(scaler, expectedMetricValue) | |||
so := buildScaledObject(nil, nil) | |||
metricSpec := createMetricSpec(3) | |||
expectStatusPatch(ctrl, client) | |||
//expectStatusPatch(ctrl, client) |
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.
If this isn't required, can we remove the code? it's confusing for new folks
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.
Sorry about the latest changes, I am testing out the fixes made to the tests. These will be removed in the final 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.
Somehow these fixes are working locally and CI is breaking. Give me sometime to fix this.
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.
sure, no worries xD
I thought that it was ready to review, but if you are working on it, just let me know when it's ready 😄
pkg/fallback/fallback_test.go
Outdated
@@ -114,7 +114,7 @@ var _ = Describe("fallback", func() { | |||
|
|||
so := buildScaledObject(nil, nil) | |||
metricSpec := createMetricSpec(3) | |||
expectStatusPatch(ctrl, client) | |||
//expectStatusPatch(ctrl, client) |
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
//mockClient.EXPECT().Status().Return(mockStatusWriter) | ||
//mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) |
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
@@ -153,7 +153,7 @@ func TestGetScaledObjectMetrics_FromCache(t *testing.T) { | |||
recorder := record.NewFakeRecorder(1) | |||
mockClient := mock_client.NewMockClient(ctrl) | |||
mockExecutor := mock_executor.NewMockScaleExecutor(ctrl) | |||
mockStatusWriter := mock_client.NewMockStatusWriter(ctrl) | |||
//mockStatusWriter := mock_client.NewMockStatusWriter(ctrl) |
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
//mockClient.EXPECT().Status().Return(mockStatusWriter) | ||
//mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) |
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
@@ -259,7 +259,7 @@ func TestGetScaledObjectMetrics_InParallel(t *testing.T) { | |||
recorder := record.NewFakeRecorder(1) | |||
mockClient := mock_client.NewMockClient(ctrl) | |||
mockExecutor := mock_executor.NewMockScaleExecutor(ctrl) | |||
mockStatusWriter := mock_client.NewMockStatusWriter(ctrl) | |||
//mockStatusWriter := mock_client.NewMockStatusWriter(ctrl) |
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
//mockClient.EXPECT().Status().Times(len(metricNames)).Return(mockStatusWriter) | ||
//mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Times(len(metricNames)).Return(nil) |
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
@@ -907,7 +907,7 @@ func TestScalingModifiersFormula(t *testing.T) { | |||
recorder := record.NewFakeRecorder(1) | |||
mockClient := mock_client.NewMockClient(ctrl) | |||
mockExecutor := mock_executor.NewMockScaleExecutor(ctrl) | |||
mockStatusWriter := mock_client.NewMockStatusWriter(ctrl) | |||
//mockStatusWriter := mock_client.NewMockStatusWriter(ctrl) |
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
//mockClient.EXPECT().Status().Return(mockStatusWriter).Times(2) | ||
//mockStatusWriter.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(2) |
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
Signed-off-by: Bharath Guvvala <bharath.raghavendra@gmail.com>
Signed-off-by: Bharath Guvvala <bharath.raghavendra@gmail.com>
Signed-off-by: Bharath Guvvala <bharath.raghavendra@gmail.com>
Signed-off-by: Bharath Guvvala <bharath.raghavendra@gmail.com>
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.
@bharathguvvala hi, what is the status of this PR please?
@zroubalik Apologies about the delay. I'll close this within a week before Jun 14. |
Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com> Signed-off-by: Bharath Raghavendra Reddy Guvvala <bharath.raghavendra@gmail.com>
no worries, thanks for the update! |
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.
@bharathguvvala there are still failures in static checks and unit tests
…nabled
This change ensures that the update health status of the scaledObjects is only updated when the fallback configuration is enabled on the scaledobject. Further optimizations to avoid redundant updates when the status doesn't change can be done later. Tests will be added, if the change is ratified by the maintainers.
Checklist
Fixes #5624