Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

[MGDSTRM-9790] Test instance upgrade while suspended #854

Merged
merged 1 commit into from
Jan 23, 2023
Merged

[MGDSTRM-9790] Test instance upgrade while suspended #854

merged 1 commit into from
Jan 23, 2023

Conversation

MikeEdgar
Copy link
Contributor

@MikeEdgar MikeEdgar commented Jan 17, 2023 •
edited
Loading

  • Add test for MGDSTRM-9790 to suspend, upgrade Strimzi, and resume an MK
  • Increase operator reconcile max retry attempts, used when unhandled exceptions occur
  • Delay sync's polling of mock control plane in test profile (account for observed HTTP server startup time)
  • Modify sync's MK agent informer to only watch own namespace (aligns with ManagedKafkaAgentSync processing)
  • Deploy Keycloak and kas-fleetshard only once per suite execution

@github-actions github-actions bot added operator changes related to operator sync changes related to sync test Test related changes labels Jan 17, 2023
@MikeEdgar MikeEdgar requested review from kornys and shawkins January 17, 2023 21:39
@MikeEdgar
Copy link
Contributor Author

While trying to add a fourth test without impacting the duration of the tests too much, this PR certainly grew larger than what was expected for MGDSTRM-9790. I'm happy to split out the other changes if it is easier to review that way.

@MikeEdgar MikeEdgar marked this pull request as ready for review January 17, 2023 21:41
shawkins
shawkins reviewed Jan 17, 2023
View reviewed changes
test/src/main/java/org/bf2/test/TestUtils.java
@@ -165,16 +166,29 @@ public static Function<InputStream, InputStream> replacer(final Map<String, Stri
* Restart kubeapi
*/
public static void restartKubeApi() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be fine with removing this method and the test that is based upon it. This will not cause an informer to relist, just the underlying watches to reconnect. We also no longer have our own informer logic.

shawkins
shawkins reviewed Jan 17, 2023
View reviewed changes
systemtest/src/test/java/org/bf2/systemtest/unit/SuiteUnitTest.java
@@ -42,9 +49,16 @@
*/
@QuarkusTest
@QuarkusTestResource(KubernetesServerTestResource.class)
public class SuiteUnitTest extends AbstractST {
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the motivation to remove the base class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aside from the annotations, the only thing gained was the setup of the KubeClient. I figured the three other *ST classes had more to gain by consolidating setup in the base class without adding an intermediate parent.

@MikeEdgar
[MGDSTRM-9790] Test instance upgrade while suspended
cd06437 
Signed-off-by: Michael Edgar <medgar@redhat.com>
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

37.5% 37.5% Coverage
0.0% 0.0% Duplication

@MikeEdgar
Copy link
Contributor Author

@shawkins please let me if you get back to this and have any other comments.

@kornys please also take a look if you have time next week.

I can split this up if it's too much to consume for one PR.

@shawkins shawkins self-requested a review January 20, 2023 20:42
shawkins
shawkins approved these changes Jan 20, 2023
View reviewed changes
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.

@shawkins please let me if you get back to this and have any other comments.

No other comments, the application code changes are minimal and I'm fine with the test changes.

kornys
kornys approved these changes Jan 22, 2023
View reviewed changes
Copy link
Contributor

@kornys kornys left a comment

Choose a reason for hiding this comment

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

LGTM!

@MikeEdgar MikeEdgar merged commit 3fc85d0 into bf2fc6cc711aee1a0c2a:main Jan 23, 2023
@MikeEdgar MikeEdgar deleted the MGDSTRM-9790 branch January 23, 2023 12:30
@MikeEdgar MikeEdgar added this to the 0.33.0 milestone Jan 23, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@kornys kornys kornys approved these changes

@shawkins shawkins shawkins approved these changes

Assignees
No one assigned
Labels
operator changes related to operator sync changes related to sync test Test related changes
Projects
None yet
Milestone
0.33.0
Development

Successfully merging this pull request may close these issues.
3 participants
@MikeEdgar @shawkins @kornys