From 25b6c169a20cb896a4d7378f9fd2398ab057c0ea Mon Sep 17 00:00:00 2001 From: gkGaneshR Date: Fri, 2 Mar 2018 21:03:59 +0530 Subject: [PATCH 1/6] Unit testing for SetNodeNameOrDie in package cmd/options 1. Why is this change necessary ? fixes: kubernetes/node-problem-detector#161 2. How does this change address the issue ? Under package cmd/options, the testing for SetNodeNameOrDie need to decide Nodename based on environment variable "NODE_NAME" or hostname or hostnameoverride variable. 3. How to verify this change ? Run "go test" with admin privilege Signed-off-by: gkGaneshR --- cmd/options/options_test.go | 134 ++++++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 cmd/options/options_test.go diff --git a/cmd/options/options_test.go b/cmd/options/options_test.go new file mode 100644 index 00000000..b5255af5 --- /dev/null +++ b/cmd/options/options_test.go @@ -0,0 +1,134 @@ +package options + +import ( + "fmt" + "os" + "os/exec" + "testing" +) + +type Options struct { + Nodename string + Hostname string + HostnameOverride string +} + +//TestSetNodeNameOrDie tests for permutations of nodename, hostname and hostnameoverride +func TestSetNodeNameOrDie(t *testing.T) { + options := map[string]struct { + Expected Options + ObtainedNodeName string + }{ + "Check Node and HostnameOverride only": { + Expected: Options{ + Nodename: "my-node-name", + Hostname: "", + HostnameOverride: "override", + }, + }, + "Check Nodename only": { + Expected: Options{ + Nodename: "my-node-name", + Hostname: "", + HostnameOverride: "", + }, + }, + + "Check HostnameOverride only": { + Expected: Options{ + Nodename: "", + Hostname: "", + HostnameOverride: "override", + }, + }, + "Check Hostname only": { + Expected: Options{ + Nodename: "", + Hostname: "my-host-name", + HostnameOverride: "", + }, + }, + "Check Node, host and HostnameOverride only": { + Expected: Options{ + Nodename: "my-node-name", + Hostname: "my-host-name", + HostnameOverride: "override", + }, + }, + + "Check Host and HostnameOverride only": { + Expected: Options{ + Nodename: "", + Hostname: "my-host-name", + HostnameOverride: "override", + }, + }, + + "Check Node and hostname": { + Expected: Options{ + Nodename: "my-node-name", + Hostname: "my-host-name", + HostnameOverride: "", + }, + }, + } + + orig_node_name := os.Getenv("NODE_NAME") + orig_host_name, err := os.Hostname() + if err != nil { + fmt.Println("Unable to get hostname") + } + + for str, opt := range options { + if opt.Expected.Nodename != "" { + err = os.Setenv("NODE_NAME", opt.Expected.Nodename) + if err != nil { + t.Errorf("Unable to set env NODE_NAME") + } + } + + if opt.Expected.Hostname != "" { + //Changing hostname + cmd := exec.Command("hostname", opt.Expected.Hostname) + _, err := cmd.CombinedOutput() + if err != nil { + //If changing hostname requires admin privilege + cmd = exec.Command("sudo", "hostname", opt.Expected.Hostname) + _, err := cmd.CombinedOutput() + if err != nil { + fmt.Println("Unable to change hostname") + return + } + } + } + + npdObj := NewNodeProblemDetectorOptions() + npdObj.HostnameOverride = opt.Expected.HostnameOverride + npdObj.SetNodeNameOrDie() + opt.ObtainedNodeName = npdObj.NodeName + + if opt.ObtainedNodeName != opt.Expected.HostnameOverride && + opt.ObtainedNodeName != opt.Expected.Nodename && + opt.ObtainedNodeName != opt.Expected.Hostname { + t.Errorf("Error at : %+v", str) + t.Errorf("Wanted: %+v. \nGot: %+v", opt.Expected.Nodename, opt.ObtainedNodeName) + } + + err = os.Setenv("NODE_NAME", "") + if err != nil { + t.Errorf("Unable to set env NODE_NAME empty") + } + + } + + err = os.Setenv("NODE_NAME", orig_node_name) + if err != nil { + fmt.Println("Unable to set original : env NODE_NAME") + } + cmd := exec.Command("sudo", "hostname", orig_host_name) + _, err = cmd.CombinedOutput() + if err != nil { + fmt.Println("Unable to set hostname") + } + +} From 3868f4858605429ff3279e52f762010d2e278e53 Mon Sep 17 00:00:00 2001 From: gkGaneshR Date: Fri, 2 Mar 2018 21:49:07 +0530 Subject: [PATCH 2/6] Include cmd/options in Makefile's test 1. Why is this change necessary ? Solves kubernetes/node-problem-detector#163 2. How does this change address the issue ? Unit testing for node-problem-detector/cmd/options is yet to be done and the corresponding package path needs to be included in makefile test 3. How to verify this change ? make test command should run the test TestSetNodeNameOrDie with admin privileges. Signed-off-by: gkGaneshR --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 1e9a08a8..c6f79881 100644 --- a/Makefile +++ b/Makefile @@ -87,7 +87,7 @@ Dockerfile: Dockerfile.in sed -e 's|@BASEIMAGE@|$(BASEIMAGE)|g' $< >$@ test: vet fmt - go test -timeout=1m -v -race ./pkg/... $(BUILD_TAGS) + go test -timeout=1m -v -race ./cmd/options ./pkg/... $(BUILD_TAGS) build-container: ./bin/node-problem-detector Dockerfile docker build -t $(IMAGE) . From a591ce52f9476818c78501aeb0aa94cc8f9deaad Mon Sep 17 00:00:00 2001 From: gkGaneshR Date: Sun, 4 Mar 2018 21:05:16 +0530 Subject: [PATCH 3/6] Added copyright 2018 statement 1. Why is this change necessary ? Added copyright 2018 statement on options_test.go and added space between // and text on the comments. Signed-off-by: gkGaneshR --- cmd/options/options_test.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/cmd/options/options_test.go b/cmd/options/options_test.go index b5255af5..2ee69e0f 100644 --- a/cmd/options/options_test.go +++ b/cmd/options/options_test.go @@ -1,3 +1,19 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package options import ( @@ -13,7 +29,7 @@ type Options struct { HostnameOverride string } -//TestSetNodeNameOrDie tests for permutations of nodename, hostname and hostnameoverride +// TestSetNodeNameOrDie tests for permutations of nodename, hostname and hostnameoverride func TestSetNodeNameOrDie(t *testing.T) { options := map[string]struct { Expected Options @@ -88,11 +104,11 @@ func TestSetNodeNameOrDie(t *testing.T) { } if opt.Expected.Hostname != "" { - //Changing hostname + // Changing hostname cmd := exec.Command("hostname", opt.Expected.Hostname) _, err := cmd.CombinedOutput() if err != nil { - //If changing hostname requires admin privilege + // If changing hostname requires admin privilege cmd = exec.Command("sudo", "hostname", opt.Expected.Hostname) _, err := cmd.CombinedOutput() if err != nil { @@ -120,7 +136,7 @@ func TestSetNodeNameOrDie(t *testing.T) { } } - + // Setting back the original attributes err = os.Setenv("NODE_NAME", orig_node_name) if err != nil { fmt.Println("Unable to set original : env NODE_NAME") From c75a35099eb5b759a90f78a50cbb014fdc0a4a04 Mon Sep 17 00:00:00 2001 From: gkGaneshR Date: Mon, 5 Mar 2018 13:49:10 +0530 Subject: [PATCH 4/6] Avoided changing hostname and changing var names 1. Why is this change necessary ? The program avoids changing hostname and the variable name "Options" is changed to "options". Also, added more comments and formatted. Removed hostname in options since it will not be changed in tests. 2. How does this change address the issue ? While the program is being run, the hostname is not changed. And options can't be accessed outside(not exported). 3. How to verify this change ? Run, make test Signed-off-by: gkGaneshR --- cmd/options/options_test.go | 100 +++++++++--------------------------- 1 file changed, 25 insertions(+), 75 deletions(-) diff --git a/cmd/options/options_test.go b/cmd/options/options_test.go index 2ee69e0f..c7df5513 100644 --- a/cmd/options/options_test.go +++ b/cmd/options/options_test.go @@ -17,73 +17,43 @@ limitations under the License. package options import ( - "fmt" "os" - "os/exec" "testing" ) -type Options struct { +type options struct { Nodename string - Hostname string HostnameOverride string } // TestSetNodeNameOrDie tests for permutations of nodename, hostname and hostnameoverride func TestSetNodeNameOrDie(t *testing.T) { - options := map[string]struct { - Expected Options + option := map[string]struct { + Expected options ObtainedNodeName string }{ "Check Node and HostnameOverride only": { - Expected: Options{ + Expected: options{ Nodename: "my-node-name", - Hostname: "", HostnameOverride: "override", }, }, "Check Nodename only": { - Expected: Options{ + Expected: options{ Nodename: "my-node-name", - Hostname: "", HostnameOverride: "", }, }, "Check HostnameOverride only": { - Expected: Options{ + Expected: options{ Nodename: "", - Hostname: "", HostnameOverride: "override", }, }, - "Check Hostname only": { - Expected: Options{ + "Check empty": { + Expected: options{ Nodename: "", - Hostname: "my-host-name", - HostnameOverride: "", - }, - }, - "Check Node, host and HostnameOverride only": { - Expected: Options{ - Nodename: "my-node-name", - Hostname: "my-host-name", - HostnameOverride: "override", - }, - }, - - "Check Host and HostnameOverride only": { - Expected: Options{ - Nodename: "", - Hostname: "my-host-name", - HostnameOverride: "override", - }, - }, - - "Check Node and hostname": { - Expected: Options{ - Nodename: "my-node-name", - Hostname: "my-host-name", HostnameOverride: "", }, }, @@ -92,59 +62,39 @@ func TestSetNodeNameOrDie(t *testing.T) { orig_node_name := os.Getenv("NODE_NAME") orig_host_name, err := os.Hostname() if err != nil { - fmt.Println("Unable to get hostname") + t.Errorf("Unable to get hostname") } - for str, opt := range options { - if opt.Expected.Nodename != "" { - err = os.Setenv("NODE_NAME", opt.Expected.Nodename) - if err != nil { - t.Errorf("Unable to set env NODE_NAME") - } - } + for str, opt := range option { - if opt.Expected.Hostname != "" { - // Changing hostname - cmd := exec.Command("hostname", opt.Expected.Hostname) - _, err := cmd.CombinedOutput() - if err != nil { - // If changing hostname requires admin privilege - cmd = exec.Command("sudo", "hostname", opt.Expected.Hostname) - _, err := cmd.CombinedOutput() - if err != nil { - fmt.Println("Unable to change hostname") - return - } - } + // Setting with expected(desired) NODE_NAME env + err = os.Setenv("NODE_NAME", opt.Expected.Nodename) + if err != nil { + t.Errorf("Unable to set env NODE_NAME") } npdObj := NewNodeProblemDetectorOptions() + + // Setting with expected(desired) HostnameOverride npdObj.HostnameOverride = opt.Expected.HostnameOverride + npdObj.SetNodeNameOrDie() opt.ObtainedNodeName = npdObj.NodeName + // Setting back the original node name + err = os.Setenv("NODE_NAME", orig_node_name) + if err != nil { + t.Errorf("Unable to set original : env NODE_NAME") + } + + // Checking for obtained node name if opt.ObtainedNodeName != opt.Expected.HostnameOverride && opt.ObtainedNodeName != opt.Expected.Nodename && - opt.ObtainedNodeName != opt.Expected.Hostname { + opt.ObtainedNodeName != orig_host_name { t.Errorf("Error at : %+v", str) t.Errorf("Wanted: %+v. \nGot: %+v", opt.Expected.Nodename, opt.ObtainedNodeName) } - err = os.Setenv("NODE_NAME", "") - if err != nil { - t.Errorf("Unable to set env NODE_NAME empty") - } - - } - // Setting back the original attributes - err = os.Setenv("NODE_NAME", orig_node_name) - if err != nil { - fmt.Println("Unable to set original : env NODE_NAME") - } - cmd := exec.Command("sudo", "hostname", orig_host_name) - _, err = cmd.CombinedOutput() - if err != nil { - fmt.Println("Unable to set hostname") } } From ca76dc12ee3a0dfb9d62669054bead86c4de903f Mon Sep 17 00:00:00 2001 From: gkGaneshR Date: Fri, 9 Mar 2018 12:08:05 +0530 Subject: [PATCH 5/6] Add dot(.) at the end of comments Signed-off-by: gkGaneshR --- cmd/options/options_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/options/options_test.go b/cmd/options/options_test.go index c7df5513..f60ba3ce 100644 --- a/cmd/options/options_test.go +++ b/cmd/options/options_test.go @@ -26,7 +26,7 @@ type options struct { HostnameOverride string } -// TestSetNodeNameOrDie tests for permutations of nodename, hostname and hostnameoverride +// TestSetNodeNameOrDie tests for permutations of nodename, hostname and hostnameoverride. func TestSetNodeNameOrDie(t *testing.T) { option := map[string]struct { Expected options @@ -67,7 +67,7 @@ func TestSetNodeNameOrDie(t *testing.T) { for str, opt := range option { - // Setting with expected(desired) NODE_NAME env + // Setting with expected(desired) NODE_NAME env. err = os.Setenv("NODE_NAME", opt.Expected.Nodename) if err != nil { t.Errorf("Unable to set env NODE_NAME") @@ -75,19 +75,19 @@ func TestSetNodeNameOrDie(t *testing.T) { npdObj := NewNodeProblemDetectorOptions() - // Setting with expected(desired) HostnameOverride + // Setting with expected(desired) HostnameOverride. npdObj.HostnameOverride = opt.Expected.HostnameOverride npdObj.SetNodeNameOrDie() opt.ObtainedNodeName = npdObj.NodeName - // Setting back the original node name + // Setting back the original node name. err = os.Setenv("NODE_NAME", orig_node_name) if err != nil { t.Errorf("Unable to set original : env NODE_NAME") } - // Checking for obtained node name + // Checking for obtained node name. if opt.ObtainedNodeName != opt.Expected.HostnameOverride && opt.ObtainedNodeName != opt.Expected.Nodename && opt.ObtainedNodeName != orig_host_name { From 821b8f41aa0923ef921f0a69a48a332f9dd0cca6 Mon Sep 17 00:00:00 2001 From: gkGaneshR Date: Fri, 9 Mar 2018 13:17:52 +0530 Subject: [PATCH 6/6] Modify unit testing of cmd/options 1. Why is this change necessary ? Modify unit testing of options in such a way that we specify the WantedNodeName 2. How to verify this change ? Run, make test Signed-off-by: gkGaneshR --- cmd/options/options_test.go | 112 ++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 64 deletions(-) diff --git a/cmd/options/options_test.go b/cmd/options/options_test.go index f60ba3ce..34a15b42 100644 --- a/cmd/options/options_test.go +++ b/cmd/options/options_test.go @@ -28,73 +28,57 @@ type options struct { // TestSetNodeNameOrDie tests for permutations of nodename, hostname and hostnameoverride. func TestSetNodeNameOrDie(t *testing.T) { - option := map[string]struct { - Expected options - ObtainedNodeName string - }{ - "Check Node and HostnameOverride only": { - Expected: options{ - Nodename: "my-node-name", - HostnameOverride: "override", - }, - }, - "Check Nodename only": { - Expected: options{ - Nodename: "my-node-name", - HostnameOverride: "", - }, - }, - - "Check HostnameOverride only": { - Expected: options{ - Nodename: "", - HostnameOverride: "override", - }, - }, - "Check empty": { - Expected: options{ - Nodename: "", - HostnameOverride: "", - }, - }, - } - - orig_node_name := os.Getenv("NODE_NAME") - orig_host_name, err := os.Hostname() + hostName, err := os.Hostname() if err != nil { - t.Errorf("Unable to get hostname") + t.Errorf("Query hostname error: %v", err) } - for str, opt := range option { - - // Setting with expected(desired) NODE_NAME env. - err = os.Setenv("NODE_NAME", opt.Expected.Nodename) - if err != nil { - t.Errorf("Unable to set env NODE_NAME") - } - - npdObj := NewNodeProblemDetectorOptions() - - // Setting with expected(desired) HostnameOverride. - npdObj.HostnameOverride = opt.Expected.HostnameOverride - - npdObj.SetNodeNameOrDie() - opt.ObtainedNodeName = npdObj.NodeName - - // Setting back the original node name. - err = os.Setenv("NODE_NAME", orig_node_name) - if err != nil { - t.Errorf("Unable to set original : env NODE_NAME") - } - - // Checking for obtained node name. - if opt.ObtainedNodeName != opt.Expected.HostnameOverride && - opt.ObtainedNodeName != opt.Expected.Nodename && - opt.ObtainedNodeName != orig_host_name { - t.Errorf("Error at : %+v", str) - t.Errorf("Wanted: %+v. \nGot: %+v", opt.Expected.Nodename, opt.ObtainedNodeName) - } - + uts := map[string]struct { + WantedNodeName string + Meta options + }{ + "Check hostname override only": { + WantedNodeName: "hostname-override", + Meta: options{ + Nodename: "node-name-env", + HostnameOverride: "hostname-override", + }, + }, + "Check hostname override and NDDE_NAME env": { + WantedNodeName: "node-name-env", + Meta: options{ + Nodename: "node-name-env", + HostnameOverride: "", + }, + }, + "Check hostname override, NODE_NAME env and hostname": { + WantedNodeName: hostName, + Meta: options{ + Nodename: "", + HostnameOverride: "", + }, + }, } + for desc, ut := range uts { + err := os.Unsetenv("NODE_NAME") + if err != nil { + t.Errorf("Desc: %v. Unset NODE_NAME env error: %v", desc, err) + } + + if len(ut.Meta.Nodename) != 0 { + err := os.Setenv("NODE_NAME", ut.Meta.Nodename) + if err != nil { + t.Errorf("Desc: %v. Set NODE_NAME env error: %v", desc, err) + } + } + + npdOpts := NewNodeProblemDetectorOptions() + npdOpts.HostnameOverride = ut.Meta.HostnameOverride + npdOpts.SetNodeNameOrDie() + + if npdOpts.NodeName != ut.WantedNodeName { + t.Errorf("Desc: %v. Set node name error. Wanted: %v. Got: %v", desc, ut.WantedNodeName, npdOpts.NodeName) + } + } }