USHIFT-741: MicroShift invariant adaptations #27989
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@chiragkyal: This pull request references USHIFT-741 which is a valid jira issue. 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/test-infra repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
chiragkyal
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chiragkyal 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
do-not-merge/work-in-progress
|
/retest |
1 similar comment
|
/retest |
|
@chiragkyal: 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. |
|
/assign ingvagabund |
|
@chiragkyal: This pull request references USHIFT-741 which is a valid jira issue. 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/test-infra repository. |
|
/cc @copejon |
|
Changes here seem good @chiragkyal |
@copejon could you please add the label. Thanks! |
|
/lgtm |
|
/assign @soltysh |
| if err != nil { | ||
| panic(err) | ||
| } | ||
| isMicroShift, err := exutil.IsMicroShiftCluster(kubeClient) |
There was a problem hiding this comment.
Instead of giving empty list of junit test cases, we should ask ourselves whether we need to run testServerAvailability at all. Checking the code:
$ grep -rn "testServerAvailability"
pkg/synthetictests/disruption.go:37:func testServerAvailability(
pkg/synthetictests/disruption.go:190: ret = append(ret, testServerAvailability("sig-network-edge", locator, allDisruptionEventsIntervals, jobRunDuration, jobType)...)
pkg/synthetictests/disruption.go:209: ret = append(ret, testServerAvailability("sig-trt", locator, allDisruptionEventsIntervals, jobRunDuration, jobType)...)which leads to https://github.com/openshift/origin/blob/master/pkg/synthetictests/event_junits.go#L38-L39:
tests = append(tests, TestAllIngressBackendsForDisruption(events, duration, jobType)...)
tests = append(tests, TestExternalBackendsForDisruption(events, duration, jobType)...)Question here is whether it is safe to skip TestAllIngressBackendsForDisruption and TestExternalBackendsForDisruption? If the answer is not, then instead of checking whether an installation is MicroShift one should check whether the ingress/externalbackends are present.
There was a problem hiding this comment.
All these disruptions depend upon JobType and for MicroShift this will always be nil. So, I think we don't want to run any of these disruption invariants for MicroShift.
And to make this happen, I see the possible way is either call IsMicroShiftCluster() inside testServerAvailability and return an empty list of junit test cases (which I've done above), or
not to append the following to tests based on the same IsMicroShiftCluster() check inside event_junits.go
tests = append(tests, TestAllAPIBackendsForDisruption(events, duration, jobType)...)
tests = append(tests, TestAllIngressBackendsForDisruption(events, duration, jobType)...)
tests = append(tests, TestExternalBackendsForDisruption(events, duration, jobType)...)/cc @pacevedom @dhellmann let me know your thoughts.
|
|
| panic(err) | ||
| } | ||
| if !nsExist { | ||
| return []*junitapi.JUnitTestCase{} |
There was a problem hiding this comment.
Better to avoid invoking testAlerts completely in https://github.com/openshift/origin/blob/master/pkg/synthetictests/event_junits.go#L54-L55. Question here to ask is whether there's a sufficient replacement for absence of the monitoring stack when checking the alerts in MicroShift. Does it make sense to e.g. run an external monitoring stack (or its simplified version)?
There was a problem hiding this comment.
Better to avoid invoking testAlerts completely in https://github.com/openshift/origin/blob/master/pkg/synthetictests/event_junits.go#L54-L55.
Based on the existence of "openshift-monitoring" ns right? So, IIUC the suggestion is to add this above check inside event_junits.go ?
Question here to ask is whether there's a sufficient replacement for absence of the monitoring stack when checking the alerts in MicroShift. Does it make sense to e.g. run an external monitoring stack (or its simplified version)?
AFAIK there is no plan to add any monitoring stack in MicroShift as of now. So, we might want to skip them as well.
|
In the last 1-2 days, a lot of refactoring was done through #28130 PR, and now I believe the changes suggested above will not hold true. @pacevedom should we consider accommodating these changes in your PR? |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
Included in #28136 |
|
/close |
|
@chiragkyal: Closed this PR. 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/test-infra repository. |
This change will skip alert invariant if
"openshift-monitoring"namespace does not exist. It will also skip disruption invariant if it's a MicroShift cluster.USHIFT-741