OPRUN-3692: Olmv1-catalogd tests for API endpoints #29580
+337
−5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci-robot
added
the
jira/valid-reference
|
@anik120: This pull request references OPRUN-3692 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 story to target the "4.19.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
anik120
commented
Mar 5, 2025
test/extended/olm/olmv1.go
Outdated
| } | ||
| }) | ||
|
|
||
| g.It("[OCPFeatureGate:NewOLMCatalogdAPIV1Metas] should serve the /v1/api/metas API endpoint", func(ctx g.SpecContext) { |
There was a problem hiding this comment.
Is this valid? Can [OCPFeatureGate:NewOLMCatalogdAPIV1Metas] be added to an It block?
There was a problem hiding this comment.
No, you should put it in it's own g.Describe()
Yes, you'll possibly duplicate a bunch of stuff, but that's OK.
There was a problem hiding this comment.
You can put each of these g.It() you've added as their own g.Describe() so they count as new tests... since you need to have 5 for each Feature Gate.
anik120
force-pushed
the
catalogd-metas-tests
branch
2 times, most recently
from
c09fb4d to
902037a
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anik120 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 |
joelanford
reviewed
Mar 5, 2025
joelanford
reviewed
Mar 5, 2025
test/extended/olm/olmv1.go
Outdated
| func verifyAPIEndpoint(ctx g.SpecContext, urlPath string) { | ||
| var lastError error | ||
|
|
||
| // Retry API call a few times to handle potential connection issues |
There was a problem hiding this comment.
I feel like we should not be allowing ourselves to retry. IIUC, this would mean that as long as our endpoint worked 1/15 times, this test would pass.
Maybe instead, we should implement a load test with 100 concurrent requests and verify:
- 100% response rate with 200 status code (for valid requests)
- Some reasonable 99 percentile response time threshold check.
There was a problem hiding this comment.
Looking into writing a load test now.
There was a problem hiding this comment.
There's also no retry in the new Job, it's a one and done operation after which the result is checked with a timeout (exit 0/exit 1)
joelanford
reviewed
Mar 5, 2025
| verifyAPIEndpoint(ctx, "https://localhost:8443/catalogs/openshift-community-operators/api/v1/metas?schema=olm.package") | ||
| }) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Is there a place to implement upgrade interruption tests?
Another useful test to add (but not specific to the new metas feature gate):
- Start a loop that on every iteration: gets a ClusterCatalog's status.URLs value and makes a query to a valid endpoint under that URL.
- Do the upgrade
The expectation would be that the loop begins to get connection errors or 404 responses as the new pod starts up, but within a certain threshold (1m?) begins responding with 200 responses again.
There was a problem hiding this comment.
By upgrade do you mean openshift upgrade?
Cursory look did not reveal anything of that nature in this repository, I'll have to research a little more. I know openshift does upgrade tests, I'm not sure if those tests are written in this repository.
joelanford
reviewed
Mar 5, 2025
test/extended/olm/olmv1.go
Outdated
Comment on lines
115
to
116
| // Wait for port-forward to be established | ||
| time.Sleep(2 * time.Second) |
There was a problem hiding this comment.
The port-forward isn't ready by the time the oc.Run("port-forward") command returns?
There was a problem hiding this comment.
I was just being extra cautious
joelanford
reviewed
Mar 5, 2025
test/extended/olm/olmv1.go
Outdated
Comment on lines
111
to
119
| // Start port-forward in background | ||
| _, _, _, err := oc.AsAdmin().Run("port-forward").Args("-n", "openshift-catalogd", "svc/catalogd-service", "8080:443").Background() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
I could imagine port-forward hiding issues that in-cluster clients would encounter (e.g. in-cluster DNS).
Doing a port-forward based test is probably a good thing to do if that is functionality we expect to work, but I also think we should run a Job workload that can perform a similar test using cluster networking.
There was a problem hiding this comment.
Yup a Job is actually perfect for this use case, and that totally slipped my mind 😮💨 updated the PR to use a Job instead of port forwarding
joelanford
reviewed
Mar 5, 2025
test/extended/olm/olmv1.go
Outdated
| func(ctx context.Context) (bool, error) { | ||
| // Create a custom HTTP client that skips TLS verification | ||
| tr := &http.Transport{ | ||
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, |
There was a problem hiding this comment.
We should not skip TLS verification. We expect real clients to connect using a trusted CA chain, so our test should use that same CAs as we expect our clients to use.
There was a problem hiding this comment.
Using a job and curling the http endpoint directly now 👍🏽
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 902037a
|
anik120
force-pushed
the
catalogd-metas-tests
branch
3 times, most recently
from
6dbc1d4 to
d02be06
Compare
anik120
commented
Mar 6, 2025
anik120
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
joelanford
reviewed
Mar 6, 2025
test/extended/olm/olmv1.go
Outdated
Comment on lines
142
to
264
| for range 100 { | ||
| wg.Add(1) | ||
| go func(catalogIdx int) { | ||
| defer wg.Done() | ||
|
|
||
| g.By(fmt.Sprintf("Testing api/v1/all endpoint for catalog %s", providedCatalogs[catalogIdx])) | ||
| verifyAPIEndpoint(ctx, oc, oc.Namespace(), providedCatalogs[catalogIdx], "all") | ||
| }(idx) | ||
| idx = (idx + 1) % len(providedCatalogs) | ||
| } |
There was a problem hiding this comment.
I'm having a really hard time grokking the way the index is working here. For clarity, can we change this to something like:
var wg sync.WaitGroup
for _, catalog := range providedCatalogs {
for range 100 {
wg.Add(1)
go func() {
verifyAPIEndpoint(ctx, oc, oc.Namespace(), catalog, "all")
}()
}
}
wg.Wait()There was a problem hiding this comment.
Also, I don't think you need separate Jobs. Might take some experimentation, but you may be able to set
spec:
activeDeadlineSeconds: 30 # how long the job is allowed to be active
backoffLimit: 0 # don't retry failed pods
completions: 100 # required 100 completions
maxFailedIndices: 0 # allow 0 failures
parallelism: 100 # run 100 pods at a time
ttlSecondsAfterFinished: 10 # make the job eligible for deletion 10 seconds after it finishes
joelanford
reviewed
Mar 6, 2025
test/extended/olm/olmv1.go
Outdated
|
|
||
| // verifyAPIEndpoint runs a job to validate the given service endpoint of a ClusterCatalog | ||
| func verifyAPIEndpoint(ctx g.SpecContext, oc *exutil.CLI, namespace, catalogName, endpoint string) { | ||
| jobName := fmt.Sprintf("test-catalog-%s-%s-%s", catalogName, endpoint, rand.String(5)) |
There was a problem hiding this comment.
I see metas?schema=olm.package being passed as endpoint and then used in the job name. Seems like that's an invalid name? Maybe pass url.Params as a separate argument in the function?
joelanford
reviewed
Mar 6, 2025
test/extended/olm/olmv1.go
Outdated
Comment on lines
568
to
702
| if job.Status.Succeeded > 0 { | ||
| return true, nil | ||
| } | ||
| if job.Status.Failed > 0 { | ||
| return false, fmt.Errorf("job failed") | ||
| } |
There was a problem hiding this comment.
Looking at the Job conditions, it seems like we can check for type: Complete (success) or type: Failed(failed). Maybe we should also consider looking at the difference between status.completionTime and status.startTime for reporting/checking duration.
joelanford
reviewed
Mar 6, 2025
test/extended/olm/olmv1.go
Outdated
Comment on lines
534
to
548
| set -ex | ||
| response=$(curl -s -k "%s" || echo "ERROR: Failed to access endpoint") | ||
| if [[ "$response" == ERROR* ]]; then | ||
| echo "$response" | ||
| exit 1 | ||
| fi | ||
| echo "$response" > /tmp/api-response | ||
|
|
||
| # check if response can be parsed as new line delimited JSON | ||
| if cat /tmp/api-response | jq -s . > /dev/null 2>&1; then | ||
| echo "Valid JSON response detected" | ||
| exit 0 | ||
| fi | ||
| echo "ERROR: Invalid JSON response" | ||
| exit 1 |
There was a problem hiding this comment.
This makes just a single request per pod, which probably isn't quite as efficient as if we could use an http load testing program to make a bunch of concurrent requests from a single process.
Not sure where to find an image with an http load tester available to the origin suite, but if we can find one, it would likely provide much more useful information (e.g. histogram of durations of the requests)
|
Job Failure Risk Analysis for sha: d02be06
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d02be06
New tests seen in this PR at sha: d02be06
|
anik120
force-pushed
the
catalogd-metas-tests
branch
from
d02be06 to
c93d38e
Compare
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: c93d38e
New tests seen in this PR at sha: c93d38e
|
anik120
force-pushed
the
catalogd-metas-tests
branch
from
c93d38e to
0b6c1b6
Compare
anik120
force-pushed
the
catalogd-metas-tests
branch
from
08114fc to
f6fe874
Compare
|
Job Failure Risk Analysis for sha: f6fe874
Showing 20 of 44 jobs analysis Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: f6fe874
New tests seen in this PR at sha: f6fe874
|
anik120
force-pushed
the
catalogd-metas-tests
branch
2 times, most recently
from
1329bee to
4b625df
Compare
|
@joelanford cleaned up the Lua script and results look much better now: Main highlight from the logs above: |
anik120
force-pushed
the
catalogd-metas-tests
branch
3 times, most recently
from
651a15d to
d85d7ea
Compare
|
/retest-required |
|
The test "[sig-olmv1][OCPFeatureGate:NewOLM][Skipped:Disconnected] OLMv1 Catalogs API Load Test all endpoint should handle concurrent load with acceptable performance on api/v1/all endpoint [Suite:openshift/conformance/parallel]" passed when I ran it manually: retesting /retest-required |
|
Job Failure Risk Analysis for sha: d85d7ea
Showing 20 of 33 jobs analysis Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d85d7ea
New tests seen in this PR at sha: d85d7ea
|
|
/retest |
anik120
force-pushed
the
catalogd-metas-tests
branch
from
d85d7ea to
a5ffb16
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
openshift-merge-robot
added
the
needs-rebase
|
Job Failure Risk Analysis for sha: a5ffb16
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: a5ffb16
New tests seen in this PR at sha: a5ffb16
|
|
/retest-required |
|
/test ci/prow/e2e-aws-ovn-edge-zones |
|
@anik120: 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. |
|
/test e2e-aws-ovn-edge-zones |
Introduces tests for the new `api/v1/metas` endpoint when NewOLMCatalogdAPIV1Metas feature gate in enabled. Signed-off-by: Anik Bhattacharjee <[email protected]>
anik120
force-pushed
the
catalogd-metas-tests
branch
from
a5ffb16 to
9fc20f4
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@anik120: 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: 9fc20f4
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 9fc20f4
New tests seen in this PR at sha: 9fc20f4
|
|
/retest |
2 similar comments
|
/retest |
|
/retest |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduces tests for the new
api/v1/metasendpoint when NewOLMCatalogdAPIV1Metas feature gate in enabled.