MON-3775: [WIP] add tests for CMO collection profiles #28768
Conversation
|
@rexagod: This pull request references MON-3775 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 task to target the "4.16.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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rexagod 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 |
|
@rexagod: This pull request references MON-3775 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 task to target the "4.16.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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-merge-robot
removed
the
needs-rebase
rexagod
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-merge-robot
added
the
needs-rebase
|
Job Failure Risk Analysis for sha: e449b02
|
openshift-merge-robot
removed
the
needs-rebase
|
/assign @simonpasquier |
|
Job Failure Risk Analysis for sha: dba0cc9
|
| } | ||
|
|
||
| func (r *runner) makeCollectionProfileConfigurationFor(ctx context.Context, collectionProfile string) error { | ||
| // Create a configuration for the operator based on the current profile. |
There was a problem hiding this comment.
The problem is that e2e tests usually come with a specific cluster monitoring configuration. Overwriting it isn't an option.
There was a problem hiding this comment.
So CMO's configuration is reconciled by some other operator and cannot be modified? Does this mean we cannot enable collection profiles at all (without hard-changing the configuration at source)?
There was a problem hiding this comment.
Perhaps those checks that require configuration changes would be better suited as part of the CMO tests themselves.
Maybe we should only keep the ones that don't require modifying the configmap. For example, we could check that profile:cluster_monitoring_operator_collection_profile:max doesn't exist/empty or that a metric that could have been dropped by the minimal or full profiles is still present. If in the future, cProfiles is enabled by default with a profile being set, we can add more checks regarding that profile here.
There was a problem hiding this comment.
Yep, profile:cluster_monitoring_operator_collection_profile:max will exist in a techpreview cluster, and will be 0 at all times for all profiles (by default).
In the future, even if we did set a default profile, we would only ever have checks for that particular profile here, since switching won't be an option. This acts against the lower-bound requirement of at-least five confident tests that should cover the feature in o/origin itself.
If your FeatureGate lacks automated testing, there is an exception process that allows QE to sign off on the promotion by commenting on the PR.
It'd also make sense to add these in CMO instead, as you said, ask for an exception here, and have the QE signoff.
There was a problem hiding this comment.
The CMO config is defined in the test workflow after the cluster is installed but it's not modified afterwards (for now). To have the tests in origin, you'd need to make sure that the original CMO configuration is preserved. E.g. the collection profile tests only modify the collectionProfile field and keep all other content identical.
There was a problem hiding this comment.
(If altering the configmap is indeed possible, I retract my previous statement, of course. We can then consider the other tests/components not failing when the minimal profile is enabled e.g. to be a cProfile test on its own.)
| } | ||
| } | ||
| }) | ||
| g.It("should have expected selectors when a profile is applied", func() { |
There was a problem hiding this comment.
IMHO this test is too tightly coupled with the implementation.
Another option would be to check that the profile:cluster_monitoring_operator_collection_profile:max metric exposes the configured profile (it should be present for both minimal and full profiles). If we want to validate from an end-user perspective, I'd pick a well-known metric which is present with the full profile but not with minimal.
There was a problem hiding this comment.
Ah, agreed! I was hesitant on adding this testcase but I wasn't sure what we can do to hit the 5-test mark, but the metrics approach seems to fit perfectly here while not being too close to the implementation details, since this testcase is itself covered by CMO's test suite.
|
/test verify The CI logs show failure for the job, but it's passing on my local, with the same go version as the project's. I'll jump on the proposed additions once this goes green again. |
|
@rexagod: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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 kubernetes-sigs/prow repository. |
|
I can see an additional The error stems from here, on |
|
Ah, nevermind, it seems my EDIT: This still seems to be the case, looking into it. EDIT: I was able to finally reproduce this by generating everything again. I believe the artifacts generated using |
|
Job Failure Risk Analysis for sha: be65e96
|
rexagod
force-pushed
the
3775
branch
2 times, most recently
from
7e56609
to
92d17c4
Compare
|
Looking at a different CI error, but it seems that the line is already generated? |
rexagod
force-pushed
the
3775
branch
3 times, most recently
from
90810a8
to
b356051
Compare
|
Job Failure Risk Analysis for sha: b356051
|
|
Job Failure Risk Analysis for sha: b356051
|
|
Job Failure Risk Analysis for sha: ade9ab4
|
openshift-merge-robot
added
the
needs-rebase
Refer: openshift/enhancements#1298 Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
openshift-merge-robot
removed
the
needs-rebase
|
/test e2e-gcp-ovn-techpreview |
|
@rexagod: 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. |
|
/test e2e-gcp-ovn-techpreview |
Refer: openshift/enhancements#1298
Signed-off-by: Pranshu Srivastava rexagod@gmail.com