NE-1970: 5 e2e origin tests for gatewayApiController featureGate#29670
Merged
openshift-merge-bot[bot] merged 3 commits intoopenshift:mainfrom Apr 16, 2025
Merged
NE-1970: 5 e2e origin tests for gatewayApiController featureGate#29670openshift-merge-bot[bot] merged 3 commits intoopenshift:mainfrom
openshift-merge-bot[bot] merged 3 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rhamini3: This pull request references NE-1970 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. DetailsIn 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. |
Contributor
Author
|
/test all |
Contributor
Author
|
/test e2e-gcp-ovn-techpreview |
openshift-merge-robot
added
the
needs-rebase
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 42fbdcb
New tests seen in this PR at sha: 42fbdcb
|
openshift-merge-robot
removed
the
needs-rebase
Contributor
Author
|
/test e2e-gcp-ovn-techpreview |
|
Job Failure Risk Analysis for sha: 657a432
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 657a432
New tests seen in this PR at sha: 657a432
|
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to find default domain name") | ||
| defaultDomain := strings.Split(defaultIngressDomain, "apps.")[1] | ||
|
|
||
| g.By("Create the default API Gateway") |
Contributor
There was a problem hiding this comment.
Replace Create the default API Gateway with Create the default Gateway
| o.Expect(err).NotTo(o.HaveOccurred(), "Gateway %s could not be deleted", name) | ||
| } | ||
|
|
||
| for _, name := range httproutes { |
Contributor
There was a problem hiding this comment.
Normal when the test is over, the namespace is deleted and because of it, the Http route will also get deleted, so do we need to delete the route explicitly ?
Contributor
Author
There was a problem hiding this comment.
good point it is just a safety net in case, if the route isn't there itll skip over it
Contributor
There was a problem hiding this comment.
Actually, seems like it would stop with an error if the route isn't there.
o.Expect(err).NotTo(o.HaveOccurred(), "HttpRoute %s could not be deleted", name)
Contributor
Author
There was a problem hiding this comment.
it seems like it doesn't fail though, so does that mean the namespace is deleted after the httproute is deleted?
https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/29670/pull-ci-openshift-origin-main-e2e-gcp-ovn-techpreview/1910373575034933248
| gwapiClient := gatewayapiclientset.NewForConfigOrDie(oc.AdminConfig()) | ||
| namespace := oc.Namespace() | ||
| g.By("Cleaning up the GatewayAPI Objects") | ||
| for _, name := range gateways { |
Contributor
There was a problem hiding this comment.
if we are deleting the gateway as a cleanup, why aren't we not deleting the gateway class
Contributor
Author
There was a problem hiding this comment.
since gatewayclass is common to all tests and is related to OSSM resources, better to keep it
| g.By("Create the http route using the custom gateway") | ||
| httproutes = append(httproutes, "test-httproute") | ||
| defaultRoutename := "test-hostname.gwapi." + defaultDomain | ||
| createHttpRoute(oc, gw, "test-httproute", defaultRoutename, "echo-pod-"+gw) |
Contributor
There was a problem hiding this comment.
the order is wrong
createHttpRoute(oc, "test-httproute", defaultRoutename, gw, "echo-pod-"+gw)
Contributor
Author
There was a problem hiding this comment.
i think the function is (oc *exutil.CLI, gwName, routeName, hostname, backendRefname string)
Contributor
Author
|
/test e2e-gcp-ovn-techpreview |
Contributor
Author
|
/test e2e-aws-ovn-single-node-techpreview |
Contributor
Author
|
/test e2e-gcp-ovn-techpreview |
Contributor
Author
|
/test e2e-aws-ovn-single-node-techpreview |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Contributor
Author
|
/test e2e-gcp-ovn-techpreview |
Contributor
Author
|
/test e2e-aws-ovn-single-node-techpreview |
Contributor
Author
|
/test e2e-gcp-ovn-techpreview |
Contributor
Author
|
/test e2e-aws-ovn-single-node-techpreview |
alebedev87
reviewed
Apr 10, 2025
Contributor
alebedev87
left a comment
alebedev87
left a comment
There was a problem hiding this comment.
Note: I'm focusing only on things which can fail tests or make them flaky. Any rough edges are left for the post branch cut followups.
|
|
||
| g.By("Create the default Gateway") | ||
| gw := names.SimpleNameGenerator.GenerateName("gateway-") | ||
| gateways = append(gateways, gw) |
Contributor
There was a problem hiding this comment.
We should append only created gateways, at this point we are not sure it's created. However, this may be not a big problem because it'll failed AfterAll of a test which is already failed.
Contributor
Author
There was a problem hiding this comment.
yes, also most likely since its a generated name, it should almost always create the gateway, not neccessarily accept it but atleast create it
| dnsRecordName, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress", "dnsrecord", "-l", "gateway.networking.k8s.io/gateway-name="+gw, "-o=jsonpath={.items[0].metadata.name}").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| e2e.Logf("The gateway API dnsrecord name is: %v", dnsRecordName) | ||
| // check status of published dnsrecord of the gateway, all zones should be True (not contains False) | ||
| dnsRecordStatus, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress", "dnsrecord", dnsRecordName, `-o=jsonpath={.status.zones[*].conditions[0].status}`).Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| e2e.Logf("The dnsrecords status of all zones: %v", dnsRecordStatus) | ||
| o.Expect(dnsRecordStatus).NotTo(o.ContainSubstring("False")) |
Contributor
There was a problem hiding this comment.
This may flake, DNSRecord is created asynchonously when the gateway service is created. So, there is a race between the servive_dns_controller of ingress operator and this test.
Can be fixed later if it never caused a problem.
Contributor
Author
There was a problem hiding this comment.
do you think we should poll it then?
Contributor
There was a problem hiding this comment.
Yes, we better poll here to avoid flakiness.
rhamini3
commented
Apr 10, 2025
|
|
||
| g.By("Create the default Gateway") | ||
| gw := names.SimpleNameGenerator.GenerateName("gateway-") | ||
| gateways = append(gateways, gw) |
Contributor
Author
There was a problem hiding this comment.
yes, also most likely since its a generated name, it should almost always create the gateway, not neccessarily accept it but atleast create it
| dnsRecordName, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress", "dnsrecord", "-l", "gateway.networking.k8s.io/gateway-name="+gw, "-o=jsonpath={.items[0].metadata.name}").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| e2e.Logf("The gateway API dnsrecord name is: %v", dnsRecordName) | ||
| // check status of published dnsrecord of the gateway, all zones should be True (not contains False) | ||
| dnsRecordStatus, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("-n", "openshift-ingress", "dnsrecord", dnsRecordName, `-o=jsonpath={.status.zones[*].conditions[0].status}`).Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| e2e.Logf("The dnsrecords status of all zones: %v", dnsRecordStatus) | ||
| o.Expect(dnsRecordStatus).NotTo(o.ContainSubstring("False")) |
Contributor
Author
There was a problem hiding this comment.
do you think we should poll it then?
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: f5832ac
|
Contributor
Author
|
/test e2e-gcp-ovn-techpreview |
Contributor
Author
|
/test e2e-aws-ovn-single-node-techpreview |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 83a1e41
New tests seen in this PR at sha: 83a1e41
|
Contributor
Author
|
/test e2e-aws-ovn-single-node-techpreview |
Contributor
Author
|
/test e2e-metal-ipi-ovn-dualstack-bgp-techpreview |
Contributor
Author
|
/test e2e-aws-ovn-single-node-techpreview |
Add a workaround for OSSM 3.0.0, which includes a version of Istio that creates a configmap in every namespace. This configmap throws off the accounting used in verifying the behavior of ClusterResourceQuota. This commit adds an adjustment to this accounting when the Istio configmap is found. This commit is related to OCPBUGS-54884. https://issues.redhat.com/browse/OCPBUGS-54884 * test/extended/quota/clusterquota.go: Add workaround for Istio.
Contributor
Author
|
/test e2e-aws-ovn-single-node-techpreview |
Contributor
Author
|
/test e2e-aws-ovn-single-node-techpreview |
|
Job Failure Risk Analysis for sha: 74ec2ad
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 74ec2ad
New tests seen in this PR at sha: 74ec2ad
|
Contributor
|
/test e2e-aws-ovn-single-node-techpreview |
|
Job Failure Risk Analysis for sha: 74ec2ad
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 74ec2ad
New tests seen in this PR at sha: 74ec2ad
|
|
Job Failure Risk Analysis for sha: 74ec2ad
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 74ec2ad
|
Contributor
Author
|
/test e2e-aws-ovn-single-node-techpreview |
Contributor
|
/hold remove Looks good to me! |
Contributor
|
The latest changes look good. Thanks! /lgtm |
Contributor
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: knobunc, Miciah, rhamini3 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
|
/retest-required |
Contributor
Author
|
/test e2e-aws-ovn-single-node-techpreview |
Contributor
Author
|
/test e2e-gcp-ovn-techpreview |
|
Job Failure Risk Analysis for sha: 74ec2ad
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 74ec2ad
New tests seen in this PR at sha: 74ec2ad
|
Contributor
Author
|
/test e2e-aws-ovn-serial |
Contributor
|
@rhamini3: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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: 74ec2ad
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 74ec2ad
|
Contributor
Author
|
/test e2e-aws-ovn-serial |
openshift-merge-bot
bot
merged commit f5a8115
into
openshift:main
33 of 58 checks passed
Contributor
|
[ART PR BUILD NOTIFIER] Distgit: openshift-enterprise-tests |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
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.
PR for Network Ingress and DNS team for the 5 e2e tests to graduate featureGate to GA
JIRA: https://issues.redhat.com/browse/NE-1970
Test Scenarios:
Confirm that OLM and OSSM resources are created
Confirm that default gatewayClass is accepted
Confirm that a custom-gatewayclass can be created, deleted and not affect the state of the OSSM resources
A Custom gateway Object can be created
A HttpRoute can be created with no errors using a custom-gateway