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.

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

Use dns cluster info from lib common get function #376

Conversation

averdagu
Copy link
Contributor

@averdagu averdagu commented Nov 12, 2024

Openshift coreDNS creates the domain name using an string located in dnses.operator.openshift.io. This string can change in the future, calling lib-common/GetDNSClusterDomain the responsability of gathering this information correctly only falls under lib-common intead of all operators.

Resolves: OSPRH-3627
Depends-on: openstack-k8s-operators/lib-common#580

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/9a5458b6a1b54dd5a4786bf7dc0412cd

openstack-k8s-operators-content-provider FAILURE in 6m 23s
⚠️ ovn-operator-tempest-multinode SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

@mtomaska mtomaska self-requested a review November 12, 2024 19:01
main.go Outdated
@@ -40,6 +40,7 @@ import (
metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"

networkv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
operatorv1 "github.com/openshift/api/operator/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm.. I don't think we need it right now.
The idea behind watching this resource (On line 101) is to have the ovn-operator ready in case clusterDomain is parameterizable. If this resource is not watched, it will only be gathered when the ovn-operator is reconciled (for external reasons).

Copy link
Contributor

Choose a reason for hiding this comment

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

but line 101 would not watch it. it just adds the permissions to be able to watch it. In my opinion we should not watch objects we don't use right now. we should add it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh true, I forgot to add the watch part when I was copying the code. Instead of watching I will add a //TODO that if clusterLocal is parameterizable we need to watch the resource

@averdagu averdagu force-pushed the fix/get-dns-from-libcommon branch 2 times, most recently from b889967 to d3f35f5 Compare November 13, 2024 16:05
Copy link

This change depends on a change that failed to merge.

Change openstack-k8s-operators/lib-common#580 is needed.

@booxter
Copy link
Contributor

booxter commented Nov 14, 2024

Code is fine but I think you need to pull the library in go.mod.

Copy link
Contributor

openshift-ci bot commented Nov 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: averdagu

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

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/9cca9f92a0de4f16a66c5d7baf3fe844

openstack-k8s-operators-content-provider FAILURE in 6m 49s
⚠️ ovn-operator-tempest-multinode SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

@averdagu averdagu force-pushed the fix/get-dns-from-libcommon branch from 7ab8f40 to 4a7a520 Compare November 18, 2024 11:38
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/d33f3a22d1a24a48ab232f7296bc21ed

openstack-k8s-operators-content-provider FAILURE in 10m 44s
⚠️ ovn-operator-tempest-multinode SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

@karelyatin
Copy link
Contributor

/retest

@karelyatin
Copy link
Contributor

Also need to fix in openstack-operator to clear this atleast DNSSuffix usage

$ grep -r -e ClusterInternalDomain -e cluster.local -e DNSSuffix
grep: bin/k8s/1.29.3-linux-amd64/kube-apiserver: binary file matches
grep: bin/k8s/1.29.3-linux-amd64/kubectl: binary file matches
grep: .git/objects/pack/pack-c8e17e8731cb1dde52601f13e680585a00ec8588.pack: binary file matches
pkg/openstack/galera.go: fmt.Sprintf("%s.%s", hostname, ClusterInternalDomain),
pkg/openstack/galera.go: fmt.Sprintf("%s.%s", hostnameHeadless, ClusterInternalDomain),
pkg/openstack/galera.go: fmt.Sprintf(".%s.%s", hostnameHeadless, ClusterInternalDomain),
pkg/openstack/galera.go: Organizations: []string{fmt.Sprintf("%s.%s", instance.Namespace, ClusterInternalDomain)},
pkg/openstack/nova.go: fmt.Sprintf("%s.%s", hostname, ClusterInternalDomain),
pkg/openstack/nova.go: Organizations: []string{fmt.Sprintf("%s.%s", instance.Namespace, ClusterInternalDomain)},
pkg/openstack/neutron.go: fmt.Sprintf("%s.%s.svc.%s", serviceName, instance.Namespace, "cluster.local"),
pkg/openstack/memcached.go: fmt.Sprintf("%s.%s.svc.%s", name, instance.Namespace, ClusterInternalDomain),
pkg/openstack/memcached.go: fmt.Sprintf("
.%s.%s.svc.%s", name, instance.Namespace, ClusterInternalDomain),
pkg/openstack/common.go: // ClusterInternalDomain - cluster internal dns domain
pkg/openstack/common.go: ClusterInternalDomain = "cluster.local"
pkg/openstack/common.go: fmt.Sprintf("%s.%s.svc.%s", ed.Name, instance.Namespace, ClusterInternalDomain),
pkg/openstack/common.go: fmt.Sprintf("%s.%s.svc.%s", ed.Name, instance.Namespace, ClusterInternalDomain),
pkg/openstack/rabbitmq.go: fmt.Sprintf("%s.%s", hostname, ClusterInternalDomain),
pkg/openstack/rabbitmq.go: fmt.Sprintf("%s.%s", hostnameHeadless, ClusterInternalDomain),
pkg/openstack/rabbitmq.go: Organizations: []string{fmt.Sprintf("%s.%s", rabbitmq.Namespace, ClusterInternalDomain)},
pkg/openstack/redis.go: fmt.Sprintf("redis-%s.%s.svc.%s", name, instance.Namespace, ClusterInternalDomain),
pkg/openstack/redis.go: fmt.Sprintf(".redis-%s.%s.svc.%s", name, instance.Namespace, ClusterInternalDomain),
pkg/openstack/redis.go: Organizations: []string{fmt.Sprintf("%s.%s", instance.Namespace, ClusterInternalDomain)},
pkg/openstack/ovn.go: fmt.Sprintf("
.%s.svc.%s", instance.Namespace, ovnv1.DNSSuffix),
pkg/openstack/ovn.go: fmt.Sprintf("%s.%s.svc.%s", serviceName, instance.Namespace, ovnv1.DNSSuffix),
pkg/openstack/ovn.go: fmt.Sprintf("%s.%s.svc.%s", serviceName, instance.Namespace, ovnv1.DNSSuffix),
pkg/openstack/octavia.go: fmt.Sprintf("%s.%s.svc.%s", serviceName, instance.Namespace, ClusterInternalDomain),
config/certmanager/certificate.yaml: - $(SERVICE_NAME).$(SERVICE_NAMESPACE).svc.cluster.local

@averdagu
Copy link
Contributor Author

averdagu commented Dec 2, 2024

Problem seen in this commit should be solved once this merges openstack-k8s-operators/openstack-operator#1215

Openshift coreDNS creates the domain name using an string
located in dnses.operator.openshift.io. This string can
change in the future, calling lib-common/GetDNSClusterDomain
the responsability of gathering this information correctly
only falls under lib-common intead of all operators.

Resolves: OSPRH-3627
Depends-on: openstack-k8s-operators/lib-common#580
@averdagu averdagu force-pushed the fix/get-dns-from-libcommon branch from 4a7a520 to d406486 Compare December 16, 2024 10:57
@booxter
Copy link
Contributor

booxter commented Dec 18, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Dec 18, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 02d4a86 into openstack-k8s-operators:main Dec 19, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants