From 689dfbe1c8d65a5cf6474b5ed48280768208971e Mon Sep 17 00:00:00 2001 From: Balaji Subramaniam Date: Fri, 16 Dec 2016 15:50:48 -0800 Subject: [PATCH] Improved unit test coverage. - Refactored code for testing purposes. - Misc. comment changes. --- main.go | 67 ++++++++++++++++----- main_test.go | 162 ++++++++++++++++++++++++++++++++++++++++++++++++++- sources.go | 10 +++- 3 files changed, 221 insertions(+), 18 deletions(-) diff --git a/main.go b/main.go index 66fec1519..29cfcc433 100644 --- a/main.go +++ b/main.go @@ -70,6 +70,29 @@ func main() { stderrLogger.Fatalf("main.version not set! Set -ldflags \"-X main.version `git describe --tags --dirty --always`\" during build or run.") } + // Parse command-line arguments. + noPublish, sourcesArg, whiteListArg := argsParse(nil) + + // Configure the parameters for feature discovery. + sources, labelWhiteList, err := configureParameters(sourcesArg, whiteListArg) + if err != nil { + stderrLogger.Fatalf("error occured while configuring parameters: %s", err.Error()) + } + + // Get the set of feature labels. + labels := createFeatureLabels(sources, labelWhiteList) + + helper := APIHelpers(k8sHelpers{}) + // Update the node with the feature labels. + err = updateNodeWithFeatureLabels(helper, noPublish, labels) + if err != nil { + stderrLogger.Fatalf("error occured while updating node with feature labels: %s", err.Error()) + } +} + +// argsParse parses the command line arguments passed to the program. +// The argument argv is passed only for testing purposes. +func argsParse(argv []string) (noPublish bool, sourcesArg []string, whiteListArg string) { usage := fmt.Sprintf(`%s. Usage: @@ -92,14 +115,20 @@ func main() { ProgramName, ) - arguments, _ := docopt.Parse(usage, nil, true, + arguments, _ := docopt.Parse(usage, argv, true, fmt.Sprintf("%s %s", ProgramName, version), false) // Parse argument values as usable types. - noPublish := arguments["--no-publish"].(bool) - sourcesArg := strings.Split(arguments["--sources"].(string), ",") - whiteListArg := arguments["--label-whitelist"].(string) + noPublish = arguments["--no-publish"].(bool) + sourcesArg = strings.Split(arguments["--sources"].(string), ",") + whiteListArg = arguments["--label-whitelist"].(string) + return noPublish, sourcesArg, whiteListArg +} + +// configureParameters returns all the variables required to perform feature +// discovery based on command line arguments. +func configureParameters(sourcesArg []string, whiteListArg string) (sources []FeatureSource, labelWhiteList *regexp.Regexp, err error) { enabledSources := map[string]struct{}{} for _, s := range sourcesArg { enabledSources[strings.TrimSpace(s)] = struct{}{} @@ -113,20 +142,27 @@ func main() { fakeSource{}, } - sources := []FeatureSource{} + sources = []FeatureSource{} for _, s := range allSources { if _, enabled := enabledSources[s.Name()]; enabled { sources = append(sources, s) } } - // compile whiteListArg regex - labelWhiteList, err := regexp.Compile(whiteListArg) + // Compile whiteListArg regex + labelWhiteList, err = regexp.Compile(whiteListArg) if err != nil { - stderrLogger.Fatalf("Error parsing whitelist regex (%s): %s", whiteListArg, err) + stderrLogger.Printf("error parsing whitelist regex (%s): %s", whiteListArg, err) + return nil, nil, err } - labels := Labels{} + return sources, labelWhiteList, nil +} + +// createFeatureLabels returns the set of feature labels from the enabled +// sources and the whitelist argument. +func createFeatureLabels(sources []FeatureSource, labelWhiteList *regexp.Regexp) (labels Labels) { + labels = Labels{} // Add the version of this discovery code as a node label versionLabel := fmt.Sprintf("%s/%s.version", Namespace, ProgramName) labels[versionLabel] = version @@ -148,21 +184,26 @@ func main() { stdoutLogger.Printf("%s = %s", name, value) // Skip if label doesn't match labelWhiteList if !labelWhiteList.Match([]byte(name)) { - stderrLogger.Printf("%s does not match the whitelist (%s) and will not be published.", name, whiteListArg) + stderrLogger.Printf("%s does not match the whitelist (%s) and will not be published.", name, labelWhiteList.String()) continue } labels[name] = value } } + return labels +} - // Update the node with the node labels, unless disabled via flags. +// updateNodeWithFeatureLabels updates the node with the feature labels, unless +// disabled via --no-publish flag. +func updateNodeWithFeatureLabels(helper APIHelpers, noPublish bool, labels Labels) error { if !noPublish { - helper := APIHelpers(k8sHelpers{}) err := advertiseFeatureLabels(helper, labels) if err != nil { - stderrLogger.Fatalf("failed to advertise labels: %s", err.Error()) + stderrLogger.Printf("failed to advertise labels: %s", err.Error()) + return err } } + return nil } // getFeatureLabels returns node labels for features discovered by the diff --git a/main_test.go b/main_test.go index d0bdb59c6..76f82f3b2 100644 --- a/main_test.go +++ b/main_test.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "regexp" "testing" . "github.com/smartystreets/goconvey/convey" @@ -52,19 +53,31 @@ func TestDiscoveryWithMockSources(t *testing.T) { var mockClient *k8sclient.Clientset var mockNode *api.Node - Convey("When I successfully advertise feature labels to a node", func() { + Convey("When I successfully update the node with feature labels", func() { mockAPIHelper.On("GetClient").Return(mockClient, nil) mockAPIHelper.On("GetNode", mockClient).Return(mockNode, nil).Once() mockAPIHelper.On("AddLabels", mockNode, fakeFeatureLabels).Return().Once() mockAPIHelper.On("RemoveLabels", mockNode, prefix).Return().Once() mockAPIHelper.On("UpdateNode", mockClient, mockNode).Return(nil).Once() - err := advertiseFeatureLabels(testHelper, fakeFeatureLabels) + noPublish := false + err := updateNodeWithFeatureLabels(testHelper, noPublish, fakeFeatureLabels) Convey("Error is nil", func() { So(err, ShouldBeNil) }) }) + Convey("When I fail to update the node with feature labels", func() { + expectedError := errors.New("fake error") + mockAPIHelper.On("GetClient").Return(nil, expectedError) + noPublish := false + err := updateNodeWithFeatureLabels(testHelper, noPublish, fakeFeatureLabels) + + Convey("Error is produced", func() { + So(err, ShouldEqual, expectedError) + }) + }) + Convey("When I fail to get a mock client while advertising feature labels", func() { expectedError := errors.New("fake error") mockAPIHelper.On("GetClient").Return(nil, expectedError) @@ -103,6 +116,151 @@ func TestDiscoveryWithMockSources(t *testing.T) { }) } +func TestArgsParse(t *testing.T) { + Convey("When parsing command line arguments", t, func() { + argv1 := []string{"--no-publish"} + argv2 := []string{"--sources=fake1,fake2,fake3"} + argv3 := []string{"--label-whitelist=.*rdt.*"} + argv4 := []string{"--no-publish", "--sources=fake1,fake2,fake3"} + + Convey("When --no-publish flag is passed", func() { + noPublish, sourcesArg, whiteListArg := argsParse(argv1) + + Convey("noPublish is set and sourcesArg is set to the default value", func() { + So(noPublish, ShouldBeTrue) + So(sourcesArg, ShouldResemble, []string{"cpuid", "rdt", "pstate"}) + So(len(whiteListArg), ShouldEqual, 0) + }) + }) + + Convey("When --sources flag is passed and set to some values", func() { + noPublish, sourcesArg, whiteListArg := argsParse(argv2) + + Convey("sourcesArg is set to appropriate values", func() { + So(noPublish, ShouldBeFalse) + So(sourcesArg, ShouldResemble, []string{"fake1", "fake2", "fake3"}) + So(len(whiteListArg), ShouldEqual, 0) + }) + }) + + Convey("When --label-whitelist flag is passed and set to some value", func() { + noPublish, sourcesArg, whiteListArg := argsParse(argv3) + + Convey("whiteListArg is set to appropriate value and sourcesArg is set to default value", func() { + So(noPublish, ShouldBeFalse) + So(sourcesArg, ShouldResemble, []string{"cpuid", "rdt", "pstate"}) + So(whiteListArg, ShouldResemble, ".*rdt.*") + }) + }) + + Convey("When --no-publish and --sources flag are passed and --sources flag is set to some value", func() { + noPublish, sourcesArg, whiteListArg := argsParse(argv4) + + Convey("--no-publish is set and sourcesArg is set to appropriate values", func() { + So(noPublish, ShouldBeTrue) + So(sourcesArg, ShouldResemble, []string{"fake1", "fake2", "fake3"}) + So(len(whiteListArg), ShouldEqual, 0) + }) + }) + }) +} + +func TestConfigureParameters(t *testing.T) { + Convey("When configuring parameters for node feature discovery", t, func() { + + Convey("When no sourcesArg and whiteListArg are passed", func() { + sourcesArg := []string{} + whiteListArg := "" + emptyRegexp, _ := regexp.Compile("") + sources, labelWhiteList, err := configureParameters(sourcesArg, whiteListArg) + + Convey("Error should not be produced", func() { + So(err, ShouldBeNil) + }) + Convey("No sources or labelWhiteList are returned", func() { + So(len(sources), ShouldEqual, 0) + So(labelWhiteList, ShouldResemble, emptyRegexp) + }) + }) + + Convey("When sourcesArg is passed", func() { + sourcesArg := []string{"fake"} + whiteListArg := "" + emptyRegexp, _ := regexp.Compile("") + sources, labelWhiteList, err := configureParameters(sourcesArg, whiteListArg) + + Convey("Error should not be produced", func() { + So(err, ShouldBeNil) + }) + Convey("Proper sources are returned", func() { + So(len(sources), ShouldEqual, 1) + So(sources[0], ShouldHaveSameTypeAs, fakeSource{}) + So(labelWhiteList, ShouldResemble, emptyRegexp) + }) + }) + + Convey("When invalid whiteListArg is passed", func() { + sourcesArg := []string{""} + whiteListArg := "*" + sources, labelWhiteList, err := configureParameters(sourcesArg, whiteListArg) + + Convey("Error is produced", func() { + So(sources, ShouldBeNil) + So(labelWhiteList, ShouldBeNil) + So(err, ShouldNotBeNil) + }) + }) + + Convey("When valid whiteListArg is passed", func() { + sourcesArg := []string{""} + whiteListArg := ".*rdt.*" + expectRegexp, err := regexp.Compile(".*rdt.*") + sources, labelWhiteList, err := configureParameters(sourcesArg, whiteListArg) + + Convey("Error should not be produced", func() { + So(err, ShouldBeNil) + }) + Convey("Proper labelWhiteList is returned", func() { + So(len(sources), ShouldEqual, 0) + So(labelWhiteList, ShouldResemble, expectRegexp) + }) + }) + }) +} + +func TestCreateFeatureLabels(t *testing.T) { + Convey("When creating feature labels from the configured sources", t, func() { + Convey("When fake feature source is configured", func() { + emptyLabelWL, _ := regexp.Compile("") + fakeFeatureSource := FeatureSource(new(fakeSource)) + sources := []FeatureSource{} + sources = append(sources, fakeFeatureSource) + labels := createFeatureLabels(sources, emptyLabelWL) + + Convey("Proper fake labels are returned", func() { + So(len(labels), ShouldEqual, 4) + So(labels, ShouldContainKey, prefix+"-fake-fakefeature1") + So(labels, ShouldContainKey, prefix+"-fake-fakefeature2") + So(labels, ShouldContainKey, prefix+"-fake-fakefeature3") + }) + }) + Convey("When fake feature source is configured with a whitelist that doesn't match", func() { + emptyLabelWL, _ := regexp.Compile(".*rdt.*") + fakeFeatureSource := FeatureSource(new(fakeSource)) + sources := []FeatureSource{} + sources = append(sources, fakeFeatureSource) + labels := createFeatureLabels(sources, emptyLabelWL) + + Convey("fake labels are not returned", func() { + So(len(labels), ShouldEqual, 1) + So(labels, ShouldNotContainKey, prefix+"-fake-fakefeature1") + So(labels, ShouldNotContainKey, prefix+"-fake-fakefeature2") + So(labels, ShouldNotContainKey, prefix+"-fake-fakefeature3") + }) + }) + }) +} + func TestAddLabels(t *testing.T) { Convey("When adding labels", t, func() { helper := k8sHelpers{} diff --git a/sources.go b/sources.go index 22380a1f1..057388764 100644 --- a/sources.go +++ b/sources.go @@ -30,6 +30,8 @@ const ( type cpuidSource struct{} func (s cpuidSource) Name() string { return "cpuid" } + +// Returns feature names for all the supported CPU features. func (s cpuidSource) Discover() ([]string, error) { // Get the cpu features as strings return cpuid.CPU.Features.Strings(), nil @@ -43,7 +45,7 @@ type rdtSource struct{} func (s rdtSource) Name() string { return "rdt" } -// Returns feature names for CMT, MBM and CAT if suppported. +// Returns feature names for CMT and CAT if suppported. func (s rdtSource) Discover() ([]string, error) { features := []string{} @@ -59,7 +61,7 @@ func (s rdtSource) Discover() ([]string, error) { if err := cmd.Run(); err != nil { stderrLogger.Printf("support for RDT L3 allocation was not detected: %s", err.Error()) } else { - // RDT monitoring detected. + // RDT L3 cache allocation detected. features = append(features, "RDTL3CA") } @@ -67,7 +69,7 @@ func (s rdtSource) Discover() ([]string, error) { if err := cmd.Run(); err != nil { stderrLogger.Printf("support for RDT L2 allocation was not detected: %s", err.Error()) } else { - // RDT monitoring detected. + // RDT L2 cache allocation detected. features = append(features, "RDTL2CA") } @@ -81,6 +83,8 @@ func (s rdtSource) Discover() ([]string, error) { type pstateSource struct{} func (s pstateSource) Name() string { return "pstate" } + +// Returns feature names for p-state related features such as turbo boost. func (s pstateSource) Discover() ([]string, error) { features := []string{}