[ST] Fix StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion test to run only when there is really an unsupported Kafka version across versions #10496

egyedt · 2024-08-26T15:49:04Z

Type of change

Enhancement / Bug

Description

It is described in #10495.
The problem is that StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion is use a supported Kafka version from start so the required test with an unsupported Kafka version is not happen. We should use unsupported Kafka version in the test.

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

Thanks for the PR. Could you please:

Fix the DCO signoff? The instructions should be under the Details link next to the DCO status in the Pr status section.
Can you please update the PR description? Issues have often a long discussions and in general describe a problem. The PR description should have an explanation what does the PR do and why. Just linking to an issue is not really the same.

systemtest/src/test/java/io/strimzi/systemtest/upgrade/regular/StrimziUpgradeST.java

scholzj · 2024-08-26T23:50:43Z

systemtest/src/test/java/io/strimzi/systemtest/upgrade/regular/StrimziUpgradeST.java

+        controllerPods = RollingUpdateUtils.waitTillComponentHasRolledAndPodsReady(TestConstants.CO_NAMESPACE, controllerSelector, 3, controllerPods);
+        brokerPods = RollingUpdateUtils.waitTillComponentHasRolledAndPodsReady(TestConstants.CO_NAMESPACE, brokerSelector, 3, brokerPods);
+        eoPods = DeploymentUtils.waitTillDepHasRolled(TestConstants.CO_NAMESPACE, KafkaResources.entityOperatorDeploymentName(clusterName), 1, eoPods);


I expect that this will not work if the Kafka version is unsupported? You have to first change the Kafka version before waiting for a rolling update. Or how does it handle such a situation?

Yeah I explained what is wrong with the test here: #10495 (comment)

The parsing of Kafka version needs to be fixed, so the test will not be "flaky" and it will test exactly what it should.
In case that this check will be there, it will somehow work now, but when the real unsupported Kafka version will be parsed there, it will fail the test.

scholzj · 2024-09-05T16:04:25Z

@egyedt Any chance to get back to this? Thanks.

egyedt · 2024-09-05T21:58:30Z

@scholzj
Thanks for pinging me. I think, I will be able to work on this in the next week or the week after. Thanks.

im-konge

Thanks for the changes, they look good to me. Only thing is the removal of this and final from the places you are editing in this PR. Could you return them back? I know that there are various code styles that we can use (or you prefer), but we are trying to standardize the final for the stuff that will not be changed and the this in the objects classes. Thanks

egyedt · 2024-09-19T14:11:38Z

@im-konge
Thanks for the review, I reverted the not necessary changes. Could you please have a look again?
Thanks.

im-konge

Looks great 🙂 thanks a lot for the PR 🚀

im-konge · 2024-09-19T16:16:16Z

/azp run upgrade

azure-pipelines · 2024-09-19T16:16:28Z

Azure Pipelines successfully started running 1 pipeline(s).

im-konge · 2024-09-19T21:55:11Z

I checked the failures in the pipeline, but FMPOV they are not relevant to this PR. Maybe @henryZrncik knows more what can be problem here?

im-konge · 2024-09-23T19:53:56Z

@egyedt because of the logic we have there now, the tests are failing when they are skipped by the assumption -> @henryZrncik is currently working on a fix, once his PR is merged, could you please rebase this PR, so we can run the tests? I will let you know. Thanks!

henryZrncik

LGTM. Thanks for PR @egyedt. The mentioned PR to wait for is here #10634

im-konge · 2024-09-24T21:25:12Z

@egyedt the PR from @henryZrncik is merged, could you please rebase the PR, so I can trigger the tests? Thanks a lot!

…only when there is really an unsupported Kafka version across versions Signed-off-by: Egyed Tamas <egyed.t@cloudera.com> Signed-off-by: Tamas Barnabas Egyed <egyed.t@gmail.com>

egyedt · 2024-09-25T08:13:20Z

Thanks @henryZrncik.
@im-konge I rebased onto the top of the main branch.

see-quick · 2024-09-25T08:14:29Z

/azp run upgrade

azure-pipelines · 2024-09-25T08:14:39Z

Azure Pipelines successfully started running 1 pipeline(s).

egyedt · 2024-09-25T13:19:09Z

@im-konge It looks like all tests are green now!

see-quick · 2024-09-25T14:26:50Z

@im-konge It looks like all tests are green now!

Thanks for working on this @egyedt . If nothing is on @scholzj mind we are good at merging this PR 💯.

im-konge · 2024-09-25T14:44:28Z

Thanks a lot for the fix @egyedt ! :)

egyedt force-pushed the fix-testUpgradeAcross-timing_issue branch from a23110f to 3cd49a8 Compare August 26, 2024 15:58

scholzj added this to the 0.44.0 milestone Aug 26, 2024

scholzj reviewed Aug 26, 2024

View reviewed changes

egyedt force-pushed the fix-testUpgradeAcross-timing_issue branch from 3cd49a8 to 5ef7836 Compare September 19, 2024 09:56

egyedt changed the title ~~Make StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion more robust~~ Fix StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion test to run only when there is really an unsupported Kafka version across versions Sep 19, 2024

egyedt mentioned this pull request Sep 19, 2024

[Bug]: Fix StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion to use unsupported Kafka version instead of a supported one #10495

Closed

egyedt force-pushed the fix-testUpgradeAcross-timing_issue branch 3 times, most recently from d4e5960 to e3bf6cc Compare September 19, 2024 11:49

im-konge reviewed Sep 19, 2024

View reviewed changes

egyedt force-pushed the fix-testUpgradeAcross-timing_issue branch from e3bf6cc to ef91327 Compare September 19, 2024 14:08

im-konge approved these changes Sep 19, 2024

View reviewed changes

im-konge requested a review from a team September 19, 2024 16:10

henryZrncik approved these changes Sep 24, 2024

View reviewed changes

Fix testUpgradeAcrossVersionsWithUnsupportedKafkaVersion test to run …

4630c73

…only when there is really an unsupported Kafka version across versions Signed-off-by: Egyed Tamas <egyed.t@cloudera.com> Signed-off-by: Tamas Barnabas Egyed <egyed.t@gmail.com>

egyedt force-pushed the fix-testUpgradeAcross-timing_issue branch from ef91327 to 4630c73 Compare September 25, 2024 08:12

see-quick approved these changes Sep 25, 2024

View reviewed changes

see-quick added System tests ready for merge Label for PRs which are ready for merge labels Sep 25, 2024

see-quick merged commit b2c9639 into strimzi:main Sep 25, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ST] Fix StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion test to run only when there is really an unsupported Kafka version across versions #10496

[ST] Fix StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion test to run only when there is really an unsupported Kafka version across versions #10496

egyedt commented Aug 26, 2024 •

edited

Loading

scholzj left a comment

scholzj Aug 26, 2024

im-konge Aug 27, 2024

scholzj commented Sep 5, 2024

egyedt commented Sep 5, 2024

im-konge left a comment

egyedt commented Sep 19, 2024

im-konge left a comment

im-konge commented Sep 19, 2024

azure-pipelines bot commented Sep 19, 2024

im-konge commented Sep 19, 2024

im-konge commented Sep 23, 2024

henryZrncik left a comment

im-konge commented Sep 24, 2024

egyedt commented Sep 25, 2024 •

edited

Loading

see-quick commented Sep 25, 2024

azure-pipelines bot commented Sep 25, 2024

egyedt commented Sep 25, 2024

see-quick commented Sep 25, 2024

im-konge commented Sep 25, 2024

[ST] Fix StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion test to run only when there is really an unsupported Kafka version across versions #10496

[ST] Fix StrimziUpgradeST.testUpgradeAcrossVersionsWithUnsupportedKafkaVersion test to run only when there is really an unsupported Kafka version across versions #10496

Conversation

egyedt commented Aug 26, 2024 • edited Loading

Type of change

Description

Checklist

scholzj left a comment

Choose a reason for hiding this comment

scholzj Aug 26, 2024

Choose a reason for hiding this comment

im-konge Aug 27, 2024

Choose a reason for hiding this comment

scholzj commented Sep 5, 2024

egyedt commented Sep 5, 2024

im-konge left a comment

Choose a reason for hiding this comment

egyedt commented Sep 19, 2024

im-konge left a comment

Choose a reason for hiding this comment

im-konge commented Sep 19, 2024

azure-pipelines bot commented Sep 19, 2024

im-konge commented Sep 19, 2024

im-konge commented Sep 23, 2024

henryZrncik left a comment

Choose a reason for hiding this comment

im-konge commented Sep 24, 2024

egyedt commented Sep 25, 2024 • edited Loading

see-quick commented Sep 25, 2024

azure-pipelines bot commented Sep 25, 2024

egyedt commented Sep 25, 2024

see-quick commented Sep 25, 2024

im-konge commented Sep 25, 2024

egyedt commented Aug 26, 2024 •

edited

Loading

egyedt commented Sep 25, 2024 •

edited

Loading