OCPBUGS-59176: skip specific tests if DNSManaged is false #29985
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@lihongan: This pull request references Jira Issue OCPBUGS-59176, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@lihongan: This pull request references Jira Issue OCPBUGS-59176, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: lihongan. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
test/extended/router/http2.go
Outdated
| return false, nil | ||
| } | ||
| if dns.Spec.PublicZone == nil && dns.Spec.PrivateZone == nil { | ||
| noDNSZones = true |
There was a problem hiding this comment.
The condition dns.Spec.PublicZone == nil && dns.Spec.PrivateZone == nil is not available to check the GCP user provisioned DNS cluster, instead the cluster set LoadBalancer.DNSManagementPolicy as below
if platformStatus.GCP != nil && platformStatus.GCP.CloudLoadBalancerConfig != nil &&
platformStatus.GCP.CloudLoadBalancerConfig.DNSType == configv1.ClusterHostedDNSType {
effectiveStrategy.LoadBalancer.DNSManagementPolicy = operatorv1.UnmanagedLoadBalancerDNS
}
We might need to update the func to check default ingresscontroller OperatorCondition Type: "DNSManaged" is ConditionFalse
lihongan
changed the title
|
@lihongan: This pull request references Jira Issue OCPBUGS-59176, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: lihongan. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
|
Job Failure Risk Analysis for sha: 49d13c3
|
|
/assign |
| if !isDNSManaged { | ||
| g.Skip("Skipping on this cluster since DNSManaged is false") | ||
| } |
There was a problem hiding this comment.
Is DNSRecord still created if there are no zones in DNSConfig? If so, I think that we should not skip but do a dedicated assertDNSRecord (instead of to the current assertDNSRecordStatus). WDYT?
There was a problem hiding this comment.
Looks DNSRecord is always created but the .status might be nil (dns.Spec.PublicZone == nil && dns.Spec.PrivateZone == nil) or shows Published type as False (PublicZone == {} or PrivateZone == {}).
Do you mean we should check dnses.config.openshift.io/v1 firstly then make decision to assertDNSRecord or assertDNSRecordStatus ?
There was a problem hiding this comment.
I removed the skip and added new logic in assertDNSRecordStatus to not check status if zone of dns.config is nil or empty.
There was a problem hiding this comment.
I removed the skip and added new logic in assertDNSRecordStatus to not check status if zone of dns.config is nil or empty.
What I meant is that assertDNSRecordStatus should check that DNSRecord's Published == False and Status is nil if there are no DNS zones in the DNS config. Just like your mentioned in the comment above the latest:
Looks DNSRecord is always created but the .status might be nil (dns.Spec.PublicZone == nil && dns.Spec.PrivateZone == nil) or shows Published type as False (PublicZone == {} or PrivateZone == {}).
| isDNSManaged, err := isDNSManaged(oc, time.Minute) | ||
| if err != nil { | ||
| e2e.Failf("Failed to get default ingresscontroller DNSManaged status: %v", err) | ||
| } | ||
| if !isDNSManaged { | ||
| g.Skip("Skipping on this cluster since DNSManaged is false") | ||
| } |
There was a problem hiding this comment.
I'm wondering why only GRPC interoperability test should be skipped. We have a lot more tests in extended/router suite, do they all work when there are no DNS zones for the cluster?
There was a problem hiding this comment.
Firstly, there are three conditions for DNSManaged status and if any of them are unsatisfied the status will be set as False, DNS zones in dns.config is just one of condition. see https://github.com/openshift/api/blob/fed56f2794b13fb3691cee40d0b429c2111d0e7f/operator/v1/types_ingress.go#L2060-L2065
Secondly, in the job "e2e-gcp-user-provisioned-dns" we just see the 4 failing tests, and the cluster set LoadBalancer.DNSManagementPolicy = UnmanagedLoadBalancerDNS but not rely on DNS zones. see https://github.com/openshift/cluster-ingress-operator/blob/cbc0b217b655f1f0ce0becc9145c2a6042beabea/pkg/operator/controller/ingress/controller.go#L478-L492
Thirdly, I believe most tests in extended/router suite are just tested with default ingresscontroller and the request go through default router pod, but GRPC/http2 tests create another shard ingresscontroller to test the feature and it rely on cloud DNS to resolve the shard ingresscontroller domain.
There was a problem hiding this comment.
@alebedev87 , @lihongan just checking to see what the resolution of this is. We need this fix so that the "e2e-gcp-user-provisioned-dns" periodic jobs are not in the Red due to these tests and force this feature to be reverted.
There was a problem hiding this comment.
Are we planning to make this a permanent skip? I don't agree with that.
There was a problem hiding this comment.
As discussed with Andrey, I plan to amend the logic of sending of HTTP requests to target the Load Balancer's IP address in case the DNS is not managed, with this trick then we don't need to skip the tests.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lihongan The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@lihongan: 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: e18b0fa
|
|
As discussed, even if the DNS is not managed on cluster, an IngressController or Gateway can still have internet facing load balancers (provisioned for their publishing services). We can amend the logic of sending of HTTP requests to target the load balancer's DNS name in case the DNS is not managed. This would also need the Host header to be set to the HTTPRoute or Route's hostname (goclient example of how to do this). |
@alebedev87 would like to see these changes done as part of this fix or as a follow up to skipping the tests temporarily. |
@sadasu: We discussed this with Hongan, we thought that we could give it a chance to be implemented. The possibility to do a follow-up PR was discussed too but we were not sure about the priority. Is it needed before 4.20 branch cut? |
|
This is more likely due to the new ClusterHostedDNS feature being tested on a nightly. /hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
|
||
| // skip status check if privateZone/publicZone in dns.config is nil or empty | ||
| emptyDNSZone := &configv1.DNSZone{} | ||
| dns, err := oc.AdminConfigClient().ConfigV1().DNSes().Get(context, "cluster", metav1.GetOptions{}) |
There was a problem hiding this comment.
Can this be checked outside the 10 minute loop, or do we need to check it every time?
Ack. This explains some things. From the customer configured DNS EP:
The http2 and grpc tests use dedicated ingresscontrollers, so the default wildcard created by the new static CoreDNS won't work for those tests.
Also, it seems like the GatewayAPI use case was overlooked. We have to follow up on the final decision, so far I see 2 PRs for this: |
|
Thanks @alebedev87 , I updated the bug description and posted new PR to try to fix the tests instead of skipping them. |
|
/close |
|
@lihongan: This pull request references Jira Issue OCPBUGS-59176. The bug has been updated to no longer refer to the pull request using the external bug tracker. 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. |
|
@lihongan: Closed this PR. 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 kubernetes-sigs/prow repository. |
5 participants
changes:
*.apps.<baseDomain>update:
The
gcp-user-provisioned-dnscluster setLoadBalancer.DNSManagementPolicyasUnmanagedLoadBalancerDNS, that is why DNSManaged is false. No dns zones is just one of conditions to set DNSManaged as false and cannot be used in this case.