Defaulting min.insync.replicas to 1 when it's not specified by the user

[ppatierno]

This PR fixes #11880
What it does is just defaulting the min.insync.replicas to 1 when the user doesn't specify it within the .spec.kafka.config in the Kafka custom resource and it happens despite what's the value of ELR (enabled or not).
This way, it happens:

• if its value is changed, it's handled as a normal dynamic configuration change and brokers rolling is not needed.
• if its value is removed, the new code force the min.insync.replicas: 1 into the ConfigMap so that when it comes to the KafkaRoller it's still seen as an update (not a removal) and the dynamic configuration change is applied (still no brokers rolling).

[scholzj]

Why not add it as default to KafkaConfiguration? We have support for it and use it in other configuration classes such as KafkaConnectConfiguration.

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.50%. Comparing base (63dc1bd) to head (9e35e1c).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...rimzi/operator/cluster/model/KafkaSpecChecker.java	60.00%	0 Missing and 2 partials ⚠️
...perator/resource/KafkaBrokerConfigurationDiff.java	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #11883      +/-   ##
============================================
- Coverage     67.51%   67.50%   -0.02%     
+ Complexity     7083     7077       -6     
============================================
  Files           574      574              
  Lines         28143    28139       -4     
  Branches       3189     3188       -1     
============================================
- Hits          19002    18994       -8     
  Misses         7824     7824              
- Partials       1317     1321       +4     

Files with missing lines	Coverage Δ
...mzi/operator/cluster/model/KafkaConfiguration.java	89.36% <100.00%> (+0.47%)	⬆️
...perator/cluster/operator/resource/KafkaRoller.java	77.21% <100.00%> (-0.10%)	⬇️
...perator/resource/KafkaBrokerConfigurationDiff.java	91.46% <50.00%> (+2.05%)	⬆️
...rimzi/operator/cluster/model/KafkaSpecChecker.java	92.59% <60.00%> (-3.49%)	⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ppatierno]

Why not add it as default to KafkaConfiguration? We have support for it and use it in other configuration classes such as KafkaConnectConfiguration.

Yes but by adding DEFAULTS including min.insync.replicas (at 1) we are going to lost the warning message for the user because the KafkaSpecChecker.checkKafkaReplicationConfig will find the configuration (not being null), so I need to change it checking it's not 1 (both set by user explicitly or with using the DEFAULTS).

In that case I can add the check but changing slightly the warning message from:

"min.insync.replicas option is not configured. It defaults to 1 which does not guarantee reliability and availability. You should configure this option in .spec.kafka.config."

to:

"min.insync.replicas option is set to 1 explicitly or it is not configured which defaults to 1 as well. It does not guarantee reliability and availability. You should configure this option in .spec.kafka.config. with a value higher than 1"

[scholzj]

Yes but by adding DEFAULTS including min.insync.replicas (at 1) we are going to lost the warning message for the user because the KafkaSpecChecker.checkKafkaReplicationConfig will find the configuration (not being null), so I need to change it checking it's not 1 (both set by user explicitly or with using the DEFAULTS).

I guess it would be pretty simple to change the KafkaSpecChecker to get it from the KafkaSpec instead of the KafkaConfiguration?

[ppatierno]

@scholzj done. I also marked it as ready for review because I removed the previous solution we merged.

[katheris]

Thanks @ppatierno for addressing this so quickly.
I've reviewed the previous PRs and the various issues and I'm happy I now understand the problem and how this changes addresses it.

I prefer this over the original fix since it means it's easier for users to understand what the value will be when they remove min.insync.replicas from the Kafka CR.

[tinaselenge]

Thank you so much @ppatierno for redoing the fix. It has been a tricky one, so unfortunately we went back and forth on the solution despite the discussions and agreement on the issues. I think this solution looks more straight forward and easier to understand the reason behind it. And we get to avoid the extra checks on feature level etc. So all good from me!

[ppatierno]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[scholzj]

Mostly LGTM. I just left some nits. The regression tests failed with some Docker issue, so I restarted them. Hopefully they pass now.

[scholzj]

The regression tests failed with some Docker issue, so I restarted them. Hopefully they pass now.

I guess I was not the first one to try that. Let's see if GHA works better 🤔

[scholzj]

/gha run pipeline=regression

[github-actions]

⏳ System test verification started: link

[github-actions]

🎉 System test verification passed: link

[showuon]

One doc related comment. The solution LGTM! Thanks for improving it!

[PaulRMellor]

Thanks Paolo.
Considering the change of approach to the fix, the warning in the docs is perfect

[ppatierno]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).