-
Notifications
You must be signed in to change notification settings - Fork 33
OPRUN-3964: OTE add first test from openshift/origin olmv1.go #415
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
OPRUN-3964: OTE add first test from openshift/origin olmv1.go #415
Conversation
@camilamacedo86: This pull request references OPRUN-3983 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
bd38fcc
to
67e2139
Compare
@camilamacedo86: This pull request references OPRUN-3983 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
@camilamacedo86: This pull request references OPRUN-3983 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
@camilamacedo86: This pull request references OPRUN-3983 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
@camilamacedo86: This pull request references OPRUN-3983 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
} | ||
] | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kuiwang02 It’s totally fine to add this in a follow-up, but I’d like to start documenting a comprehensive guide here on how to run the tests locally using openshift-bot.
Ideally something like:
- Step 1
- Step 2
- Step 3
I haven’t tried it yet, but from what I understand, we need to:
- Create the OCP instance with the PR changes
- Set KUBECONFIG to point to that OCP cluster
- Then run the tests against it
If you know of any tricks or caveats—for example, how to enable OLMv1 with TechPreview features using openshift-bot—I’d really appreciate your guidance.
Once I try it locally, I’ll create a follow-up PR with the detailed steps.
Thanks! 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86
actually whether the cluster is created by kind, cluster-bot, and installation step of openshift CI, as long as there is KUBECONFIG file, it is same to run case (for kind cluster, need to manually enable TP if needed).
for example,
1, cluster created by kind: you will get KUBECONFIG. and you enable TP manually. and then you set KUBECONFIG and test case.
2, cluster created by cluster-bot (I think openshift-bot you said is cluster-bot): when you create it with launch
with variant techpreview
, like launch openshift/operator-framework-operator-controller#393,4.20 aws,techpreview
, the TP is already enabled if the cluster is created successfully. and then you set KUBECONFIG and test case. no need to enable TP manually before testing case.
3, cluster created by installation step of openshift CI, (like openshift-e2e-aws): after you set env FEATURE_SET: TechPreviewNoUpgrade
in configuration, the TP is ready enabled after the cluster is created sucessfully. and then you set KUEBECONFG and test case. no need to enable TP manually before testing casee.
@camilamacedo86: This pull request references OPRUN-3983 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
@camilamacedo86: This pull request references OPRUN-3983 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
// | ||
// References: | ||
// - https://github.com/openshift/origin/blob/main/pkg/cmd/openshift-tests/run/flags.go#L53 | ||
// - https://github.com/openshift/origin/blob/main/pkg/clioptions/clusterdiscovery/provider.go#L100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We’re initialising the suite directly.
WHY?
- If a test doesn’t require a cluster (e.g., sanity checks or validations that run outside the cluster), we skip environment initialisation.
- Most Important If initialising the env globally here for ALL it can cause issues for [parallel] tests. It would result in a shared config across all tests, but we want isolated configs per test. This avoids conflicts when tests are run in parallel across different instances or clusters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86
the OTE suggests this way https://github.com/openshift-eng/openshift-tests-extension/blob/main/cmd/example-tests/main.go#L64-L66.
Because your code has
func Init() *TestEnv {
if testEnv == nil {
testEnv = initTestEnv()
}
return testEnv
}
to avoid to init cluster repeatedly
so, I do not find side effective with initing it in BeforeEach, not taking the way suggested by OTE.
So, I think it is ok. and we can go in this way to monitor if we meet issue late.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done the change since it makes it easier for us to develop.
Less one thing to remmber and will do the same :-)
} | ||
} | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same test done in : https://github.com/openshift/origin/blob/48d56f2e5bb8bcf6c4560fb077c7b809e18a5c19/test/extended/olm/olmv1.go#L34-L74
(Migration - After we migrate for here and check that all is fine with Sippy we can remove from opc/origin)
if !env.Get().IsOpenShift { | ||
extlogs.Warn("Skipping feature capability check: not OpenShift") | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: If OLMv1 is installed locally in KinD, the same test will run just fine.
We’re only checking if the CRDs are present, which works outside OpenShift.
The capability check (e.g. "OperatorLifecycleManagerV1") is specific to OpenShift
and only required in OpenShift CI.
So now, we skip that check when running on non-OCP clusters.
This allows us to run and validate the test locally, making development easier and faster before pushing the PR.
This is the same logic used here:
https://github.com/openshift/origin/blob/48d56f2e5bb8bcf6c4560fb077c7b809e18a5c19/test/extended/olm/olmv1.go#L321-L327
@camilamacedo86: This pull request references OPRUN-3983 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
utilruntime.Must(configv1.AddToScheme(scheme)) | ||
} | ||
|
||
k8sClient, err := crclient.New(cfg, crclient.Options{Scheme: scheme}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don’t need to use RunCommand("oc") as done in https://github.com/openshift/origin/blob/48d56f2e5bb8bcf6c4560fb077c7b809e18a5c19/test/extended/util/client.go#L165-L193 and ALL that complixities.
Instead, we can use the controller-runtime client, just like we do in our e2e tests, to interact with the cluster.
This approach helps:
- Keep tests compatible with both oc and Kubernetes
- Simplify the codebase
- Avoid depending on CLI tooling
For reference, see how we do our e2e tests:
https://github.com/openshift/operator-framework-operator-controller/tree/main/test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kuiwang02
It works like our current e2e tests and keeps things simple.
If we find any test that really needs something different, we’ll handle it case by case.
Let’s keep it simple (KISS) and only add what’s truly needed for those special cases.
That way, we avoid unnecessary complexity and keep our setup clean.
If we try to add everything we might need up front, we risk falling into the premature optimization trap.
That often leads to unnecessary complexity and code that's harder to maintain — without actually solving real problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86
1, For all QE cases, we need oc client (it seems olmv1 origin case does not need it), like https://github.com/openshift/origin/blob/48d56f2e5bb8bcf6c4560fb077c7b809e18a5c19/test/extended/util/client.go#L165-L193. currently you could not care it because you are migrate olmv1 origin case. when we migrate QE cases, I will think about it.
2, For all QE cases, it depends on k8s.io/kubernetes/test/e2e/framework (it seems olmv1 origin case does not need it). it also need init when initing cluster. when we migrate QE cases, I will think about it.
So, as summary the current code on it is ok to migrate olmv1 origin cases. you could go on for your work.
I will care about it late for QE cases migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1, For all QE cases, we need oc client
Currently, we use oc/kubectl by calling them from the command line and parsing the output manually.
The change here is to use the Kubernetes API directly instead.
If needed, we can still call the CLI like in this example:
https://github.com/operator-framework/operator-controller/blob/main/test/utils/utils.go#L11-L23
—but using the API gives us the same result, in a cleaner way.
k8s.io/kubernetes/test/e2e/framework
We’ll need to review the specific cases as we migrate them, but from what I’ve seen so far across the repos, there’s a lot of complexity that might not be necessary.
It looks like we have multiple ways to achieve the same outcomes. I see this migration as a good opportunity to revisit the tests being added, reduce duplication, avoid unnecessary complexity, and make sure we have solid logging in place to help us debug when errors occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86
Frankly, QE test cases are far more complex than the origin OLMv1 cases. They’re similar to the complex cases in other Origin modules that rely on Origin’s NewCLI, which in turn depends on k8s.io/kubernetes/test/e2e/framework and various other initializations.
Because the Origin OLMv1 cases are relatively simple, the cluster initialization work you’ve done so far is sufficient—but it may only apply to those simple OLMv1 cases. When I eventually migrate the QE cases, I’ll revisit and adapt the initialization logic; that work will take quite some time. Right now I’m tied up with other priorities and don’t have the bandwidth to tackle it.
Fortunately, your current work can proceed as planned.
Therefore:
/lgtm
And maybe I’ll adjust the init logic when migrating the QE cases in the future.
Thanks
// The default limits (QPS=5, Burst=10) are too low for most test workloads. | ||
func configureQPS(cfg *rest.Config) *rest.Config { | ||
cfg.QPS = 10000 | ||
cfg.Burst = 10000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See we are using the same values
func configureQPS(cfg *rest.Config) *rest.Config { | ||
cfg.QPS = 10000 | ||
cfg.Burst = 10000 | ||
cfg.RateLimiter = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See that ocp/origin client utils will create a TOM of complexities, using code accross diff repos to create a fakeRateLimiter that return nil at the end :
4af8314
to
82ffddd
Compare
/test okd-scos-e2e-aws-ovn |
@camilamacedo86: This pull request references OPRUN-3983 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
@camilamacedo86: This pull request references OPRUN-3983 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
82ffddd
to
8d4e0db
Compare
@camilamacedo86: This pull request references OPRUN-3983 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
}, | ||
"source": "openshift:payload:olmv1", | ||
"lifecycle": "blocking", | ||
"environmentSelector": {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/acknowledge-critical-fixes-only ^ we are not in code freeze yet. To ensure that all is fine. |
/acknowledge-critical-fixes-only |
8d4e0db
to
cf04ac0
Compare
/test openshift-e2e-aws-techpreview |
/test openshift-e2e-aws |
@camilamacedo86: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
@camilamacedo86: This pull request references OPRUN-3964 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1b06fc6
into
openshift:main
[ART PR BUILD NOTIFIER] Distgit: ose-olm-operator-controller |
[ART PR BUILD NOTIFIER] Distgit: ose-olm-catalogd |
This PR adds the setup to initialise the test environment and access the cluster config.
It also includes the first test migrated from OpenShift/Origin (more will be added in follow-up PRs; I want to ensure that we have a good base before migrating all).
We added
extlogs
to improve test logs. It uses Ginkgo’s writer to show clear info and warnings,which helps when tests fail. Since we will need to migrate the tests, it is an excellent opportunity to ensure that we have good logs to cover the scenarios.
The setup utilises the controller-runtime client, similar to our E2E tests.
This lets us run the tests locally with KinD (with OLMv1 installed) or in CI on OpenShift.
Only a few parts are OCP-specific; we have only 3/4 of the features specific to OCP, while the rest is the same as used in upstream. Therefore, most tests can run using Kind and locally, which facilitates the development and troubleshooting process. Furthermore, my vision for the future is to explore the possibility of reusing the tests created for our e2e tests within our organisation, rather than duplicating staff efforts
This makes writing and running tests for OCP much easier and faster.
c/c @tmshort @kuiwang02 @dtfranz @anik120 @grokspawn