support component-extension for openshift-tests #29909
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kuiwang02 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Thanks for the contribution --
Are the command line arguments you're adding intended to make it conditional which extension is used in an image? We didn't expect extensions to be used that way, if you want to control what tests are going to get run, you can use suites.
If a binary provides multiple extensions, I'd expect them to all be registered
There was a problem hiding this comment.
I don't fully understand why the images command needs changes, can you clarify this more?
There was a problem hiding this comment.
yes. from selecting and executing case's view, no need to change it for images command.
Here was my thought to change it for image command as well:
both "openshift-tests run" and "openshift-tests images" command involves in external binary.
"openshift-tests run" is related to info/list/run-test interface of the external binary
"openshift-tests images" is related to images interface of the external binary.
I add this argument for "openshift-tests run" and in order to keep consistent, I add it for "openshift-tests images".
In cases the different images for different extension of the binary, it is useful.
if you think it is not necessary, I could remove this part. please help to share your final suggestion. thanks.
pkg/test/extensions/binary.go
Outdated
| @@ -34,6 +34,8 @@ type TestBinary struct { | |||
| // The binary path to extract from the image | |||
| binaryPath string | |||
|
|
|||
| ext string | |||
There was a problem hiding this comment.
s/ext/extension/ please and add a comment when it should be specified
There was a problem hiding this comment.
thanks. update it. please help review it again.
There was a problem hiding this comment.
kuiwang02
force-pushed
the
support-non-default-extension
branch
from
8a36baf to
52ed9a1
Compare
yes. we add this command line argument so that we can set extension for selecting cases. this argument is optional. if we do not set it, it will take default extension (same behavior with before). the binary provides multiple extensions, and we register all them. currently when openshift-tests selects cases from the external binary, it only take "default" extension (the first registered extension). while we want to select cases from other extension, so need this command line argument. for how the openshift-tests select cases from external binary, it take ListTests with list interface without "--component" to get cases. Per List, it take "default" extension and select cases within this extension. even we set testsuite, it will testsuite's qualifiers to filter the case within default extension. hope make me clarified. |
…d run extenal cases besides default extension
kuiwang02
force-pushed
the
support-non-default-extension
branch
from
52ed9a1 to
86b12a6
Compare
|
@kuiwang02: The following tests 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. |
|
Job Failure Risk Analysis for sha: 86b12a6
|
By default when an extension adds tests, its scope is limited to tests only from itself. To do cross-extension test selection with a suite you have to use the AddGlobalSuite helper. #29888 makes the suite test selection work as designed, I'd wait for that to become available. It looks like this is a workaround for suites not being fully implemented yet. All extensions should be registered and examined when considering the tests to run. Only suites are used for test selection, we don't want a second hidden way of filtering tests that's not obvious. |
@stbenjam Thanks for the information #29888. I will learn it. It seems I do not make me cleared so it seems you misunderstand what I want to do. Here I take example to explain my requirement which is the reason why I add this command cli argument: Suppose we have 100 cases in openshift-tests-private repo, and we have to migrate them to component repo based on OTE when openshift-tests-private repo is deprecated. So, we expect 70 cases can be executed by openshift-e2e-test and 30 cases are not. and 30 cases are executed by specific step which still use openshift-tests with this argument. current code from origin (without #29888) and openshift-tests-extension can not support this requirement if we implement 100 cases into only one extension of the external binary (the first one and default) because openshift-tests select cases only from default extension only with environment var and 30 cases is possible be selected. so, my solution is that by the way, even there is no my requirement, it is better to support openshift-tests to select cases from non-default extension of the external binary. how to make my requirement if we do not add this argument? |
support component-extension for openshift-tests so that it can get and run extenal cases except for default extension
as we known, for one component, there is external binary for it. it has one registry and then we register extension in registry.
when we register the first extension, it is also named "default" besides its ID, like"production:type:xxx".
from second extension, it is not named "default" as well and only has its ID.
currently when openshift-tests execute the external cases, it only support to get cases and execute them from "default" extension of the external binary. if we want openshift-tests to execute the cases only from non default extension of the external binary, we need one parameter to tell openshift-tests which extension it need to get cases so that it set "--component=xxx" when openshift-tests call info/images/list/run-test interface of the external binary.
the usage of parameter:
--component-extension=tag1:id1,tag2=id2or--component-extension=tag1:id1 --component-extension=tag2=id2the tag is image stream tag of the component(for example, machine-api-operator), and the id is the extension id when creating extension (for example, openshift:payload:machine-api-operator).
note: for current openshift CI job, no need to any change for the steps which use openshift-tests because it suppose to get cases from default extension and we do not set parameter component-extension.
by the way, why we possible register the second extension?
when we try to migrate the QE cases from openshift-tests-private repo to component repo, the case which is in default extension will be selected and executed by openshift-tests in openshift CI job. it requires the cases stable and less duration.
but we have some cases which can not meet this and we can not drop them, so maybe we create the second extension and put such cases into the second extension once the openshift-tests-private repo is deprecated. and then we could create module owned prow job to use openshift-tests with parameter component-extension to execute these external cases.
(if the openshift-tests-private repo is not deprecated, maybe we do not migrate such case to component repo, and do not need to create second extension and do not need this parameter)
why we do not execute such case with external binary directly in module owned prow job and have to use the openshift-tests?
the run-test/run-suite of the external binary execute the case in one process, and it can only run cases in serial although the case should be ran in parallel. but openshift-tests execute the case in different process, so it could run the case in parallel.
/cc @stbenjam
Hi, you are the expert of OTE. could you please help review it? thanks
if you want more members to review it, please add the members. thanks
(Frankly, I am not sure if it is good and correct solution.)