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

PoC for Kubernetes provisioning support (via OLM) #206

Merged
merged 8 commits into from
Nov 25, 2024

Conversation

fabiobrz
Copy link
Member

@fabiobrz fabiobrz commented Nov 4, 2024

Description

  • Adding a target cloud environment concept to address both Kubernetes and OpenShift provisoning
  • Adding a Kubernetes client module, inspired by XTF OpenShift client implementation, to support K8s provisioning
  • Refactoring existing classes to cover both K8s and OpenShift cases
  • Adjusting the integration tests accordingly, and adding a GitHub workflow to run K8s e2e tests

Resolves #133

Additional issues which are solved by this PR:

Since the changes are broadly impacting the codebase, some issues have been fixed along the ways, ideally in separated commits:

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • Pull Request contains a description of the changes
  • Pull Request does not include fixes for multiple issues/topics
  • Code is self-descriptive and/or documented
  • I have implemented unit tests to cover my changes
  • I tested my code in OpenShift

@fabiobrz fabiobrz force-pushed the feat.k8s-take5 branch 10 times, most recently from b6fb8df to 4298e0d Compare November 7, 2024 15:59
@fabiobrz fabiobrz force-pushed the feat.k8s-take5 branch 4 times, most recently from a505dff to 788a78c Compare November 13, 2024 11:39
new HyperfoilOperatorApplication() {
@Override
public Hyperfoil getHyperfoil() {
Hyperfoil hyperfoil = new HyperfoilBuilder(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this could return directly without using a local variable as in HyperfoilOpenShiftOperatorProvisionerTest

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

}

@Override
public TargetType getTarget() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

couldn't this be deduced from execution context? I am assuming we run against K8 OR OCP, not the two at the same time in the same test run

Copy link
Member Author

Choose a reason for hiding this comment

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

Intersmash uses ProvisionerFactory instances to dynamically select the relevant provisioner for an Application, i.e. the Application type drives the decision, and this is the logic that we want to build upon.

That being said I changed the implementation in the last push so that there is no longer need for the implementing Application instance to set an enum value, but just to additionally implement either OpenShiftApplication or KubernetesApplication

IIUC what you are relating to, is the ability for a test suite that uses Intersmash to execute a set of tests just against K8s or OpenShift in one single run. This is something that must be implemented in the domain of the client test suite, and not by Intersmash. We actually do something similar in the Intersmash testsuite, where we use the OpenShiftTest or KubernetesTest categories to select which tests should be run.


@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
@KubernetesTest
public class HyperfoilKubernetesOperatorProvisionerTest implements NamespaceCreationCapable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

couldn't we use a single interface and deduce if K8 OR OCP from some context? I am assuming we run against K8 OR OCP, not the two at the same time in the same test run

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean for Project vs. Namespace creation?
Tests are meant to be different, depending on whether they would target Kubernetes or OpenShift, and such detail (i.e. whether to create a Namespace or a Project), is one of the details that should be set explicitly to decouple the provisioning functionality from the execution context.
The same applies if you're relating to K8s vs. OpenShift tests. In fact, it is very likely that different resources or processes would be verified, depending on the target platform.
Of course there could also be the chance that K8s or OpenShift scenarios and tests could be similar, but this should not break the original requirement to be able and deploy/tests on both the platforms.

IIUC what you are relating to, is the ability for a test suite that uses Intersmash to execute a set of tests just against K8s or OpenShift in one single run. This is something that must be implemented in the domain of the client test suite, and not by Intersmash. We actually do something similar in the Intersmash testsuite, where we use the OpenShiftTest or KubernetesTest categories to select which tests should be run.

@fabiobrz
Copy link
Member Author

/test

@fabiobrz fabiobrz force-pushed the feat.k8s-take5 branch 8 times, most recently from b0eb8af to 2045dc9 Compare November 18, 2024 17:39
@fabiobrz fabiobrz closed this Nov 18, 2024
@fabiobrz fabiobrz reopened this Nov 18, 2024
@fabiobrz fabiobrz force-pushed the feat.k8s-take5 branch 2 times, most recently from f49be5e to d88eabe Compare November 20, 2024 16:18
@fabiobrz
Copy link
Member Author

Thanks for the initial review comments, @tommaso-borgato
Hopefully I've answered your questions.

Regarding the CI checks, we are now green on Kubernetes, as you can see from the CI checks, but I am still fixing the OpenShift ones.
Feel free to add any additional comment.

@fabiobrz fabiobrz force-pushed the feat.k8s-take5 branch 6 times, most recently from 51ecc38 to 78a9a0e Compare November 21, 2024 15:51
@fabiobrz
Copy link
Member Author

/test

@fabiobrz fabiobrz mentioned this pull request Nov 21, 2024
10 tasks
@fabiobrz
Copy link
Member Author

Intersmash PR CI check results

Hi @fabiobrz, this comment is meant to report the outcome of Intersmash integration tests,
which were triggered by @fabiobrz comment, see #206 (comment).

CI checks job <continuous-testing-umb-listener-for-intersmash-pr-checks#145> FAILED:

  • Intersmash integration tests job <eap-8.x-intersmash-integration-tests-products-jboss-eap#84> reported failures.

  • Intersmash integration tests job <eap-8.x-intersmash-integration-tests-products-jboss-eap-xp#93> reported failures.

    Please verify your changes locally or reach out to maintainers in order to investigate the issues.

@fabiobrz
Copy link
Member Author

Intersmash PR CI check results

Hi @fabiobrz, this comment is meant to report the outcome of Intersmash integration tests, which were triggered by @fabiobrz comment, see #206 (comment).

CI checks job <continuous-testing-umb-listener-for-intersmash-pr-checks#145> FAILED:

* Intersmash integration tests job <_eap-8.x-intersmash-integration-tests-products-jboss-eap#84_> reported failures.

<eap-8.x-intersmash-integration-tests-products-jboss-eap#84> reported 1 failure:

cz.xtf.core.waiting.WaiterException: Waiting for exactly 1 pods with label app.kubernetes.io/instance=wildfly-helm-helloworld-qs to be ready.
	at org.jboss.intersmash.testsuite.provision.openshift.WildflyHelmChartExistingValuesProvisionerTest.basicProvisioningTest(WildflyHelmChartExistingValuesProvisionerTest.java:50)

which is an unrelated random issue, reported as fixed by the subsequent run: <eap-8.x-intersmash-integration-tests-products-jboss-eap#85> ✅

* Intersmash integration tests job <_eap-8.x-intersmash-integration-tests-products-jboss-eap-xp#93_> reported failures.

<eap-8.x-intersmash-integration-tests-products-jboss-eap-xp#93> was actually a failed build, restarted as <eap-8.x-intersmash-integration-tests-products-jboss-eap-xp#94>, where we had only 1 failure:

[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.087 s <<< FAILURE! - in org.jboss.intersmash.testsuite.provision.openshift.operator.OperatorSubscriptionTestCase
[ERROR] org.jboss.intersmash.testsuite.provision.openshift.operator.OperatorSubscriptionTestCase  Time elapsed: 1.087 s  <<< ERROR!
io.fabric8.kubernetes.client.KubernetesClientException: Failure executing: DELETE at
...

which is an intermittent one, due to cluster instability. ✅

Based on the above findings, the changes in the PR are deemed to pass the integration tests and can now be reviewed.

…es in order to introduce Olm based provisioning, and generic operator provisioners. Also fixing the testsuite modules accordingly
Copy link
Collaborator

@tommaso-borgato tommaso-borgato left a comment

Choose a reason for hiding this comment

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

LGTM, I just asked one question ....

@@ -1,2 +1 @@
cz.xtf.junit5.listeners.TestExecutionLogger
cz.xtf.junit5.listeners.ProjectCreator
Copy link
Collaborator

Choose a reason for hiding this comment

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

@fabiobrz I didn't understand why we had to introduce it here ... I believed openshift project creation was already taken care ....

Copy link
Member Author

@fabiobrz fabiobrz Nov 22, 2024

Choose a reason for hiding this comment

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

Thanks for your question @tommaso-borgato
I removed the class from the SPI registration because we can only use it for tests that are expected to run on OpenShift.

Previously we had it in here because we knew we were running only on OpenShift, and project creation was expected to happen anyway.

This is why now we let specific tests implement the two different interfaces for project (OCP) or namespace (k8s) creation.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 22, 2024
@tommaso-borgato tommaso-borgato merged commit c160b26 into Intersmash:main Nov 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment