Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Retrieve DNS domain name from cluster #258

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Retrieve DNS domain name from cluster #258

wants to merge 1 commit into from

Conversation

averdagu
Copy link
Contributor

@averdagu averdagu commented Apr 3, 2024 •
edited by openshift-ci bot
Loading

Jira: OSPRH-3627

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 3, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

averdagu
averdagu commented Apr 3, 2024
View reviewed changes
controllers/ovndbcluster_controller.go
Comment on lines +631 to +661
// Using this approach in case CP have multiple domains
// also it does not depend on the DNS CR name.
// ATM in case of multiple domains will return last one.
for _, dns := range DNSClusterInfoList.Items {
DNSDomain = dns.Status.ClusterDomain
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's possible or if it'll be a use case the fact that exists multiple DNS domains (besides cluster.local) and how OVN operator should handle it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the Reconcile loop of the openshift dns-operator, it only expects a DNS CR named Default, Could modify this chunk of code to get the object by name instead of getting a list and iterating over it.
https://github.com/openshift/cluster-dns-operator/blob/master/pkg/operator/controller/controller.go#L180-L184

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/2f28d7cb69234cb9b0e139a543d6f205

✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 24m 11s
❌ ovn-operator-tempest-multinode FAILURE in 1h 01m 57s

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/3d820c03902b4db08044c0e7f22d5129

✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 08m 16s
❌ ovn-operator-tempest-multinode FAILURE in 1h 03m 59s

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/8fafc03a2c234ed8a6c1b267bba71f43

✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 15m 49s
❌ ovn-operator-tempest-multinode FAILURE in 1h 04m 18s

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 28, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: averdagu
Once this PR has been reviewed and has the lgtm label, please assign viroel for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/f1fce6e33ed14a0a9918a1b8e5e2e958

✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 42m 26s
❌ ovn-operator-tempest-multinode FAILURE in 1h 06m 15s

@averdagu averdagu marked this pull request as ready for review October 29, 2024 09:40
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/ed8b4e7d61a04124abaf8ac7688c7b16

✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 18m 24s
❌ ovn-operator-tempest-multinode FAILURE in 1h 04m 02s

@averdagu
Retrieve DNS domain name from cluster
5784570 
OVNDBCluster uses openshift dns name for Status.InternalDbAddress,
which the dnsrecord is created using the following pattern:
[pod_name].[namespace].[type(svc)].[ClusterDomain]
The default for ClusterDomain is 'cluster.local'
but this could change in the future as mentioned in [0].
With this patch the ClusterDomain is gathered from the
environment and not hardcoded.

[0] https://github.com/openshift/cluster-dns-operator/blob/208d50c1a5e0aaeb991366daa749abdffa803224/pkg/operator/controller/controller.go#L498

Jira: OSPRH-3627
@averdagu
Copy link
Contributor Author

/retest as it doesn't seem caused by this patch

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 29, 2024

@averdagu: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test functional
  • /test images
  • /test ovn-operator-build-deploy-kuttl
  • /test precommit-check

The following commands are available to trigger optional jobs:

  • /test ovn-operator-build-deploy

Use /test all to run the following jobs that were automatically triggered:

  • pull-ci-openstack-k8s-operators-ovn-operator-main-functional
  • pull-ci-openstack-k8s-operators-ovn-operator-main-images
  • pull-ci-openstack-k8s-operators-ovn-operator-main-ovn-operator-build-deploy-kuttl
  • pull-ci-openstack-k8s-operators-ovn-operator-main-precommit-check

In response to this:

/retest as it doesn't seem caused by this patch

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.

@averdagu
Copy link
Contributor Author

/retest

@karelyatin
Copy link
Contributor

thx Arnau for the patch, now seeing it's usage i see it's hard coded at multiple places, so would be good to consider raising it and have a common solution and have something in lib-common/openstack-operator ?
https://github.com/search?q=org%3Aopenstack-k8s-operators%20cluster.local&type=code

@openshift-merge-robot
Copy link
Collaborator

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-sigs/prow repository.

@averdagu
Copy link
Contributor Author

averdagu commented Nov 6, 2024

Oh, thanks yatin for raising this, I was only focus on ovn-operator. Indeed I think it should be done globally. Putting this PR back to draft and looking into a more global solution

@averdagu averdagu marked this pull request as draft November 6, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lewisdenny lewisdenny Awaiting requested review from lewisdenny

@slawqo slawqo Awaiting requested review from slawqo

Assignees
No one assigned
Labels
do-not-merge/work-in-progress needs-rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@averdagu @karelyatin @openshift-merge-robot