Set up DNS server in containerized mounter path #51645

jingxu97 · 2017-08-30T18:37:00Z

During NFS/GlusterFS mount, it requires to have DNS server to be able to
resolve service name. This PR gets the DNS server ip from kubelet and
add it to the containerized mounter path. So if containerized mounter is
used, service name could be resolved during mount
Release note:

Allow DNS resolution of service name for COS using containerized mounter.  It fixed the issue with DNS resolution of NFS and Gluster services.

jingxu97 · 2017-08-30T18:40:16Z

Manually tested. Will add an e2e test soon. The following is the yaml file for the manual test

---
#Single NFS Server Pod
apiVersion: v1
kind: Pod
metadata:
  name: nfs-server
  labels:
    k8s-app: nfs-pod
    tag: nfs-pod-test
spec:
  containers:
  - name: nfs-server
    image: gcr.io/google_containers/volume-nfs:0.8
    ports:
      - name: nfs
        hostPort: 2049
        containerPort: 2049
        protocol: TCP
      - name: mountd
        hostPort: 20048
        containerPort: 20048
        protocol: TCP
      - name: rpcbind
        hostPort: 111
        containerPort: 111
        protocol: TCP
    securityContext:
      privileged: true
---
#Single Pod NFS Service
kind: Service
metadata:
  name: nfs-pod-service
  labels:
    k8s-app: nfs-pod
    tag: nfs-pod-test
spec:
  ports:
    - name: nfs
      port: 2049
      protocol: TCP
    - name: mountd
      port: 20048
      protocol: TCP
    - name: rpcbind
      port: 111
      protocol: TCP
  selector:
    k8s-app: nfs-pod
    tag: nfs-pod-test
---
#Single Pod NFS client
apiVersion: v1
kind: Pod
metadata:
  name: nfs-pod-client1
  labels:
    name: nfs-pod-client
    tag: nfs-pod-test
spec:
  containers:
  - name: app-pod
    image: redis
    volumeMounts:
    - name: webapp
      mountPath: /mnt
  volumes:
  - name: webapp
    nfs:
      # FIXME: The ip or domain of nfs-pod-service
      server: nfs-pod-service.default.svc.cluster.local
      path: "/"
      readOnly: false

jingxu97 · 2017-08-30T18:41:03Z

cc @MrHohn

msau42 · 2017-08-30T18:53:20Z

pkg/kubelet/kubelet.go

@@ -745,7 +746,26 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration,
 	// check node capabilities since the mount path is not the default
 	if len(kubeCfg.ExperimentalMounterPath) != 0 {
 		kubeCfg.ExperimentalCheckNodeCapabilitiesBeforeMount = false
+		resolvePath := filepath.Join(strings.TrimSuffix(kubeCfg.ExperimentalMounterPath, "/mounter"), "rootfs", "etc", "resolv.conf")


Are there any issues with adding the dns servers to the host resolv.conf? Wouldn't that also solve the hostname problem for non-containerized mounts?

Adding kube-dns's service VIP to (edited: host) resolv.conf might introduce too many side effects:

kube-dns's service VIP may not be routable on host in some environments (ref minkube: Dynamic Flexvolume plugin discovery proposal. community#833 (comment))

Processes on host namespace will send DNS requests to kube-dns first, then kube-dns will forward requests to upstream. This brings in additional hop and latency.

kube-dns pod copies resolv.conf from host in order to mirror the upstream DNS server setup, if we set kube-dns's service VIP there, this seems to be a circle?

If kube-dns dies, it breaks the DNS resolution on host even if upstream server is healthy.

And I think it will breaks dnsPolicy field on pod.spec as well :)

I guess when we move to CSI, the volume mounts will happen inside a container, and no longer from the host, so in the meantime, for non-containerized mounts using containerized filesystem services, we still only support using IPs and not hostnames.

MrHohn · 2017-08-30T20:16:32Z

pkg/kubelet/kubelet.go

+			for _, dns := range klet.clusterDNS {
+				dnsString = dnsString + fmt.Sprintf("nameserver %s\n", dns)
+			}
+			dnsString = dnsString + fmt.Sprintf("options rotate\n")


fmt.Sprintf("options rotate\n" -> "options rotate\n"?

what is the difference?

We shouldn't need fmt.Sprintf() because there is no arguments?

Humm..BTW do we really need "options rotate"? It won't hurt to do DNS query in the order as defined in resolv.conf right?

Without rotate option, the name still cannot be resolved.

address the comments, PTAL

Yep I think it occasionally worked with "options rotate" because sometime kube-dns got rotated as the first server.

MrHohn · 2017-08-30T20:17:40Z

pkg/kubelet/kubelet.go

+			defer fileHandle.Close()
+
+			dnsString := ""
+			for _, dns := range klet.clusterDNS {


Could we just don't write if len(klet.clusterDNS) == 0?

jingxu97 · 2017-08-30T20:30:44Z

cc @vishh could you please help approve this PR?

MrHohn · 2017-08-30T20:54:04Z

pkg/kubelet/kubelet.go

+			}
+			dnsString = dnsString + "options rotate\n"
+			glog.V(4).Infof("DNS nameserver string is %s", dnsString)
+			if _, err = fileHandle.WriteString(dnsString); err != nil {


Just realize we might have problem here. If we are appending kube-dns as an additional nameserver, dns request will be sent to upstream DNS server first. If we query in-cluster DNS name, upstream server will return NXDOMAIN, then the resolver won't check the other server (kube-dns in this case). Ref: https://unix.stackexchange.com/questions/150703/can-subsequent-nameservers-defined-in-etc-resolv-conf-be-used-if-the-previous-n

I think we should overwrite the nameserver entry instead of appending a new one?

Does kube-dns forward to upstream dns if it can't resolve the name?

Yep it will :)

So basically overwrite is enough. If kube-dns could not resolve it, upstream dns will work (just similar to use host dns nameserver)?

So basically overwrite is enough. If kube-dns could not resolve it, upstream dns will work (just similar to use host dns nameserver)?

Correct, this is exactly the way cluster DNS works in k8s :)

MrHohn · 2017-08-30T22:48:47Z

@krzyzacy Robot went crazy here :O

grodrigues3 · 2017-08-30T22:50:04Z

/release-note-none

MrHohn

This LGTM, did it work locally?

MrHohn · 2017-08-30T22:51:30Z

pkg/kubelet/kubelet.go

+				dnsString = dnsString + fmt.Sprintf("nameserver %s\n", dns)
+			}
+
+			ioutil.WriteFile(resolvePath, []byte(dnsString), 0600)


In this way the mounter wouldn't be able to utilize the internal search paths (e.g. "c.[PROJECT_ID].internal.", "google.internal."), but I guess that is okay?

Ref: https://cloud.google.com/compute/docs/vpc/internal-dns

Oh and we should check error here?

What does it mean to not utilize the internal search paths? The NFS or Gluster service could be setup directly on a GCE instance instead of as a Pod.

It means user will have to specify FQDN instead of relying on resolver automatically appending those search paths.

The NFS or Gluster service could be setup directly on a GCE instance instead of as a Pod.

Humm, then seems like this will change the behavior (e.g. INSTANCE_NAME alone is not resolvable anymore) and may break existing users?

If a user is migrating from debian to gci, then it will break.

In GCI, we had this timeline:
Initial: DNS resolution failed to work at all
1.6.?, 1.7.? patch: DNS resolution works for non-Pod based services.

An alternative is to rewrite the nameserver only, while keeping the original search paths in resolv.conf. There are some similar logic in GetClusterDNS():

kubernetes/pkg/kubelet/kubelet.go

Lines 1341 to 1344 in 7c70dec

hostDNS, hostSearch, err = kl.parseResolvConf(f)

if err != nil {

return nil, nil, false, err

}

ok, I also write the search string as suggested. So this time non-pod nfs service will also work, right?

krzyzacy · 2017-08-30T22:52:46Z

@grodrigues3 whoops, who suppose to own this munger?

msau42 · 2017-08-30T23:39:20Z

I think this needs a release note because it's going to be fixing a user-facing issue with DNS resolution of NFS and Gluster services.

But we need to make sure that this solution will work with both pod-based services and non-pod based services.

feiskyer · 2017-08-31T06:24:40Z

@krzyzacy Robot went crazy here :O

CI is down? all CIs didn't report status.

feiskyer · 2017-08-31T06:28:34Z

/test all

jingxu97 · 2017-09-02T00:21:30Z

@MrHohn I rebased it.

MrHohn · 2017-09-02T00:26:20Z

/lgtm

vishh · 2017-09-02T00:45:27Z

/approve

calebamiles · 2017-09-02T02:03:51Z

manually added approved label per intent by @vishh

cc: @kubernetes/kubernetes-release-managers

calebamiles · 2017-09-02T02:04:03Z

/retest

fejta-bot · 2017-09-02T08:39:51Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

jingxu97 · 2017-09-02T14:57:29Z

/retest

fejta-bot · 2017-09-03T00:24:51Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

During NFS/GlusterFS mount, it requires to have DNS server to be able to resolve service name. This PR gets the DNS server ip from kubelet and add it to the containerized mounter path. So if containerized mounter is used, service name could be resolved during mount

MrHohn · 2017-09-05T19:26:34Z

/lgtm

k8s-github-robot · 2017-09-05T19:26:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MrHohn, bowei, jingxu97, vishh

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

Needs approval from an approver in each of these OWNERS Files:

~~cluster/gce/OWNERS~~ [bowei,vishh]
~~pkg/kubelet/OWNERS~~ [vishh]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

jingxu97 · 2017-09-05T20:05:00Z

/retest

k8s-ci-robot · 2017-09-05T20:46:52Z

@jingxu97: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`3d4bc93`	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

k8s-github-robot · 2017-09-06T00:30:08Z

Automatic merge from submit-queue (batch tested with PRs 51186, 50350, 51751, 51645, 51837)

jdumars · 2017-09-06T19:46:11Z

@jingxu97 could you please add a SIG to this PR please? Thanks!

jingxu97 · 2017-09-06T20:47:41Z

cc @kubernetes/sig-storage-misc

jsafrane · 2017-09-07T07:53:29Z

I'd really appreciate if this was a generic PR that would be usable outside of GCE/GKE. It's not only Google who needs to mount nfs/gluster volumes hosted by Kybernetes!

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 30, 2017

k8s-github-robot assigned feiskyer and sjpotter Aug 30, 2017

k8s-github-robot added the release-note-label-needed label Aug 30, 2017

jingxu97 force-pushed the Aug/nameserver branch from ab1769f to e9643ed Compare August 30, 2017 18:38

jingxu97 requested a review from MrHohn August 30, 2017 18:40

msau42 reviewed Aug 30, 2017

View reviewed changes

MrHohn reviewed Aug 30, 2017

View reviewed changes

jingxu97 force-pushed the Aug/nameserver branch from e9643ed to 634532c Compare August 30, 2017 20:48

MrHohn reviewed Aug 30, 2017

View reviewed changes

jingxu97 force-pushed the Aug/nameserver branch from 634532c to 6a1769d Compare August 30, 2017 22:16

k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 30, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Aug 30, 2017

k8s-github-robot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 30, 2017

MrHohn approved these changes Aug 30, 2017

View reviewed changes

jingxu97 force-pushed the Aug/nameserver branch from 6a1769d to 8c6f989 Compare August 31, 2017 18:16

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 31, 2017

jingxu97 force-pushed the Aug/nameserver branch from fa2c4df to d5434e0 Compare September 2, 2017 00:20

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2017

calebamiles added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2017

calebamiles added this to the v1.8 milestone Sep 2, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2017

jingxu97 force-pushed the Aug/nameserver branch from d5434e0 to 3d4bc93 Compare September 5, 2017 18:40

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 5, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2017

k8s-github-robot merged commit e8d99f5 into kubernetes:master Sep 6, 2017

jingxu97 added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Sep 6, 2017

msau42 mentioned this pull request Oct 9, 2017

NFS example needs updates kubernetes/examples#44

Closed

	hostDNS, hostSearch, err = kl.parseResolvConf(f)
	if err != nil {
	return nil, nil, false, err
	}

Set up DNS server in containerized mounter path #51645

Set up DNS server in containerized mounter path #51645

Conversation

jingxu97 commented Aug 30, 2017 • edited Loading

jingxu97 commented Aug 30, 2017

jingxu97 commented Aug 30, 2017

Choose a reason for hiding this comment

MrHohn Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrHohn commented Aug 30, 2017

grodrigues3 commented Aug 30, 2017

MrHohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krzyzacy commented Aug 30, 2017

msau42 commented Aug 30, 2017

feiskyer commented Aug 31, 2017

feiskyer commented Aug 31, 2017

jingxu97 commented Sep 2, 2017

MrHohn commented Sep 2, 2017

vishh commented Sep 2, 2017

calebamiles commented Sep 2, 2017

calebamiles commented Sep 2, 2017

fejta-bot commented Sep 2, 2017

jingxu97 commented Sep 2, 2017

fejta-bot commented Sep 3, 2017

MrHohn commented Sep 5, 2017

k8s-github-robot commented Sep 5, 2017

jingxu97 commented Sep 5, 2017

k8s-ci-robot commented Sep 5, 2017

k8s-github-robot commented Sep 6, 2017

jdumars commented Sep 6, 2017

jingxu97 commented Sep 6, 2017

jsafrane commented Sep 7, 2017

jingxu97 commented Aug 30, 2017 •

edited

Loading

MrHohn Aug 30, 2017 •

edited

Loading