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

tests: Splits hostname from DNS test #75591

Merged

Conversation

claudiubelu
Copy link
Contributor

@claudiubelu claudiubelu commented Mar 22, 2019

What type of PR is this?

/kind failing-test

/area conformance

/sig testing
/sig windows

What this PR does / why we need it:

At the moment, Windows cannot mount individual files into Containers, which means
that the Kubelet-managed hosts file cannot be mounted into the Container, causing
the [sig-network] DNS should provide DNS for pods for Hostname and Subdomain
test to fail.

The test mentioned test has /etc/hosts file entry checks. This commits separates the DNS
check and the /etc/hosts checks into two tests.

Which issue(s) this PR fixes:

Related issue: #70189
Related issue: #73425

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

At the moment, Windows cannot mount individual files into Containers, which means
that the Kubelet-managed hosts file cannot be mounted into the Container, causing
the "should provide DNS for pods for Hostname and Subdomain" test to fail.

The mentioned test has /etc/hosts file entry checks. This commits separates the
DNS check and the /etc/hosts checks into two tests.
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. area/conformance Issues or PRs related to kubernetes conformance tests sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/windows Categorizes an issue or PR as relevant to SIG Windows. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/test labels Mar 22, 2019
@claudiubelu
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-100-performance

@spiffxp spiffxp added this to To Triage in conformance-definition Mar 26, 2019
@johnbelamaric johnbelamaric moved this from To Triage to In Progress in conformance-definition Mar 26, 2019
@johnbelamaric
Copy link
Member

/assign

@johnbelamaric
Copy link
Member

When these are promoted to compliance, there will need to be an explanation for the LinuxOnly tag plus a link to the windows KEP section with details.

/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 26, 2019
@johnbelamaric johnbelamaric moved this from In Progress to Needs Approval in conformance-definition Mar 27, 2019
@timothysc timothysc removed the area/conformance Issues or PRs related to kubernetes conformance tests label Apr 2, 2019
@timothysc timothysc removed this from Needs Approval in conformance-definition Apr 2, 2019
@johnbelamaric
Copy link
Member

Is it the checking for the hostname looking at /etc/hosts that is the problem, or the actual functionality does not work on Windows? That is, if it works but the way we checked was wrong, can we fix that with the agnhost image?

@claudiubelu
Copy link
Contributor Author

Is it the checking for the hostname looking at /etc/hosts that is the problem, or the actual functionality does not work on Windows? That is, if it works but the way we checked was wrong, can we fix that with the agnhost image?

The actual functionality does not work on Windows. Basically, the /etc/hosts file should be mounted by Kubelet, but mounting individual files is not supported on Windows. For more details on this, see [1].

That is something that will eventually be fixed on Windows once ContainerD support is added, allowing us to be able to do this. When that is done, we will have to switch to the agnhost image.

[1] kubernetes/community#3521

@johnbelamaric
Copy link
Member

Ok, sounds good.

/priority important-soon

@k8s-ci-robot k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 10, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Apr 10, 2019
Copy link
Member

@MrHohn MrHohn left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bclau, MrHohn

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 2, 2019
@k8s-ci-robot k8s-ci-robot merged commit 75546a0 into kubernetes:master May 2, 2019
@mgdevstack
Copy link
Contributor

Subdomain resolution is verified for a headless service based on it's FQDN but not actual subdomain name. On linux platforms, A subdomain default-subdomain.my-namespace.svc.cluster.local MUST be resolvable for a headless service named default-subdomain by serving "A" records at that name.

If above subdomain resolution is not supported on Windows, does it make sense to add [LinuxOnly] to subdomain e2e with following code addition?

hostFQDN := fmt.Sprintf("%s.%s.%s.svc.%s", podHostname, serviceName, f.Namespace.Name, framework.TestContext.ClusterDNSDomain)
subdomain := fmt.Sprintf("%s.%s.svc.%s", serviceName, f.Namespace.Name, framework.TestContext.ClusterDNSDomain)
namesToResolve := []string{hostFQDN, subdomain}

@claudiubelu
Copy link
Contributor Author

Subdomain resolution is verified for a headless service based on it's FQDN but not actual subdomain name. On linux platforms, A subdomain default-subdomain.my-namespace.svc.cluster.local MUST be resolvable for a headless service named default-subdomain by serving "A" records at that name.

If above subdomain resolution is not supported on Windows, does it make sense to add [LinuxOnly] to subdomain e2e with following code addition?

hostFQDN := fmt.Sprintf("%s.%s.%s.svc.%s", podHostname, serviceName, f.Namespace.Name, framework.TestContext.ClusterDNSDomain)
subdomain := fmt.Sprintf("%s.%s.svc.%s", serviceName, f.Namespace.Name, framework.TestContext.ClusterDNSDomain)
namesToResolve := []string{hostFQDN, subdomain}

The subdomain default-subdomain.my-namespace.svc.cluster.local should also be resolvable on Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. 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-none Denotes a PR that doesn't merit a release note. sig/testing Categorizes an issue or PR as relevant to SIG Testing. 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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants