USHIFT-3455: Skip legacy-etcd monitor tests in microshift #28874
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pacevedom: This pull request references USHIFT-3455 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.17.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. |
|
/cc @dgoodwin |
| } | ||
| return w.notSupportedReason | ||
| } | ||
|
|
||
| jobType, err := platformidentification.GetJobType(ctx, adminRESTConfig) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to determine job type: %v", err) |
There was a problem hiding this comment.
Looking at how w.jobType is actually used, it's just to temporarily adjust a threshold for vsphere which is struggling. This seems too thin a reason to permanently kill off several tests which help identify that etcd is struggling.
I'd suggest a safer route would be to modify this to not return an error here, set a default of an empty JobType{} with an empty string Platform.
There was a problem hiding this comment.
So that would certainly do, but I wonder if these tests are meaningful in MicroShift. We do not run etcd in a pod, and we have no events because of that (which is what all of these tests check).
Does it make sense to use a fake empty platform (or legit, depends on how you look at it!) only to know that any etcd test afterwards will check on non-existing events?
There was a problem hiding this comment.
Looks like about 5 tests are in here, 3 involve pod logs, so it sounds like those are irrelvant, the other is install related, which I'm guessing is not relevant to you either then, and lastly the clusteroperator state, do you have an etcd cluster operator in microshift? If not, then I would just add a comment along side your disable explaning why and we can merge this as is.
There was a problem hiding this comment.
We dont have etcd operator so I guess I will add the comment explaining why we can safely skip all tests here. Thanks!
|
@pacevedom: 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: c272c01
|
|
Perfect that's helpful for anyone outside microshift I think. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, pacevedom 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 |
openshift-ci
bot
added
the
approved
openshift-merge-bot
bot
merged commit cea3f67
into
openshift:master
|
[ART PR BUILD NOTIFIER] This PR has been included in build openshift-enterprise-tests-container-v4.17.0-202406281412.p0.gcea3f67.assembly.stream.el9 for distgit openshift-enterprise-tests. |
No description provided.