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

Add Windows support to the system verification check #53730

Merged

Conversation

bsteciuk
Copy link
Contributor

@bsteciuk bsteciuk commented Oct 11, 2017

What this PR does / why we need it: This PR (in conjunction with #53553 ) adds initial support for adding a Windows worker node to a Kubernetes cluster using
kubeadm. It was suggested on that PR to open a separate PR for the changes in test/e2e_node for review by sig-node devs.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #364 in conjuction with #53553

Special notes for your reviewer:

Release note:

Add Windows support to the system verification check

@k8s-ci-robot
Copy link
Contributor

@bsteciuk: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 11, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @bsteciuk. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 11, 2017
@luxas
Copy link
Member

luxas commented Oct 11, 2017

PTAL @kubernetes/sig-node-pr-reviews
/assign @Random-Liu @yujuhong

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Oct 11, 2017
@luxas luxas changed the title Added changes for initial support for Windows worker nodes to join cl… Add Windows support to the system verification check Oct 11, 2017
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 11, 2017
@luxas luxas added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 11, 2017
@luxas
Copy link
Member

luxas commented Oct 11, 2017

/release-note
/ok-to-test

@k8s-ci-robot
Copy link
Contributor

@luxas: the /release-note and /release-note-action-required commands have been deprecated.
Please edit the release-note block in the PR body text to include the release note. If the release note requires additional action include the string action required in the release note. For example:

```release-note
Some release note with action required.
```

In response to this:

/release-note
/ok-to-test

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.

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 11, 2017
@Random-Liu
Copy link
Member

@luxas @bsteciuk We'll build different binaries for windows and linux, right? I don't think in windows binary, we need the linux sys spec, and vise verse. Why don't we use build tag instead of using a table?

@luxas
Copy link
Member

luxas commented Oct 11, 2017

@Random-Liu Yeah, a build tag is even better. Let's do that instead, thanks ;-)

@bsteciuk
Copy link
Contributor Author

@luxas @Random-Liu - 👍 on it

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 11, 2017
}

// DefaultKernelValidatorHelper is the 'linux' implementation of KernelValidatorHelper
type DefaultKernelValidatorHelper struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: prefer empty struct on one line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

}

// WindowsKernelValidatorHelper is the 'windows' implementation of KernelValidatorHelper
type WindowsKernelValidatorHelper struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: prefer empty struct on one line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

bsteciuk added a commit to bsteciuk/kubernetes that referenced this pull request Oct 11, 2017
@bsteciuk
Copy link
Contributor Author

/retest

Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

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

Thanks @bsteciuk
After these changes, this PR LGTM


func (o *WindowsKernelValidatorHelper) GetKernelRelease() ([]byte, error) {
args := []string{"(Get-CimInstance Win32_OperatingSystem).Version"}
kernel, err := exec.Command("powershell", args...).Output()
Copy link
Member

Choose a reason for hiding this comment

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

nit, you can just write return exec.Command("powershell", args...).Output() here directly

}

// DefaultKernelValidatorHelper is the 'linux' implementation of KernelValidatorHelper
type DefaultKernelValidatorHelper struct{}
Copy link
Member

Choose a reason for hiding this comment

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

I would call this LinuxKernelValidatorHelper

// DefaultKernelValidatorHelper is the 'linux' implementation of KernelValidatorHelper
type DefaultKernelValidatorHelper struct{}

func (o *DefaultKernelValidatorHelper) GetKernelRelease() ([]byte, error) {
Copy link
Member

Choose a reason for hiding this comment

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

comment

type DefaultKernelValidatorHelper struct{}

func (o *DefaultKernelValidatorHelper) GetKernelRelease() ([]byte, error) {
kernel, err := exec.Command("uname", "-r").CombinedOutput()
Copy link
Member

Choose a reason for hiding this comment

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

return this directly

// WindowsKernelValidatorHelper is the 'windows' implementation of KernelValidatorHelper
type WindowsKernelValidatorHelper struct{}

func (o *WindowsKernelValidatorHelper) GetKernelRelease() ([]byte, error) {
Copy link
Member

Choose a reason for hiding this comment

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

comment


package system

// GetSysSpec returns os specific SysSpec
Copy link
Member

Choose a reason for hiding this comment

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

// DefaultSysSpec...


package system

// DefaultSysSpec is the default SysSpec.
Copy link
Member

Choose a reason for hiding this comment

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

... for Linux

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2017
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2017
@bsteciuk
Copy link
Contributor Author

/retest

@bsteciuk
Copy link
Contributor Author

/assign @Random-Liu

var DefaultSysSpec = SysSpec{
OS: "Microsoft Windows Server 2016",
KernelSpec: KernelSpec{
Versions: []string{`10\.[0-9]\.1439[3-9]*`, `10\.[0-9]\.144[0-9]*`, `10\.[0-9]\.15[0-9]*`, `10\.[0-9]\.2[0-9]*`}, //requires >= '10.0.14393'
Copy link
Member

Choose a reason for hiding this comment

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

  • 10\.[0-9]\.1439[3-9]* will match also 10.2.1439.
  • 10\.[0-9]\.144[0-9]* will match 10.0.144 which is lower than '10.0.14393'
  • 10\.[0-9]\.15[0-9]*, same here. will match 10.0.15, which is lower than '10.0.14393'
  • 10\.[0-9]\.2[0-9]*, will match 10.0.200, , which is lower than '10.0.14393'

Something that you probably want to have here is:

Versions:  []string{`10\.0\.1439[3-9]`, `10\.0\.14[4-9][0-9]{2}`, `10\.0\.1[5-9][0-9]{3}`, `10\.0\.[2-9][0-9]{4}`,`10\.[1-9]+\.[0-9]+}, //requires >= '10.0.14393'

That would match ranges:

  • 10.0.14393 -- 10.0.14399
  • 10.0.14400 -- 10.0.14999
  • 10.0.15000 -- 10.0.19999
  • 10.0.20000 -- 10.0.99999
  • 10.1.0+

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2017
@bsteciuk
Copy link
Contributor Author

@Random-Liu PTAL

@luxas
Copy link
Member

luxas commented Oct 24, 2017

@brendandburns Please take a look at this one and approve.
The system verification check parts of #53553 were broken out into this PR; which is now ready for review and merge.

import "os/exec"

// DockerEndpoint is the os specific endpoint for docker communication
const DockerEndpoint = "unix:///var/run/docker.sock"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why exporting the variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, export was not necessary

// KernelValidatorHelper is an interface intended to help with os specific kernel validation
type KernelValidatorHelper interface {
// GetKernelRelease gets the current kernel release version of the system
GetKernelRelease() ([]byte, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

GetKernelVersion or GetKernelReleaseVersion is more precise.

Also, I think the function should just return a string. Any trimming and byte array conversation should be done in the function.

@bsteciuk
Copy link
Contributor Author

/retest

@yujuhong
Copy link
Contributor

Didn't review the Windows spec closely, but the rest LGTM

@bsteciuk
Copy link
Contributor Author

/retest

@kad
Copy link
Member

kad commented Oct 25, 2017

please don't forget to squash commits.

Pulled SysSpecs out of types.go and created two os specific implementations with build tags

Similarly created conditionally compiled implementations of KernelValidationHelper to get Kernel version in os specific manner, as well as os specific docker endpoints (socket vs named pipes)
@bsteciuk
Copy link
Contributor Author

squashed

@luxas
Copy link
Member

luxas commented Oct 26, 2017

@brendanburns @yujuhong This ok to approve now?

@brendandburns
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, bsteciuk, luxas

Associated issue: 364

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

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

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

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Oct 27, 2017

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-kubeadm-gce fc40bf6b96eadce29d2a021fd129e628edcc1e74 link /test pull-kubernetes-e2e-kubeadm-gce

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.

@bsteciuk
Copy link
Contributor Author

/retest

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 931bc9e into kubernetes:master Oct 27, 2017
vdemeester pushed a commit to vdemeester/kubernetes that referenced this pull request Nov 1, 2017
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Kubeadm - Added initial support for Windows worker nodes to join cluster using kubeadm

**What this PR does / why we need it**:
This PR adds initial support for adding a Windows worker node to a Kubernetes cluster with kubeadm.  Also adds Windows build of kubeadm to node build targets. 

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes kubernetes/kubeadm#364 

**Special notes for your reviewer**:

Depends on kubernetes#53730 

**Release note**:

```release-note
kubeadm: Add support for adding a Windows node
```
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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. 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.

10 participants