sync prometheus ext tests running in parallel #17717
Conversation
openshift-ci-robot
added
the
size/S
|
/lgtm |
openshift-ci-robot
added
lgtm
|
/test cmd |
|
conformance install failure flake #17605 /test extended_conformance_install |
| func bringUpPrometheusFromTemplate(oc *exutil.CLI) (ns, host, bearerToken string, statsPort int) { | ||
| ns = oc.KubeFramework().Namespace.Name | ||
| host = "prometheus.kube-system.svc" | ||
| statsPort = 443 | ||
| mustCreate := false | ||
| mux.Lock() |
There was a problem hiding this comment.
Won't work, other jobs are run in separate processes.
There was a problem hiding this comment.
Ah ... gotcha.
So seems like we need to interpret the error, and if it is "already exists", do not abort the test case.
I'll circle back and give that a go (unless some other direction is supplied).
There was a problem hiding this comment.
I'll see if I can remove the lgtm label
/hold
There was a problem hiding this comment.
appears I can :-)
openshift-ci-robot
added
the
do-not-merge/hold
gabemontero
force-pushed
the
check-for-sa-prom-ext
branch
from
100628c
to
16d25f8
Compare
openshift-ci-robot
added
size/XS
openshift-merge-robot
removed
the
lgtm
|
adjustment pushed ... fyi, totally removed the mutex (seemed unnecessary given the multi-process basis) |
gabemontero
removed
the
do-not-merge/hold
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| // still check if it exists, as the prior not found check above may have been in a race with a test | ||
| // running in another process | ||
| if !kapierrs.IsAlreadyExists(err) { |
There was a problem hiding this comment.
you're not making an api call here, you're effectively shelling out to invoke "oc create -f", so this error check is not going to work.
There was a problem hiding this comment.
you may have to literally scan the output from the command. Or just log the error w/o failing the test (so if the test ultimately fails, we can at least go back and discover the error that occurred)
There was a problem hiding this comment.
I'll go with the latter ... the existing use of IsNotFound presumably also falls into this bucket
There was a problem hiding this comment.
The existing call is not using the oc client:
if _, err := oc.AdminKubeClient().Apps().StatefulSets("kube-system").Get("prometheus", metav1.GetOptions{}); err != nil {
if !kapierrs.IsNotFound(err) {
it's using the KubeClient (aka the API client)
so it's fine.
You could consider doing the same thing here.
There was a problem hiding this comment.
I've been exploring the various client entryways in cli.go but am not finding a verb that allows me to "process" the template
I'll look a bit more but unless you have a precise pointer, thinking of falling back to parsing output.
And thanks for the clarification around the not found path.
There was a problem hiding this comment.
you don't have to change the process logic, just the create logic.
There was a problem hiding this comment.
but honestly this should fail so rarely i'd be fine w/ you just logging the error so if the test fails further down, we have the error on record.
|
the error in https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/17717/test_pull_request_origin_extended_conformance_install/3954/ |
gabemontero
force-pushed
the
check-for-sa-prom-ext
branch
from
16d25f8
to
0269f40
Compare
|
update pushed @bparees |
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
|
/test extended_conformance_install |
openshift-merge-robot
added
the
needs-rebase
gabemontero
force-pushed
the
check-for-sa-prom-ext
branch
from
0269f40
to
9b9f68d
Compare
openshift-merge-robot
removed
the
lgtm
openshift-merge-robot
removed
the
needs-rebase
|
rebase pushed ... @bparees please re-review / re-post the good to merge comment at your convenience |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, gabemontero The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Opened #17811 for the cmd failure. |
|
conformance gce was my flake #17694 hopefully the debug data PR merged in time for it to capture data for this one. UPDATE: unfortunately the timing was not in our favor. The debug PR merge must have happened after this job started ... no intermediate |
|
conformance install was pre-test, env failures in origin prereqs and install origin |
|
/retest |
|
conformance install failure was flake #17556 |
|
/retest |
|
opened and taking flake #17836 |
|
/test extended_conformance_gce |
|
Automatic merge from submit-queue (batch tested with PRs 16281, 17293, 17717, 17753, 17830). |
Fixes #17693
@bparees @smarterclayton @mfojtik fyi