etcd: remove etcd entries from mdns by mandre · Pull Request #1556 · openshift/machine-config-operator

mandre · 2020-03-11T12:55:02Z

Similar to openshift/installer#3265, this
removes the etcd entries from mdns config because etcd no longer uses
DNS.

abhinavdahiya · 2020-03-11T14:45:49Z

I think we should stop using the etcd discovery domain all together...

I couldn't find where to get the base domain from. Any suggestion?

I think the baremetal-runtime-cfg has a way to provide the cluster domain at runtime.. that might be an option?

I can change it here to {{ .Cluster.Domain }} but many of the templates are not rendered via baremetal-runtimecfg, so I'd rather leave {{ .EtcdDiscoveryDomain }} alone until we decide to effectively get rid of it globally.

I can change it here to {{ .Cluster.Domain }} but many of the templates are not rendered via baremetal-runtimecfg, so I'd rather leave {{ .EtcdDiscoveryDomain }} alone until we decide to effectively get rid of it globally.

api-int. is plain wrong imo, it works because that's the equal today. so i don't see why we would continue to use it..?

OK, I misunderstood your first comment - I'll make the change.

This is just checking that the DNS server is responding. It doesn't matter what record we look up. api-int happens to be a known name that should always be there.

This is just checking that the DNS server is responding. It doesn't matter what record we look up. api-int happens to be a known name that should always be there.

api-int.<cluster domain> is a known DNS name that should always be there, and not api-int.<etcd discovery domain>

Ah, Github ate the domain part of your comment. I thought you were objecting to the use of api-int in general. :-)

deads2k · 2020-03-12T15:05:15Z

@mandre I'd be very happy to see this and backported to 4.4. It will reduce the risk of people accidentally depending on "naughty" things

retroflexer · 2020-03-12T15:41:10Z

/retest

mandre · 2020-03-12T17:11:44Z

/test e2e-openstack
/test e2e-ovirt
/test e2e-vsphere

/cc rgolangh celebdor cybertron jcpowermac

Could you have a look? This PR touches the BM, openStack, ovirt and vsphere mDNS config.

cybertron · 2020-03-12T17:22:56Z

I've tested this on baremetal, and while the deployment works, it appears to be breaking our loadbalancing. This is because we repurposed the SRV record for getting the haproxy backends, and if we remove it we end up with no backends and thus no load balancing.

I'm looking into whether there is another way we can handle that, but because baremetal-runtimecfg runs so early in the deployment I'm not sure whether we can use the same methods that etcd is to get the node addresses. If any other platforms are also using haproxy for loadbalancing they should probably verify that this doesn't result in an invalid haproxy config too.

jcpowermac · 2020-03-12T17:33:40Z

For vSphere IPI (starting with 4.5) we were following baremetal. So if baremetal works most likely vsphere will as well. The current test is for UPI only.

rgolangh · 2020-03-12T22:12:07Z

@celebdor oVirt should fail on the same thing.

mandre · 2020-03-13T08:15:54Z

I've tested this on baremetal, and while the deployment works, it appears to be breaking our loadbalancing. This is because we repurposed the SRV record for getting the haproxy backends, and if we remove it we end up with no backends and thus no load balancing.

I'm looking into whether there is another way we can handle that, but because baremetal-runtimecfg runs so early in the deployment I'm not sure whether we can use the same methods that etcd is to get the node addresses. If any other platforms are also using haproxy for loadbalancing they should probably verify that this doesn't result in an invalid haproxy config too.

Indeed, I hadn't notice the broken haproxy config. Putting this on hold for now since it breaks on all platforms.
/hold

mandre · 2020-04-20T11:54:28Z

SRV records should no longer be a dependency for haproxy config thanks to openshift/baremetal-runtimecfg#52.

cybertron · 2020-04-21T17:49:56Z

I verified that this works on baremetal now so should be good to go from our end.

mandre · 2020-04-22T08:08:35Z

I've also checked that it worked for OpenStack. I was waiting for #1666 to merge to avoid merge conflicts but could push the rebased patch so everyone can try with their platform.

mandre · 2020-04-23T07:04:01Z

/retest

mandre · 2020-04-23T08:43:24Z

/test e2e-openstack
/test e2e-ovirt
/test e2e-vsphere

rgolangh · 2020-05-03T06:18:08Z

/lgtm ovirt

Thanks @mandre !

dougsland · 2020-06-01T18:01:46Z

/test e2e-aws
/test e2e-aws-scaleup-rhel7
/test e2e-vsphere
/lgtm

dougsland · 2020-06-01T18:08:18Z

@mandre @rgolangh looks like we also need to update: templates/worker/00-worker/ovirt/files/ovirt-non-virtual-ip.yaml:121 ?

I see: api_vip, dns_vip, ingress_vip = sys.argv[1:4]

jcpowermac · 2020-06-01T18:08:43Z

@patrickdillon

abhinavdahiya · 2020-06-01T19:35:12Z

should we use the https://github.com/openshift/api/blob/95abe2d2f4223d5931e418bf8e4d3773d16b42c0/config/v1/types_infrastructure.go#L74-L78

So that we are not assuming the api-int subdomain exists on EtcdDiscoveryDomain. Because that is technically not true or part of the API.

This check no longer exists, cf #1666

abhinavdahiya · 2020-06-01T19:35:32Z

same as https://github.com/openshift/machine-config-operator/pull/1556/files#r433445452

Same here, this check was removed in #1775

Similar to openshift/installer#3265, this removes the etcd entries from mdns config because etcd no longer uses DNS.

mandre · 2020-06-03T07:12:42Z

/test e2e-openstack
/test e2e-ovirt
/test e2e-vsphere

mandre · 2020-06-03T07:14:05Z

/hold cancel

mandre · 2020-06-03T08:59:11Z

/test e2e-openstack

dougsland · 2020-06-03T10:52:46Z

/test e2e-aws-scaleup-rhel7

cybertron

Looks good for baremetal.

dougsland · 2020-06-05T10:46:52Z

/test e2e-aws-scaleup-rhel7

sdodson · 2020-06-09T14:30:07Z

/lgtm
@ericavonb PTAL, needs approved

ericavonb · 2020-06-09T17:33:18Z

/lgtm

openshift-ci-robot · 2020-06-09T17:33:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cybertron, dougsland, ericavonb, mandre, rgolangh, sdodson

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ericavonb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cybertron · 2020-07-09T14:05:41Z

/cherry-pick release-4.5

openshift-cherrypick-robot · 2020-07-09T14:05:50Z

@cybertron: new pull request created: #1917

Details

In response to this:

/cherry-pick release-4.5

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/test-infra repository.

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 11, 2020

mandre mentioned this pull request Mar 11, 2020

Bug 1813003: etcd: remove etcd DNS entries because etcd no longer uses DNS openshift/installer#3265

Merged

openshift-ci-robot requested review from russellb and sinnykumari March 11, 2020 12:57

abhinavdahiya reviewed Mar 11, 2020

View reviewed changes

openshift-ci-robot requested review from celebdor, cybertron, jcpowermac and rgolangh March 12, 2020 17:11

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 13, 2020

mandre force-pushed the etcd-records branch from 5782d63 to 643aaf7 Compare April 22, 2020 08:12

openshift-ci-robot assigned dougsland Jun 1, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 1, 2020

abhinavdahiya reviewed Jun 1, 2020

View reviewed changes

etcd: remove etcd entries from mdns

777416c

Similar to openshift/installer#3265, this removes the etcd entries from mdns config because etcd no longer uses DNS.

mandre force-pushed the etcd-records branch from 643aaf7 to 777416c Compare June 3, 2020 07:11

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2020

cybertron approved these changes Jun 3, 2020

View reviewed changes

openshift-ci-robot assigned sdodson Jun 9, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 9, 2020

openshift-ci-robot assigned ericavonb Jun 9, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2020

openshift-merge-robot merged commit 97d5ed6 into openshift:master Jun 9, 2020

openshift-cherrypick-robot mentioned this pull request Jul 9, 2020

Bug 1855373: etcd: remove etcd entries from mdns #1917

Merged

Conversation

mandre commented Mar 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Mar 12, 2020

Uh oh!

retroflexer commented Mar 12, 2020

Uh oh!

mandre commented Mar 12, 2020

Uh oh!

cybertron commented Mar 12, 2020

Uh oh!

jcpowermac commented Mar 12, 2020

Uh oh!

rgolangh commented Mar 12, 2020

Uh oh!

mandre commented Mar 13, 2020

Uh oh!

mandre commented Apr 20, 2020

Uh oh!

cybertron commented Apr 21, 2020

Uh oh!

mandre commented Apr 22, 2020

Uh oh!

mandre commented Apr 23, 2020

Uh oh!

mandre commented Apr 23, 2020

Uh oh!

rgolangh commented May 3, 2020

Uh oh!

dougsland commented Jun 1, 2020

Uh oh!

dougsland commented Jun 1, 2020

Uh oh!

jcpowermac commented Jun 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mandre commented Jun 3, 2020

Uh oh!

mandre commented Jun 3, 2020

Uh oh!

mandre commented Jun 3, 2020

Uh oh!

dougsland commented Jun 3, 2020

Uh oh!

cybertron left a comment

Choose a reason for hiding this comment

Uh oh!

dougsland commented Jun 5, 2020

Uh oh!

sdodson commented Jun 9, 2020

Uh oh!

ericavonb commented Jun 9, 2020

Uh oh!

abhinavdahiya Mar 12, 2020 •

edited

Loading