Skip to content

Conversation

@cybertron
Copy link
Member

Instead of relying on coredns to provide the api-int record, which
requires our coredns to be deployed as a static pod so it will be
available before the node tries to register with the api, we can
put the api-int record in /etc/hosts. This way the node can register
without coredns running and then coredns can be started at a later
point in the deployment. This makes our coredns deployment much more
flexible.

Since I'm told we should not try to template all of /etc/hosts because
other service may be trying to add entries to it as well, this
change adds a systemd service and timer to ensure that the correct
entry is in /etc/hosts. It will run once per minute and append the
api-int line if it doesn't already find one there.

- What I did

- How to verify it

- Description for the changelog

Instead of relying on coredns to provide the api-int record, which
requires our coredns to be deployed as a static pod so it will be
available before the node tries to register with the api, we can
put the api-int record in /etc/hosts. This way the node can register
without coredns running and then coredns can be started at a later
point in the deployment. This makes our coredns deployment much more
flexible.

Since I'm told we should not try to template all of /etc/hosts because
other service may be trying to add entries to it as well, this
change adds a systemd service and timer to ensure that the correct
entry is in /etc/hosts. It will run once per minute and append the
api-int line if it doesn't already find one there.
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cybertron
To complete the pull request process, please assign runcom after the PR has been reviewed.
You can assign the PR to them by writing /assign @runcom in a comment when ready.

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

Details 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

match api.{{ .DNS.Spec.BaseDomain }}
fallthrough
}
template IN {{`{{ .Cluster.APIVIPRecordType }}`}} {{ .DNS.Spec.BaseDomain }} {
Copy link
Contributor

@yboaron yboaron Dec 1, 2020

Choose a reason for hiding this comment

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

The overall idea LGTM, I have two Qs.

  1. I understand that you deleted api-int from CoreDNS to verify that we can break the Kubelet->coredns->Kubelet circular dependency and have a stable deployment without relying on CoreDNS for resolving api-int.
    I wonder if it will be safer (for 4.7) to keep also api-int in CoreDns ? in case something went wrong with /etc/hosts .

  2. IIUC, the service setting api-int in /etc/hosts supposed to run on all on-prem platforms, so this PR changes the api-int resolution also for other on-prem platforms. if that is the case, maybe you should update the PR title.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. I considered that, but then we have multiple definitions of the api-int record which can get out of sync. If we're going to do this, I think I'd rather just make a clean switch over. I'm open to being convinced otherwise though.

  2. Good point. I didn't want to introduce differences between the platforms when this should work on all of them.

Co-authored-by: Miciah Dashiel Butler Masters <mmasters@redhat.com>
@cybertron cybertron changed the title [baremetal] Move api-int to /etc/hosts [baremetal & friends] Move api-int to /etc/hosts Dec 1, 2020
@kikisdeliveryservice
Copy link
Contributor

@mandre @jcpowermac : friends.. PTAL

@jcpowermac
Copy link
Contributor

/assign @patrickdillon

cc: @rvanderp3

@mandre
Copy link
Member

mandre commented Dec 3, 2020

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

@cgwalters
Copy link
Member

Since I'm told we should not try to template all of /etc/hosts because
other service may be trying to add entries to it as well, this
change adds a systemd service and timer to ensure that the correct
entry is in /etc/hosts. It will run once per minute and append the
api-int line if it doesn't already find one there.

OK a few things here. First just a note: we can use inotify to watch the file and avoid wakeups. (In Kubernetes this doesn't really matter because there's so much polling going on anyways...but it's exactly the kind of problem that becomes important in e.g. small scale IoT/edge type systems where you just want to run some containers on a single system in a CPU/power efficient way)

Second...why does this have to be in shell script at all? Can't it be part of the MCD Go code? If we need this api-int record before kubelet comes up, this process could be part of the existing machine-config-daemon-firstboot.service - basically this is part of a general pattern where we can include things in the MCD and run them either before or after kubelet.

Third, as implemented today if the internal IP changes won't this result in multiple lines ending up in /etc/hosts? I think the logic probably needs to be something more like writing a section of the file that looks like

127.0.0.1 localhost blah blah the default /etc/hosts content
...
(stuff here might be injected manually by custom Ignition)
...

# Managed by machine-config-operator; do not edit
10.x.x.x api-int
# End machine-config-operator managed section

and the MCO code carefully only edits the latter section.

@openshift-merge-robot
Copy link
Contributor

@cybertron: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws 5ef9693 link /test e2e-aws
ci/prow/e2e-aws-serial 5ef9693 link /test e2e-aws-serial
ci/prow/okd-e2e-aws 5ef9693 link /test okd-e2e-aws
ci/prow/e2e-ovn-step-registry 5ef9693 link /test e2e-ovn-step-registry
ci/prow/e2e-aws-workers-rhel7 5ef9693 link /test e2e-aws-workers-rhel7
ci/prow/e2e-ovirt 5ef9693 link /test e2e-ovirt
ci/prow/e2e-vsphere 5ef9693 link /test e2e-vsphere
ci/prow/e2e-openstack 5ef9693 link /test e2e-openstack

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 3, 2021
@cybertron
Copy link
Member Author

After further discussion, we don't think /etc/hosts is a good place for this after all. If we pursue this, #2374 is likely a better option.

@cybertron cybertron closed this Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Projects

None yet

Development

Successfully merging this pull request may close these issues.