Merge pull request #517 from jeremyje/fixwin

Windows Support: Fix Build Regressions, Tests Pass

Author: k8s-ci-robot

.travis.yml

@@ -31,3 +31,4 @@ script:
  - BUILD_TAGS="disable_stackdriver_exporter" make test
  - make clean && ENABLE_JOURNALD=0 make
  - ENABLE_JOURNALD=0 make test
+ - ENABLE_JOURNALD=0 make build-binaries

Makefile

@@ -38,7 +38,12 @@ UPLOAD_PATH:=$(shell echo $(UPLOAD_PATH) | sed '$$s/\/*$$//')
 PKG:=k8s.io/node-problem-detector
 
 # PKG_SOURCES are all the go source code.
+ifeq ($(OS),Windows_NT)
+PKG_SOURCES:=
+# TODO: File change detection does not work in Windows.
+else
 PKG_SOURCES:=$(shell find pkg cmd -name '*.go')
+endif
 
 # PARALLEL specifies the number of parallel test nodes to run for e2e tests.
 PARALLEL?=3
@@ -74,6 +79,12 @@ BUILD_TAGS?=
 LINUX_BUILD_TAGS = $(BUILD_TAGS)
 WINDOWS_BUILD_TAGS = $(BUILD_TAGS)
 
+ifeq ($(OS),Windows_NT)
+HOST_PLATFORM_BUILD_TAGS = $(WINDOWS_BUILD_TAGS)
+else
+HOST_PLATFORM_BUILD_TAGS = $(LINUX_BUILD_TAGS)
+endif
+
 ifeq ($(ENABLE_JOURNALD), 1)
 	# Enable journald build tag.
 	LINUX_BUILD_TAGS := $(BUILD_TAGS) journald
@@ -91,9 +102,9 @@ else
 endif
 
 vet:
-	GO111MODULE=on go list -mod vendor -tags "$(LINUX_BUILD_TAGS)" ./... | \
+	GO111MODULE=on go list -mod vendor -tags "$(HOST_PLATFORM_BUILD_TAGS)" ./... | \
 		grep -v "./vendor/*" | \
-		GO111MODULE=on xargs go vet -mod vendor -tags "$(LINUX_BUILD_TAGS)"
+		GO111MODULE=on xargs go vet -mod vendor -tags "$(HOST_PLATFORM_BUILD_TAGS)"
 
 fmt:
 	find . -type f -name "*.go" | grep -v "./vendor/*" | xargs gofmt -s -w -l
@@ -109,7 +120,10 @@ ifeq ($(ENABLE_JOURNALD), 1)
 	LINUX_AMD64_BINARIES += bin/linux_amd64/log-counter
 endif
 
-windows-binaries: $(WINDOWS_AMD64_BINARIES) $(WINDOWS_AMD64_TEST_BINARIES)
+WINDOWS_BINARIES = $(WINDOWS_AMD64_BINARIES) $(WINDOWS_AMD64_TEST_BINARIES)
+LINUX_BINARIES = $(LINUX_AMD64_BINARIES) $(LINUX_AMD64_TEST_BINARIES)
+
+windows-binaries: $(WINDOWS_BINARIES)
 
 bin/windows_amd64/%.exe: $(PKG_SOURCES)
 ifeq ($(ENABLE_JOURNALD), 1)
@@ -191,10 +205,10 @@ endif
 		cmd/healthchecker/health_checker.go
 
 test: vet fmt
-	GO111MODULE=on go test -mod vendor -timeout=1m -v -race -short -tags "$(LINUX_BUILD_TAGS)" ./...
+	GO111MODULE=on go test -mod vendor -timeout=1m -v -race -short -tags "$(HOST_PLATFORM_BUILD_TAGS)" ./...
 
 e2e-test: vet fmt build-tar
-	GO111MODULE=on ginkgo -nodes=$(PARALLEL) -mod vendor -timeout=10m -v -tags "$(LINUX_BUILD_TAGS)" -stream \
+	GO111MODULE=on ginkgo -nodes=$(PARALLEL) -mod vendor -timeout=10m -v -tags "$(HOST_PLATFORM_BUILD_TAGS)" -stream \
 	./test/e2e/metriconly/... -- \
 	-project=$(PROJECT) -zone=$(ZONE) \
 	-image=$(VM_IMAGE) -image-family=$(IMAGE_FAMILY) -image-project=$(IMAGE_PROJECT) \
@@ -203,7 +217,7 @@ e2e-test: vet fmt build-tar
 	-boskos-project-type=$(BOSKOS_PROJECT_TYPE) -job-name=$(JOB_NAME) \
 	-artifacts-dir=$(ARTIFACTS)
 
-build-binaries: ./bin/node-problem-detector ./bin/log-counter ./bin/health-checker
+build-binaries: ./bin/node-problem-detector ./bin/log-counter ./bin/health-checker $(WINDOWS_BINARIES) $(LINUX_BINARIES)
 
 build-container: build-binaries Dockerfile
 	docker build -t $(IMAGE) --build-arg BASEIMAGE=$(BASEIMAGE) --build-arg LOGCOUNTER=$(LOGCOUNTER) .

config/windows-containerd-monitor-filelog.json

@@ -8,13 +8,18 @@
 	"logPath": "C:\\Program Files\\containerd\\containerd.log",
 	"lookback": "5m",
 	"bufferSize": 10,
-	"source": "docker-monitor",
+	"source": "containerd",
 	"conditions": [],
 	"rules": [
 		{
 			"type": "temporary",
-			"reason": "BadCNIConfig",
-			"pattern": "failed to reload cni configuration.*"
+			"reason": "MissingPigz",
+			"pattern": "unpigz not found.*"
+		},
+		{
+			"type": "temporary",
+			"reason": "IncompatibleContainer",
+			"pattern": ".*CreateComputeSystem.*"
 		}
 	]
 }

pkg/custompluginmonitor/plugin/plugin.go

@@ -29,6 +29,7 @@ import (
 
 	"github.com/golang/glog"
 	cpmtypes "k8s.io/node-problem-detector/pkg/custompluginmonitor/types"
+	"k8s.io/node-problem-detector/pkg/util"
 	"k8s.io/node-problem-detector/pkg/util/tomb"
 )
 
@@ -147,12 +148,7 @@ func (p *Plugin) run(rule cpmtypes.CustomRule) (exitStatus cpmtypes.Status, outp
 	}
 	defer cancel()
 
-	// create a process group
-	sysProcAttr := &syscall.SysProcAttr{
-		Setpgid: true,
-	}
-	cmd := exec.Command(rule.Path, rule.Args...)
-	cmd.SysProcAttr = sysProcAttr
+	cmd := util.Exec(rule.Path, rule.Args...)
 
 	stdoutPipe, err := cmd.StdoutPipe()
 	if err != nil {
@@ -172,6 +168,9 @@ func (p *Plugin) run(rule cpmtypes.CustomRule) (exitStatus cpmtypes.Status, outp
 	waitChan := make(chan struct{})
 	defer close(waitChan)
 
+	var m sync.Mutex
+	timeout := false
+
 	go func() {
 		select {
 		case <-ctx.Done():
@@ -183,7 +182,12 @@ func (p *Plugin) run(rule cpmtypes.CustomRule) (exitStatus cpmtypes.Status, outp
 				glog.Errorf("Error in cmd.Process check %q", rule.Path)
 				break
 			}
-			err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
+
+			m.Lock()
+			timeout = true
+			m.Unlock()
+
+			err := util.Kill(cmd)
 			if err != nil {
 				glog.Errorf("Error in kill process %d, %v", cmd.Process.Pid, err)
 			}
@@ -202,12 +206,12 @@ func (p *Plugin) run(rule cpmtypes.CustomRule) (exitStatus cpmtypes.Status, outp
 
 	wg.Add(2)
 	go func() {
+		defer wg.Done()
 		stdout, stdoutErr = readFromReader(stdoutPipe, maxCustomPluginBufferBytes)
-		wg.Done()
 	}()
 	go func() {
+		defer wg.Done()
 		stderr, stderrErr = readFromReader(stderrPipe, maxCustomPluginBufferBytes)
-		wg.Done()
 	}()
 	// This will wait for the reads to complete. If the execution times out, the pipes
 	// will be closed and the wait group unblocks.
@@ -234,7 +238,11 @@ func (p *Plugin) run(rule cpmtypes.CustomRule) (exitStatus cpmtypes.Status, outp
 	output = string(stdout)
 	output = strings.TrimSpace(output)
 
-	if cmd.ProcessState.Sys().(syscall.WaitStatus).Signaled() {
+	m.Lock()
+	cmdKilled := timeout
+	m.Unlock()
+
+	if cmdKilled {
 		output = fmt.Sprintf("Timeout when running plugin %q: state - %s. output - %q", rule.Path, cmd.ProcessState.String(), output)
 	}
 
@@ -257,6 +265,7 @@ func (p *Plugin) run(rule cpmtypes.CustomRule) (exitStatus cpmtypes.Status, outp
 	}
 }
 
+// Stop the plugin.
 func (p *Plugin) Stop() {
 	p.tomb.Stop()
 	glog.Info("Stop plugin execution")

pkg/custompluginmonitor/plugin/plugin_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package plugin
 
 import (
+	"runtime"
 	"testing"
 	"time"
 
@@ -25,6 +26,13 @@ import (
 
 func TestNewPluginRun(t *testing.T) {
 	ruleTimeout := 1 * time.Second
+	timeoutExitStatus := cpmtypes.Unknown
+	ext := "sh"
+
+	if runtime.GOOS == "windows" {
+		ext = "cmd"
+		timeoutExitStatus = cpmtypes.NonOK
+	}
 
 	utMetas := map[string]struct {
 		Rule       cpmtypes.CustomRule
@@ -33,30 +41,31 @@ func TestNewPluginRun(t *testing.T) {
 	}{
 		"ok": {
 			Rule: cpmtypes.CustomRule{
-				Path:    "./test-data/ok.sh",
+				Path:    "./test-data/ok." + ext,
 				Timeout: &ruleTimeout,
 			},
 			ExitStatus: cpmtypes.OK,
 			Output:     "OK",
 		},
 		"non-ok": {
 			Rule: cpmtypes.CustomRule{
-				Path:    "./test-data/non-ok.sh",
+				Path:    "./test-data/non-ok." + ext,
 				Timeout: &ruleTimeout,
 			},
 			ExitStatus: cpmtypes.NonOK,
 			Output:     "NonOK",
 		},
 		"unknown": {
 			Rule: cpmtypes.CustomRule{
-				Path:    "./test-data/unknown.sh",
+				Path:    "./test-data/unknown." + ext,
 				Timeout: &ruleTimeout,
 			},
 			ExitStatus: cpmtypes.Unknown,
 			Output:     "UNKNOWN",
 		},
 		"non executable": {
 			Rule: cpmtypes.CustomRule{
+				// Intentionally run .sh for Windows, this is meant to be not executable.
 				Path:    "./test-data/non-executable.sh",
 				Timeout: &ruleTimeout,
 			},
@@ -65,45 +74,48 @@ func TestNewPluginRun(t *testing.T) {
 		},
 		"longer than 80 stdout with ok exit status": {
 			Rule: cpmtypes.CustomRule{
-				Path:    "./test-data/longer-than-80-stdout-with-ok-exit-status.sh",
+				Path:    "./test-data/longer-than-80-stdout-with-ok-exit-status." + ext,
 				Timeout: &ruleTimeout,
 			},
 			ExitStatus: cpmtypes.OK,
 			Output:     "01234567890123456789012345678901234567890123456789012345678901234567890123456789",
 		},
 		"non defined exit status": {
 			Rule: cpmtypes.CustomRule{
-				Path:    "./test-data/non-defined-exit-status.sh",
+				Path:    "./test-data/non-defined-exit-status." + ext,
 				Timeout: &ruleTimeout,
 			},
 			ExitStatus: cpmtypes.Unknown,
 			Output:     "NON-DEFINED-EXIT-STATUS",
 		},
 		"sleep 3 second with ok exit status": {
 			Rule: cpmtypes.CustomRule{
-				Path:    "./test-data/sleep-3-second-with-ok-exit-status.sh",
+				Path:    "./test-data/sleep-3-second-with-ok-exit-status." + ext,
 				Timeout: &ruleTimeout,
 			},
-			ExitStatus: cpmtypes.Unknown,
-			Output:     `Timeout when running plugin "./test-data/sleep-3-second-with-ok-exit-status.sh": state - signal: killed. output - ""`,
+			ExitStatus: timeoutExitStatus,
+			Output:     `Timeout when running plugin "./test-data/sleep-3-second-with-ok-exit-status.` + ext + `": state - signal: killed. output - ""`,
 		},
 	}
 
-	conf := cpmtypes.CustomPluginConfig{}
-	(&conf).ApplyConfiguration()
-	p := Plugin{config: conf}
-	for desp, utMeta := range utMetas {
-		gotExitStatus, gotOutput := p.run(utMeta.Rule)
-		// cut at position max_output_length if expected output is longer than max_output_length bytes
-		if len(utMeta.Output) > *p.config.PluginGlobalConfig.MaxOutputLength {
-			utMeta.Output = utMeta.Output[:*p.config.PluginGlobalConfig.MaxOutputLength]
-		}
-		if gotExitStatus != utMeta.ExitStatus || gotOutput != utMeta.Output {
-			t.Errorf("%s", desp)
-			t.Errorf("Error in run plugin and get exit status and output for %q. "+
-				"Got exit status: %v, Expected exit status: %v. "+
-				"Got output: %q, Expected output: %q",
-				utMeta.Rule.Path, gotExitStatus, utMeta.ExitStatus, gotOutput, utMeta.Output)
-		}
+	for k, v := range utMetas {
+		desp := k
+		utMeta := v
+		t.Run(desp, func(t *testing.T) {
+			conf := cpmtypes.CustomPluginConfig{}
+			(&conf).ApplyConfiguration()
+			p := Plugin{config: conf}
+			gotExitStatus, gotOutput := p.run(utMeta.Rule)
+			// cut at position max_output_length if expected output is longer than max_output_length bytes
+			if len(utMeta.Output) > *p.config.PluginGlobalConfig.MaxOutputLength {
+				utMeta.Output = utMeta.Output[:*p.config.PluginGlobalConfig.MaxOutputLength]
+			}
+			if gotExitStatus != utMeta.ExitStatus || gotOutput != utMeta.Output {
+				t.Errorf("Error in run plugin and get exit status and output for %q. "+
+					"Got exit status: %v, Expected exit status: %v. "+
+					"Got output: %q, Expected output: %q",
+					utMeta.Rule.Path, gotExitStatus, utMeta.ExitStatus, gotOutput, utMeta.Output)
+			}
+		})
 	}
 }

pkg/custompluginmonitor/plugin/test-data/longer-than-80-stdout-with-ok-exit-status.cmd

@@ -0,0 +1,4 @@
+@echo off
+
+echo 012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789
+exit 0

pkg/custompluginmonitor/plugin/test-data/non-defined-exit-status.cmd

@@ -0,0 +1,4 @@
+@echo off
+
+echo NON-DEFINED-EXIT-STATUS
+exit 100

pkg/custompluginmonitor/plugin/test-data/non-ok.cmd

@@ -0,0 +1,4 @@
+@echo off
+
+echo NonOK
+exit 1

pkg/custompluginmonitor/plugin/test-data/ok.cmd

@@ -0,0 +1,4 @@
+@echo off
+
+echo OK
+exit 0

pkg/custompluginmonitor/plugin/test-data/sleep-3-second-with-ok-exit-status.cmd

@@ -0,0 +1,5 @@
+@echo off
+
+ping 127.0.0.1 -n 3 > nul
+echo SLEEP 3S SECOND
+exit 0

pkg/custompluginmonitor/plugin/test-data/unknown.cmd

@@ -0,0 +1,4 @@
+@echo off
+
+echo UNKNOWN
+exit 3