WIP: MON-3513: add resource metrics api availability test #28699
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@slashpai: This pull request references MON-3513 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
added
the
do-not-merge/work-in-progress
slashpai
force-pushed
the
metrics-server
branch
from
a2c5f95 to
03aea86
Compare
openshift-ci
bot
added
the
approved
|
/retest |
|
/test verify |
Signed-off-by: Jayapriya Pai <janantha@redhat.com>
slashpai
force-pushed
the
metrics-server
branch
from
03aea86 to
b28c376
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: slashpai 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 |
openshift-ci
bot
removed
the
approved
| g.By("verifying whether the nodes.metrics command works after the upgrade") | ||
| postUpgradeResponse, err := oc.Run("get").Args("nodes.metrics").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(postUpgradeResponse).NotTo(o.BeEmpty()) |
There was a problem hiding this comment.
Not sure if there are any cases where non-emptiness could equate to an unexpected output (without any errors), but if so, would it be worth checking the contents of the output as well, just to be safe?
There was a problem hiding this comment.
I think that checking for the correctness should happen on oc and/or CMO sides, IIUC the test is only to show that the API continues to work (is responsive/available) after the p-a->m-server switch (which will happen during an upgrade).
We can even get rid of the test in future versions.
If not already done, JP could provoke a failure (putting some nonexisting stuff in Args()) of the test so we get an idea about how it'd look like and to test the test :)
I know the release folks are migrating to the new monitortests framework for upgrade tests: you can see how we use it here https://github.com/openshift/origin/pull/28361/files. But for a temp test, I'm ok with keeping it like this.
There was a problem hiding this comment.
(If you agree with me that it's a temp test, it'd be great to add that as a comment in the test and to have a ticket that will revert it in the future)
There was a problem hiding this comment.
The idea to have the test is to make sure resource metrics api is always available. We didn't have test for that but it was getting tested indirect way for HPA. The test will be still useful even full migration (PA->MS) happened as well. But for now ya we needed a test to prove it will always be functional
We do have other tests as well so would that mean we will need to move all tests using new framework?
There was a problem hiding this comment.
If it's not a temp test, I'd go for the monitortests framework (I could help if you have questions.)
But let's see what the others think.
(I'd be interested in a commit that provokes a failure in both cases though)
There was a problem hiding this comment.
Good remark on monitortests, I'm definitely not up to date with the origin testing 😅
It looks like a good option and the statefulsets example is a good starting point from what I can tell.
There was a problem hiding this comment.
As discussed with JP, we'll go with #28737
|
/retest-required |
|
@slashpai: 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/test-infra repository. I understand the commands that are listed here. |
|
Job Failure Risk Analysis for sha: b28c376
|
|
Closing in favour of #28737 |
openshift/origin e2e tests don't explicitly exercise the metrics.k8s.io API group. We should add tests under the test/extended/prometheus proving that the API works before/after upgrade.
It will help showing off that the prometheus-adpater -> metrics-server migration happens seamlessly when we turn the feature gate on by default.