Skip to content

Conversation

@hzhao-github
Copy link
Contributor

@hzhao-github hzhao-github commented Nov 5, 2021

Currently TwoDomainLoadBalancer test class has method testDeployAppAndInstallIngressControllers() which install all three lb ingress and deploy the clusterView application.

When one of the loadbalancers fails to install, all the test fail, as each test uses the cluster view application to verify inter-server communication. If ngnix installation fails, traefik and apache tests also fails.

So we need to de associate each test methods to be independent of all type of lb igrress installation.

Jenkin Run(s)
https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-kind-new/7137/
https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-kind-new/71334

Copy link
Member

@anpanigr anpanigr left a comment

Choose a reason for hiding this comment

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

Can we avoid the Ordering of the tests in the class.

@hzhao-github
Copy link
Contributor Author

I'd not recommend removing the ordering because if we do so, we need to move installation of three LBers to BeforeAll. Then we go back to where we have this change -- when one installation of LBer failed, entire tests fail

Copy link
Member

@anpanigr anpanigr left a comment

Choose a reason for hiding this comment

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

All the LoadBalancer tests ( 3 to 15) depends on Test(2) i.e.
testTwoDomainsManagedByOneOperatorSharingPV because Test(1) creates TwoDomains and shutdown both.
How the NGINX tests depends on Traefik if you consider Ordering in needed.

Why testTraefikHostRoutingAdminServer(#5) depend on testNginxTLSPathRoutingAcrossDomains(#4)

@hzhao-github
Copy link
Contributor Author

You are right that all LBers related tests have to be done after order(1) and order(2). However, in order to remove the ordering, we have to install all three LBers before all tests (except 1 & 2) start, that strategy bring back the issue of one LBer failed to install, no test will run. It doesn't matter whether 5 depends on 4, I just didn't touch the original ordering. If you don't like it, I can re-order to group same LBer test together.

Copy link
Member

@anpanigr anpanigr left a comment

Choose a reason for hiding this comment

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

The other option is create separate class for each LB. Move the Test(1) and Test(2) to a separate class. For each LoadBalancer class beforeAll() create two domains and install the LoadBalancer, then all tests are independent.

We need a separate JIRA to accomplish this. Can you try to fails the Ngix LoadBalancer installation and make sure that Traefik and Apache test works fine.

Copy link
Member

@anpanigr anpanigr left a comment

Choose a reason for hiding this comment

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

LGTM

@hzhao-github
Copy link
Contributor Author

I changed NGINX namespace to force installing NGINX fail,

nginxHelmParams = installAndVerifyNginx(nginxNamespace + "123", 0, 0);

The results are expected, only NIGNX related tests failed all other tests passed

[INFO]
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] ItTwoDomainsLoadBalancers.testNginxTLSPathRoutingAdminServer:356->installIngressController:1928 [Test NGINX installation succeeds] NGINX installation is failed
[ERROR] Errors:
[ERROR] ItTwoDomainsLoadBalancers.testNginxHttpHostRoutingAcrossDomains:513->getNginxLbNodePort:1551 NullPointer
[ERROR] ItTwoDomainsLoadBalancers.testNginxHttpsHostRoutingAcrossDomains:531->getNginxLbNodePort:1551 NullPointer
[ERROR] ItTwoDomainsLoadBalancers.testNginxPathRoutingAcrossDomains:547->getNginxLbNodePort:1551 NullPointer
[ERROR] ItTwoDomainsLoadBalancers.testNginxTLSPathRoutingAcrossDomains:382->getNginxLbNodePort:1551 NullPointer
[INFO]
[ERROR] Tests run: 15, Failures: 1, Errors: 4, Skipped: 0
[INFO]

Copy link
Contributor

@vanajamukkara vanajamukkara left a comment

Choose a reason for hiding this comment

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

LG

@rjeberhard rjeberhard merged commit 1ec2903 into main Nov 9, 2021
@hzhao-github hzhao-github deleted the two-domain-lb branch November 29, 2021 20:08
@hzhao-github hzhao-github restored the two-domain-lb branch November 29, 2021 20:08
@rjeberhard rjeberhard deleted the two-domain-lb branch January 31, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants