Handle the case when the request to CC /state endpoint returns the async task instead of the result #974

aguzovatii · 2023-05-16T14:42:30Z

Description

This PR attempts to fix #828.

Currently marked this PR as draft until the change in the API module introduced with the #975 PR is merged.

When the request on CC /state endpoint takes longer than webserver.request.maxBlockTimeMs, then CC will convert the request into an async task and will respond with the taskID instead of the result. The Koperator should handle this case by waiting for CC to complete the task.

The approach is the following:

When getting the state of Cruise Control we can have 2 possible scenarios
1. Cruise Control returns the actual state, in this case (as before) we just return the state and continue the reconciliation
2. Cruise Control converts the Status request into an async task and returns just the taskID, then:
  1. We create a new CruiseControlOperation to track the progress of the Status request and requeue the reconciliation for later
  2. Next time the reconciliation is started, we notice that there is already a Status CruiseControlOperation in progress and just request the latest status of this task. If not ready, then we requeue again. This step is repeated until the CC task is completed

Notes:

we will have no more than 1 Status CruiseControlOperation in progress for a kafkaClusterReference.
I added a default timeout of 5 minutes, meaning that if we get the Status of CC in more than 5 minutes, then we will consider this outdated and retry

Type of Change

Bug Fix

Checklist

I have read the contributing guidelines
Existing issues have been referenced (where applicable)
I have verified this change is not present in other open pull requests
Functionality is documented
All code style checks pass
New code contribution is covered by automated tests
All new and existing tests pass

CLAassistant · 2023-05-16T14:42:35Z

All committers have signed the CLA.

controllers/cruisecontroloperation_controller.go

pkg/scale/scale.go

aguzovatii · 2023-05-20T07:18:14Z

Hey @bartam1, thank you for your review!
I addressed a few of the easier suggestions

I will be in vacation for a week, so I will not be responsive during this time.
The rest of suggestions I will be able to address starting with 29th of May.

bartam1 · 2023-06-02T06:16:13Z

New api tag has been created: https://github.com/banzaicloud/koperator/tree/api/v0.28.1
cc @aguzovatii

aguzovatii · 2023-06-06T12:05:26Z

Thank you, @bartam1!
I upgraded github.com/banzaicloud/koperator/api to v0.28.1 and marked this pr as ready for review, let me know if there are other things to address

panyuenlau

The implementation LGTM.

Just one question (I might be nit-picking here):

I found the naming of this OperationStatus CruiseControlTaskOperation = "status" in #975 is a bit misleading: the CruiseControlTaskOperation type was created to represent the actual CC operation. So when I saw OperationStatus CruiseControlTaskOperation = "status", I would assume there is an actual CC operation called "status", but I checked the CC wiki and source code and I couldn't really find such an operation.

But this could be due to my limited knowledge on cruise control

aguzovatii · 2023-06-08T06:43:28Z

Hi @panyuenlau, there is the CC GET state endpoint that is corresponding with the newly added OperationStatus CruiseControlTaskOperation = "status", check CC REST API docs.

I used the status name instead of state just to be consistent with how it was used in Koperator until now (check CruiseControlScaler):

koperator/pkg/scale/types.go

Line 28 in d00b18c

Status(ctx context.Context) (CruiseControlStatus, error)

I don't know the reason why it's status in Koperator, but we can change everywhere from status to state to not create confusion in the future if you think it's better.

panyuenlau · 2023-06-09T15:17:04Z

@aguzovatii Thank you for the explanation behind this!

I think it is fine in terms of this PR. We will refactor the naming in a separate PR if our team decide to go that way.

amuraru · 2023-06-11T13:37:16Z

controllers/tests/mocks/scale.go

@@ -295,6 +295,21 @@ func (mr *MockCruiseControlScalerMockRecorder) Status(ctx interface{}) *gomock.C
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Status", reflect.TypeOf((*MockCruiseControlScaler)(nil).Status), ctx)
 }

+// StatusTask mocks base method.


re-run make mock-generate to generate this file

done, generated the file in 193a716

…nc task instead of the result When the request on /state endpoint of CC takes longer than webserver.request.maxBlockTimeMs, then CC will convert the request into an async task and will respond with the taskID instead of the result. The Koperator should handle this case by waiting for CC to complete the task.

aguzovatii mentioned this pull request May 16, 2023

Add new CruiseControlTaskOperation to represent Status Cruise Control Operation #975

Merged

8 tasks

bartam1 reviewed May 18, 2023

View reviewed changes

bartam1 reviewed May 19, 2023

View reviewed changes

pkg/scale/scale.go Outdated Show resolved Hide resolved

bartam1 reviewed May 19, 2023

View reviewed changes

pkg/scale/scale.go Outdated Show resolved Hide resolved

aguzovatii force-pushed the handle_async_response_from_cc_state_endpoint branch from a4b68c1 to 9cb5644 Compare May 19, 2023 12:59

aguzovatii force-pushed the handle_async_response_from_cc_state_endpoint branch from a53d75b to aae0bd7 Compare June 6, 2023 11:53

aguzovatii marked this pull request as ready for review June 6, 2023 12:02

aguzovatii requested a review from a team as a code owner June 6, 2023 12:02

panyuenlau previously approved these changes Jun 7, 2023

View reviewed changes

panyuenlau requested a review from bartam1 June 7, 2023 15:28

panyuenlau mentioned this pull request Jun 9, 2023

Adding certificate validity duration in Kafka user #980

Merged

6 tasks

bartam1 previously approved these changes Jun 10, 2023

View reviewed changes

amuraru reviewed Jun 11, 2023

View reviewed changes

aguzovatii dismissed stale reviews from bartam1 and panyuenlau via 193a716 June 12, 2023 07:16

panyuenlau previously approved these changes Jun 12, 2023

View reviewed changes

aguzovatii added 3 commits June 14, 2023 18:15

Address code review suggestions

616dfca

Generate the mocks

02c7fc3

aguzovatii dismissed panyuenlau’s stale review via 02c7fc3 June 14, 2023 15:22

aguzovatii force-pushed the handle_async_response_from_cc_state_endpoint branch from 193a716 to 02c7fc3 Compare June 14, 2023 15:22

aguzovatii requested review from amuraru, bartam1 and panyuenlau June 14, 2023 15:23

bartam1 approved these changes Jun 15, 2023

View reviewed changes

panyuenlau approved these changes Jun 15, 2023

View reviewed changes

panyuenlau merged commit 7be5b82 into banzaicloud:master Jun 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle the case when the request to CC /state endpoint returns the async task instead of the result #974

Handle the case when the request to CC /state endpoint returns the async task instead of the result #974

aguzovatii commented May 16, 2023 •

edited

Loading

CLAassistant commented May 16, 2023 •

edited

Loading

aguzovatii commented May 20, 2023

bartam1 commented Jun 2, 2023 •

edited

Loading

aguzovatii commented Jun 6, 2023

panyuenlau left a comment

aguzovatii commented Jun 8, 2023

panyuenlau commented Jun 9, 2023

amuraru Jun 11, 2023

aguzovatii Jun 12, 2023

Handle the case when the request to CC /state endpoint returns the async task instead of the result #974

Handle the case when the request to CC /state endpoint returns the async task instead of the result #974

Conversation

aguzovatii commented May 16, 2023 • edited Loading

Description

Type of Change

Checklist

CLAassistant commented May 16, 2023 • edited Loading

aguzovatii commented May 20, 2023

bartam1 commented Jun 2, 2023 • edited Loading

aguzovatii commented Jun 6, 2023

panyuenlau left a comment

Choose a reason for hiding this comment

aguzovatii commented Jun 8, 2023

panyuenlau commented Jun 9, 2023

amuraru Jun 11, 2023

Choose a reason for hiding this comment

aguzovatii Jun 12, 2023

Choose a reason for hiding this comment

aguzovatii commented May 16, 2023 •

edited

Loading

CLAassistant commented May 16, 2023 •

edited

Loading

bartam1 commented Jun 2, 2023 •

edited

Loading