Skip to content
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

Fixed status updated only when Kafka instance ready with no updating #472

Conversation

ppatierno
Copy link
Contributor

When a Strimzi version upgrade is started by changing the spec.versions.strimzi in the ManagedKafka resource, the corresponding status.versions.strimzi field should still report the older one while the upgrade is in process so when the StrimziUpdating condition is still true.

Currently, there is a bug with fleetshard operator reporting the new Strimzi version in the status while the updating is still in progress; the status field should be updated only at the end of the update.

This PR fixes this issue checking that even if ready, Kafka instance is not updating.

@ppatierno ppatierno requested review from shawkins and k-wall August 9, 2021 16:13
@github-actions github-actions bot added the operator changes related to operator label Aug 9, 2021
@ppatierno
Copy link
Contributor Author

@shawkins I opened the PR this way but was wondering if it's much better just checking that "reason" has to be null other than status True and not checking the specific StrimziUpdating; maybe for other possible reason(s) we don't have to update the status.
Because you changed this readiness part I would like to get your opinion on this.

@k-wall k-wall added this to the 0.10.0 milestone Aug 9, 2021
@shawkins
Copy link
Contributor

shawkins commented Aug 9, 2021

I opened the PR this way but was wondering if it's much better just checking that "reason" has to be null other than status True and not checking the specific StrimziUpdating; maybe for other possible reason(s) we don't have to update the status.
Because you changed this readiness part I would like to get your opinion on this.

It seems if status is ready we should be setting the additional fields, but being more specific about the versions:

if (Status.True.equals(readiness.getStatus())) {
    status.setCapacity(new ManagedKafkaCapacityBuilder(managedKafka.getSpec().getCapacity()).build());
    if (!Reason.StrimziUpdating.equals(readiness.getReason())) {
        status.setVersions(new VersionsBuilder(managedKafka.getSpec().getVersions()).build());
    } else {
        //old version?
    }
    status.setAdminServerURI(kafkaInstance.getAdminServer().uri(managedKafka));
}

I realize that I may not be answering completely - you will see the Kafka reason as the reason if there is one as it takes priority.

@ppatierno
Copy link
Contributor Author

What you are proposing makes sense but takes into account that when it's ready for the first time, we are already setting everything.

Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ppatierno ppatierno merged commit 13133bc into bf2fc6cc711aee1a0c2a:main Aug 9, 2021
@ppatierno ppatierno deleted the fix-strimzi-version-status-update branch August 9, 2021 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
operator changes related to operator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants