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

test images: uses nanoserver #89425

Merged

Conversation

claudiubelu
Copy link
Contributor

What type of PR is this?

/kind feature

/sig testing
/sig windows

What this PR does / why we need it:

Using Windows nanoserver container images as a base instead of the current Windows servercore image will reduce the image size by about ~10x.

However, the nanoserver image lacks several things we need:

  • netapi32.dll
  • powershell
  • certain powershell commands
  • chocolatey cannot be used

When building the nanoserver images, we are going to use a Windows servercore helper, in which we are going to install the necessary dependencies, and then copy them over to our nanoserver image, including necessary DLLs.

Other notable changes include:

  • switch from wget to curl (wget was a powershell alias).
  • implement in code getting the DNS suffix list and DNS server list.
  • reimplement getting file permissions for mounttest.

Which issue(s) this PR fixes:

Partially Fixes #77268

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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


@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. sig/windows Categorizes an issue or PR as relevant to SIG Windows. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 24, 2020
@k8s-ci-robot k8s-ci-robot added area/dependency Issues or PRs related to dependency changes area/release-eng Issues or PRs related to the Release Engineering subproject area/test sig/release Categorizes an issue or PR as relevant to SIG Release. labels Mar 24, 2020
@adelina-t
Copy link
Contributor

/cc @adelina-t

@claudiubelu claudiubelu force-pushed the test-images/switch-to-nanoserver branch from bf65ff3 to 1e7d06e Compare March 25, 2020 14:12
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2020
@claudiubelu claudiubelu force-pushed the test-images/switch-to-nanoserver branch 2 times, most recently from 6481c6c to 5fcc809 Compare April 1, 2020 16:24
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2020
@claudiubelu claudiubelu force-pushed the test-images/switch-to-nanoserver branch from 5fcc809 to 57317aa Compare April 6, 2020 11:40
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2020
@claudiubelu claudiubelu force-pushed the test-images/switch-to-nanoserver branch 2 times, most recently from 60a74f3 to 68fe365 Compare April 9, 2020 14:12
@marosset marosset added this to In Progress (v1.19) in SIG-Windows Apr 13, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2020
Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

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

I can't speak to the windows specifics; this looks reasonable however I have one nit

@@ -17,40 +17,131 @@ limitations under the License.
package dns

import (
"bytes"
"os/exec"
"fmt"
Copy link
Member

Choose a reason for hiding this comment

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

line unrelated

Is there any particular reason we don't have // +build windows as line 1 of this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, it felt redundant, we were already relying on the file name suffix for that (conditional compilation can also be done through filename suffix). Added that line now.

@claudiubelu claudiubelu force-pushed the test-images/switch-to-nanoserver branch from b3f4d89 to ed00653 Compare September 17, 2020 10:33
@spiffxp
Copy link
Member

spiffxp commented Sep 17, 2020

needs rebase

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2020
@claudiubelu claudiubelu force-pushed the test-images/switch-to-nanoserver branch from ed00653 to 732705b Compare September 18, 2020 11:15
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2020
Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2020
@spiffxp
Copy link
Member

spiffxp commented Sep 18, 2020

/assign @liggitt @thockin
I don't have /approve rights for the /go.sum change

go.sum Outdated Show resolved Hide resolved
@marosset marosset moved this from In Progress (v1.20) to In Review (v1.20) in SIG-Windows Sep 18, 2020
Using Windows nanoserver container images as a base instead of the current
Windows servercore image will reduce the image size by about ~10x.

However, the nanoserver image lacks several things we need:
- netapi32.dll
- powershell
- certain powershell commands
- chocolatey cannot be used

When building the nanoserver images, we are going to use a Windows servercore helper,
in which we are going to install the necessary dependencies, and then copy them over
to our nanoserver image, including necessary DLLs.

Other notable changes include:
- switch from wget to curl (wget was a powershell alias).
- implement in code getting the DNS suffix list and DNS server list.
- reimplement getting file permissions for mounttest.
@claudiubelu claudiubelu force-pushed the test-images/switch-to-nanoserver branch from 732705b to 46c820e Compare September 21, 2020 11:54
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: claudiubelu, spiffxp

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 Sep 21, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 21, 2020

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

Test name Commit Details Rerun command
pull-kubernetes-node-e2e-containerd 60a74f327935a410d565eecdcf4bb77a72b52684 link /test pull-kubernetes-node-e2e-containerd
pull-kubernetes-cross e36868c57edfed77b34216feece247650ee7da3f link /test pull-kubernetes-cross
pull-kubernetes-files-remake e36868c57edfed77b34216feece247650ee7da3f link /test pull-kubernetes-files-remake
pull-kubernetes-e2e-gce e36868c57edfed77b34216feece247650ee7da3f link /test pull-kubernetes-e2e-gce
pull-kubernetes-e2e-gce-device-plugin-gpu e36868c57edfed77b34216feece247650ee7da3f link /test pull-kubernetes-e2e-gce-device-plugin-gpu
pull-kubernetes-kubemark-e2e-gce-big e36868c57edfed77b34216feece247650ee7da3f link /test pull-kubernetes-kubemark-e2e-gce-big

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.

@claudiubelu
Copy link
Contributor Author

/test pull-kubernetes-e2e-azure-disk-windows

@claudiubelu
Copy link
Contributor Author

Can the /lgtm be reapplied?

@spiffxp
Copy link
Member

spiffxp commented Sep 23, 2020

/lgtm

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

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit 4ad26f2 into kubernetes:master Sep 23, 2020
SIG-Windows automation moved this from In Review (v1.20) to Done (v1.20) Sep 23, 2020
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/dependency Issues or PRs related to dependency changes area/release-eng Issues or PRs related to the Release Engineering subproject area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/release Categorizes an issue or PR as relevant to SIG Release. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kubernetes test images should include Windows images in their manifest lists