Fix KafkaRebalanceAssemblyOperator to not ignore exception coming from Cruise Control

[ShubhamRwt]

Type of change

This PR fixes #10631. Now the KafkaRebalanceAssemblyOperator doesn't ignore the Illegal argument excepts which comes from Cruise Control and move the KafkaRebalance to NotReady in case any IllegalArgumentExceptions occurs

• Bugfix
• Enhancement / new feature
• Refactoring
• Documentation

Description

Please describe your pull request

Checklist

Please go through this checklist and make sure all applicable tasks have been done

• Write tests
• Make sure all tests pass
• Update documentation
• Check RBAC rights for Kubernetes / OpenShift roles
• Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
• Reference relevant issue(s) and close them after merging
• Update CHANGELOG.md
• Supply screenshots for visual changes, such as Grafana dashboards

[scholzj]

I'm not sure I follow the changes, that is probably more something for @ppatierno. But should there be some unit tests to check what you describe? You just changed some unit tests where reconciliation was failing to succeeding. It is not clear to me why. But none seems to test the situation I had where the reconciliation was stuck.

[ppatierno]

@scholzj I had an offline conversation with @ShubhamRwt about this and right now this is the best solution to fix that issue. Right now errors coming from CC are not handled pretty well. Some of them are "translated" into actual responses (i.e. the not enough data in window) to avoid showing errors to users and putting the KR in a "NotReady" state but showing a "PendingProposal" (because that's what the error means).
Said that, in this case we hava an actual error but failing the future doesn't pop up into the chain of calls moving the resource to "NotReady" so we decided to go this way for now in order to have this fix soon and also @see-quick tests to pass in relation to the stuck situation you see.
After this, I will try to revisit the way we handle CC errors to improve it, but it's better doing that later imho.

[ppatierno]

@scholzj update :-) ... we'll try to have something better on handling errors from CC for this use case as well, because it seems we do already something this way when getting user tasks status but not when rebalancing requests (as in this scenario). So ... stay tuned ;-)

[scholzj]

TBH, I do bot understand the code here very well. So my main issue was that it does not look anything like what the PR name/description suggests. So, maybe the PR is good and the explanation of what it does needs to be better? I don't know, you should be better placed to decide that.

…

On Mon, Oct 14, 2024, 12:01 Paolo Patierno ***@***.***> wrote: @scholzj <https://github.com/scholzj> update :-) ... we'll try to have something better on handling errors from CC for this use case as well, because it seems we do already something this way when getting user tasks status but not when rebalancing requests (as in this scenario). So ... stay tuned ;-) — Reply to this email directly, view it on GitHub <#10701 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLFOR5Q3YSDBXHLDRUPN53Z3OI7RAVCNFSM6AAAAABPYTKHJWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJQGY3DQMJXGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[ppatierno]

I went deeper in debugging the issue and I found the related problem which is about a "never ending" reconciliation keeping the Rebalancing state on the KafkaRebalance and of course not leaving the lock for the next reconciliation.
I will open a PR soon with the proposed fix. Then I guess this one could be closed.

[ppatierno]

After further investigation I found the root cause of the issue and fixed it via #10717.
We can close this one when #10717 is merged.

[scholzj]

If #10717 replaces this you should probably close it right away to avoid any confusions?