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

Windows: Fixes /etc/hosts file mounting support for containerd #83730

Merged
merged 1 commit into from Mar 5, 2021
Merged

Windows: Fixes /etc/hosts file mounting support for containerd #83730

merged 1 commit into from Mar 5, 2021

Conversation

claudiubelu
Copy link
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

If Containerd is used on Windows, then we can also mount individual files into containers (e.g.: /etc/hosts), which was not possible with Docker.

Checks if the container runtime is containerd, and if it is, then also mount /etc/hosts file (to C:\Windows\System32\drivers\etc\hosts).

Which issue(s) this PR fixes:

Fixes #70189

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

When using Containerd on Windows, the "C:\Windows\System32\drivers\etc\hosts" file will now be managed by kubelet.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 10, 2019
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 10, 2019
@claudiubelu claudiubelu mentioned this pull request Oct 21, 2019
Closed
@josiahbjorgaard
Copy link
Contributor

josiahbjorgaard commented Nov 3, 2019 •
edited

/milestone v1.17 This PR is intended to solve an issue that is in milestone 1.17. Will this PR be merged before code freeze on Nov. 14?

@k8s-ci-robot
Copy link
Contributor

@josiahbjorgaard: The provided milestone is not valid for this repository. Milestones in this repository: [next-candidate, v1.13, v1.14, v1.15, v1.16, v1.17, v1.18, v1.19, v2.0]

Use /milestone clear to clear the milestone.

In response to this:

/milestone 1.17 This PR is intended to solve an issue that is in milestone 1.17. Will this PR be merged before code freeze on Nov. 14?

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.

@josiahbjorgaard
Copy link
Contributor

/milestone v1.17

@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Nov 3, 2019
@PatrickLang PatrickLang added this to In Review (v1.17) in Windows CRI-ContainerD Bring-up Nov 5, 2019
PatrickLang
PatrickLang reviewed Nov 5, 2019
View reviewed changes
pkg/kubelet/kubelet_pods.go Outdated
@@ -465,7 +469,9 @@ func (kl *Kubelet) GenerateRunContainerOptions(pod *v1.Pod, container *v1.Contai
}
opts.Envs = append(opts.Envs, envs...)

mounts, cleanupAction, err := makeMounts(pod, kl.getPodDir(pod.UID), container, hostname, hostDomainName, podIP, volumes, kl.hostutil, kl.subpather, opts.Envs)
// we can only mount individual files (e.g.: /etc/hosts, termination-log files) on Windows only if we're using Containerd.
isContainerd := kl.containerRuntime.Type() == kubetypes.ContainerdContainerRuntime
Copy link
Contributor

Choose a reason for hiding this comment

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

where is this set? The kubelet config / command line still just includes a remote runtime.

I'm wondering if it makes more sense to implement a function that is capability focused, rather than returning the runtime

func SupportsSingleFileMapping() bool {
    switch (runtime.GOOS) {
       "windows": {
           return kl.containerRuntime.Type() == kubetypes.RemoteContainerRuntime
       }
       default: {
           return true
       }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The container runtime is queried directly by the kubeGenericRuntimeManager here: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kuberuntime/kuberuntime_manager.go#L242

Done

@josiahbjorgaard
Copy link
Contributor

Bug triage team for v1.17 here. This PR and its referenced issue need to be merged and closed by the code freeze deadline of this Thursday, end of day Pacific time in order to make the release. Otherwise, it should be bumped to v1.18. Will this PR be merged by then?

@claudiubelu claudiubelu force-pushed the windows/containerd-etc-hosts branch 6 times, most recently from f7f8f9f to 9505eaa Compare November 11, 2019 18:14
@PatrickLang
Copy link
Contributor

/lgtm
/assign @Random-Liu @yujuhong
Can one of you review the kubelet changes?

Thanks @BCLAU !

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2019
@claudiubelu
Windows: Fixes /etc/hosts file mounting support for containerd
de46029 
If Containerd is used on Windows, then we can also mount individual
files into containers (e.g.: /etc/hosts), which was not possible with Docker.

Checks if the container runtime is containerd, and if it is, then also
mount /etc/hosts file (to C:\Windows\System32\drivers\etc\hosts).
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2021
@claudiubelu
Copy link
Contributor Author

FWIW, this is how the hosts file looks in a Windows containerd container with this PR:

PS: it seems to also be alright with the Windows /etc/hosts file support:

kubectl exec -n dns-6267 pod/dns-test-f27a7950-ae6a-4efc-9a0e-99a26ba5a6b0 -- cat /Windows/System32/drivers/etc/hosts
Defaulting container name to webserver.
Use 'kubectl describe pod/dns-test-f27a7950-ae6a-4efc-9a0e-99a26ba5a6b0 -n dns-6267' to see all of the containers in this pod.
# Kubernetes-managed hosts file.
127.0.0.1       localhost
::1     localhost ip6-localhost ip6-loopback
fe00::0 ip6-localnet
fe00::0 ip6-mcastprefix
fe00::1 ip6-allnodes
fe00::2 ip6-allrouters
10.240.0.63     dns-querier-1.dns-test-service.dns-6267.svc.cluster.local       dns-querier-1

@claudiubelu
Copy link
Contributor Author

/recheck

@marosset marosset mentioned this pull request Mar 4, 2021
Merged
@marosset
Copy link
Contributor

marosset commented Mar 4, 2021

@claudiubelu Is this PR ready to merge?
From the comments it looks like we were waiting on some Windows OS fixes which were delivered in Sept 2020.

@marosset
Copy link
Contributor

marosset commented Mar 4, 2021

/sig windows

@k8s-ci-robot k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Mar 4, 2021
@marosset
Copy link
Contributor

marosset commented Mar 4, 2021

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 4, 2021
@claudiubelu
Copy link
Contributor Author

@claudiubelu Is this PR ready to merge?
From the comments it looks like we were waiting on some Windows OS fixes which were delivered in Sept 2020.

Yes, I've mentioned above that I've spawned some pods using this PR and another one, and the /etc/hosts files seems to be the one mounted by kubelet.

@kcmartin
Copy link

kcmartin commented Mar 5, 2021

Hello! I am from the Bug Triage team! Friendly reminder that code freeze is starting March 9th, 2021 (less than 1 week from now). We want to ensure that each PR has a chance to be merged before code freeze.

As the PR is tagged for 1.21, is it still planned for this release?

@marosset
Copy link
Contributor

marosset commented Mar 5, 2021

/test pull-kubernetes-unit

@marosset
Copy link
Contributor

marosset commented Mar 5, 2021

@kcmartin yes we'd like to get this merged for 1.21

@marosset
Copy link
Contributor

marosset commented Mar 5, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2021
@marosset
Copy link
Contributor

marosset commented Mar 5, 2021

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2021
@fejta-bot
Copy link

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 55f2552 into kubernetes:master Mar 5, 2021
Windows CRI-ContainerD Bring-up automation moved this from Backlog for Beta (v1.19) to Done Mar 5, 2021
SIG Node PR Triage automation moved this from Waiting on Author to Done Mar 5, 2021
@jsturtevant jsturtevant mentioned this pull request Jul 2, 2021
Merged
This was referenced Jan 19, 2022
Merged
Closed
@marosset marosset mentioned this pull request May 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TBBle TBBle TBBle left review comments

@liggitt liggitt liggitt left review comments

@PatrickLang PatrickLang PatrickLang left review comments

@dashpole dashpole Awaiting requested review from dashpole

@mtaufen mtaufen Awaiting requested review from mtaufen

Assignees

@Random-Liu Random-Liu

@yujuhong yujuhong

@PatrickLang PatrickLang

@marosset marosset

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
SIG Node PR Triage
Done
Windows CRI-ContainerD Bring-up
  
Done
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

Windows containers do not have entries in the hosts file