From 36f30a3cf3a60d8014bd4098dcc432f63e64b987 Mon Sep 17 00:00:00 2001 From: Leo Palmer Sunmo Date: Tue, 14 Jan 2020 09:57:06 +1300 Subject: [PATCH 1/4] Adds "null" handling to SetClusterSetting This function now takes *string where a nil value is treated as a JSON null value. It allows you to pass the nil *string that is returned as "existingValue" back in as the "newValue" to reset it at the end of an operation. This allows you to "remove" settings by setting them to JSON null. This is the recommended way to remove an explicit configuration to fall back to defaults in Elasticsearch. --- es.go | 42 ++++++++++++++++++++---------- es_test.go | 75 ++++++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 91 insertions(+), 26 deletions(-) diff --git a/es.go b/es.go index ead8b28..b1aeb97 100644 --- a/es.go +++ b/es.go @@ -761,37 +761,53 @@ func (c *Client) SetAllocation(allocation string) (string, error) { return allocationVal.String(), nil } -//Set a new value for a cluster setting +// Set a new value for a cluster setting. Returns existing value and new value as well as error, in that order +// If the setting is not set in Elasticsearch (it's falling back to default configuration) SetClusterSetting's existingValue will be nil. +// If the value provided is nil, SetClusterSetting will remove the setting so that Elasticsearch falls back on default configuration for that setting. // //Use case: You've doubled the number of nodes in your cluster and you want to increase the number of shards the cluster can relocate at one time. Calling `SetClusterSetting("cluster.routing.allocation.cluster_concurrent_rebalance", "100")` will update that value with the cluster. Once data relocation is complete you can decrease the setting by calling `SetClusterSetting("cluster.routing.allocation.cluster_concurrent_rebalance", "20")`. -func (c *Client) SetClusterSetting(setting string, value string) (string, string, error) { - +func (c *Client) SetClusterSetting(setting string, value *string) (*string, *string, error) { + var existingValue *string + var newValue *string settingsBody, err := handleErrWithBytes(c.buildGetRequest(clusterSettingsPath)) if err != nil { - return "", "", err + return existingValue, newValue, err } - existingValues := gjson.GetManyBytes(settingsBody, fmt.Sprintf("transient.%s", setting), fmt.Sprintf("persistent.%s", setting)) + existingResults := gjson.GetManyBytes(settingsBody, fmt.Sprintf("transient.%s", setting), fmt.Sprintf("persistent.%s", setting)) + + var newSettingBody string + + if value == nil { + newSettingBody = fmt.Sprintf(`{"transient" : { "%s" : null}}`, setting) + } else { + newSettingBody = fmt.Sprintf(`{"transient" : { "%s" : "%s"}}`, setting, *value) + } agent := c.buildPutRequest(clusterSettingsPath). Set("Content-Type", "application/json"). - Send(fmt.Sprintf(`{"transient" : { "%s" : "%s"}}`, setting, value)) + Send(newSettingBody) body, err := handleErrWithBytes(agent) if err != nil { - return "", "", err + return existingValue, newValue, err } - newValue := gjson.GetBytes(body, fmt.Sprintf("transient.%s", setting)).String() - - var existingValue string + newResults := gjson.GetBytes(body, fmt.Sprintf("transient.%s", setting)).String() + if newResults != "" { + newValue = &newResults + } - if existingValues[0].String() == "" { - existingValue = existingValues[1].String() + if existingResults[0].String() == "" { + if existingResults[1].String() != "" { + value := existingResults[1].String() + existingValue = &value + } } else { - existingValue = existingValues[0].String() + value := existingResults[0].String() + existingValue = &value } return existingValue, newValue, nil diff --git a/es_test.go b/es_test.go index 2c748a0..dc9d6f9 100644 --- a/es_test.go +++ b/es_test.go @@ -28,6 +28,10 @@ func buildTestServer(t *testing.T, setups []*ServerSetup, tls bool) *httptest.Se matched := false for _, setup := range setups { + // Extra piece of debug incase there's a typo in your test's response, like a rogue space somewhere + if r.Method == setup.Method && r.URL.EscapedPath() == setup.Path && requestBody != setup.Body { + t.Errorf("request body not matching: %s != %s", requestBody, setup.Body) + } if r.Method == setup.Method && r.URL.EscapedPath() == setup.Path && requestBody == setup.Body { matched = true if setup.HTTPStatus == 0 { @@ -77,6 +81,8 @@ func setupTestTLSServers(t *testing.T, setups []*ServerSetup) (string, int, *htt return url.Hostname(), port, ts } +func stringToPointer(v string) *string { return &v } + func TestGetClusterExcludeSettings(t *testing.T) { testSetup := &ServerSetup{ @@ -680,9 +686,9 @@ func TestSetClusterSettings(t *testing.T) { GetResponse string PutResponse string Setting string - SetValue string + SetValue *string HTTPStatus int - OldValue string + OldValue *string }{ { // Tests for behavior with existing transient setting. @@ -691,8 +697,8 @@ func TestSetClusterSettings(t *testing.T) { Body: `{"transient":{"cluster.routing.allocation.exclude._name":"10.0.0.99"}}`, PutResponse: `{"persistent":{},"transient":{"cluster":{"routing":{"allocation":{"exclude":{"_name":"10.0.0.99"}}}}}}`, Setting: "cluster.routing.allocation.exclude._name", - SetValue: "10.0.0.99", - OldValue: "10.0.0.2", + SetValue: stringToPointer("10.0.0.99"), + OldValue: stringToPointer("10.0.0.2"), }, { @@ -702,8 +708,8 @@ func TestSetClusterSettings(t *testing.T) { Body: `{"transient":{"cluster.routing.allocation.exclude._name":"10.0.0.99"}}`, PutResponse: `{"persistent":{},"transient":{"cluster":{"routing":{"allocation":{"exclude":{"_name":"10.0.0.99"}}}}}}`, Setting: "cluster.routing.allocation.exclude._name", - SetValue: "10.0.0.99", - OldValue: "10.0.0.2", + SetValue: stringToPointer("10.0.0.99"), + OldValue: stringToPointer("10.0.0.2"), }, { @@ -713,8 +719,30 @@ func TestSetClusterSettings(t *testing.T) { Body: `{"transient":{"cluster.routing.allocation.exclude._name":"10.0.0.99"}}`, PutResponse: `{"persistent":{},"transient":{"cluster":{"routing":{"allocation":{"exclude":{"_name":"10.0.0.99"}}}}}}`, Setting: "cluster.routing.allocation.exclude._name", - SetValue: "10.0.0.99", - OldValue: "", + SetValue: stringToPointer("10.0.0.99"), + OldValue: nil, + }, + + { + // Tests for behavior when removing setting (null'ing it). + Name: "Removing Existing Persistent Setting", + GetResponse: `{"transient":{},"persistent":{"cluster":{"routing":{"allocation":{"exclude":{"_name":"10.0.0.2"}}}}}}`, + Body: `{"transient":{"cluster.routing.allocation.exclude._name":null}}`, + PutResponse: `{"transient":{},"persistent":{}}`, + Setting: "cluster.routing.allocation.exclude._name", + SetValue: nil, + OldValue: stringToPointer("10.0.0.2"), + }, + + { + // Tests for behavior when removing setting (null'ing it) that is already null. + Name: "Removing Null Persistent Setting", + GetResponse: `{"transient":{},"persistent":{}}`, + Body: `{"transient":{"cluster.routing.allocation.exclude._name":null}}`, + PutResponse: `{"transient":{},"persistent":{}}`, + Setting: "cluster.routing.allocation.exclude._name", + SetValue: nil, + OldValue: nil, }, } @@ -743,14 +771,35 @@ func TestSetClusterSettings(t *testing.T) { st.Errorf("Expected error to be nil, %s", err) } - if oldSetting != x.OldValue { - st.Errorf("Unexpected old value, got %s", oldSetting) + if oldSetting == nil { + if x.OldValue != nil { + st.Fatalf("Unexpected old value: expected old value to be %s, got nil", *x.OldValue) + } + } + + if oldSetting != nil { + if x.OldValue == nil { + st.Fatalf("Unexpected old value: expected old value to be nil, got %v", *oldSetting) + } + if *oldSetting != *x.OldValue { + st.Errorf("Unexpected old value: expected %s, got %s", *x.OldValue, *oldSetting) + } } - if newSetting != "10.0.0.99" { - st.Errorf("Unexpected new value, got %s", newSetting) + if newSetting == nil { + if x.SetValue != nil { + st.Fatalf("Unexpected new value, got nil, expected %s", *x.SetValue) + } } + if newSetting != nil { + if x.SetValue == nil { + st.Errorf("Unexpected new value, got %s, expected nil", *newSetting) + } + if *newSetting != *x.SetValue { + st.Errorf("Unexpected new value, got %v, expected %v", newSetting, x.SetValue) + } + } }) } } @@ -831,7 +880,7 @@ func TestSetClusterSetting_BadRequest(t *testing.T) { defer ts.Close() client := NewClient(host, port) - _, _, err := client.SetClusterSetting("cluster.routing.allocation.enable", "foo") + _, _, err := client.SetClusterSetting("cluster.routing.allocation.enable", stringToPointer("foo")) if err == nil { t.Errorf("Expected error to not be nil, %s", err) From 92d186d61e36b4a02adc9a65a8e70a758287184d Mon Sep 17 00:00:00 2001 From: Leo Palmer Sunmo Date: Tue, 14 Jan 2020 10:00:55 +1300 Subject: [PATCH 2/4] Enable removing a setting using the CLI tool This implements the new "null"ing function in the CLI to allow the user to reset a configuration back to its defaults by using the "--remove" flag. Also adds logic to manage the "--remove" and "--value|-v" flags. Adds a helper function to print nil *strings in a nicer manner. --- pkg/cli/entrypoint.go | 9 ++++++++- pkg/cli/setting.go | 37 +++++++++++++++++++++++++++---------- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/pkg/cli/entrypoint.go b/pkg/cli/entrypoint.go index bb64a07..ea39655 100644 --- a/pkg/cli/entrypoint.go +++ b/pkg/cli/entrypoint.go @@ -5,11 +5,11 @@ import ( "crypto/tls" "crypto/x509" "fmt" - "github.com/github/vulcanizer" "io" "io/ioutil" "os" + "github.com/github/vulcanizer" "github.com/olekukonko/tablewriter" "github.com/spf13/cobra" "github.com/spf13/viper" @@ -113,6 +113,13 @@ func getClient() *vulcanizer.Client { return v } +func printableNil(ptrValue *string) string { + if ptrValue == nil { + return "null" + } + return *ptrValue +} + func renderTable(rows [][]string, header []string) string { var result bytes.Buffer table := tablewriter.NewWriter(&result) diff --git a/pkg/cli/setting.go b/pkg/cli/setting.go index bf2fc1b..c1feea6 100644 --- a/pkg/cli/setting.go +++ b/pkg/cli/setting.go @@ -9,6 +9,8 @@ import ( var settingToUpdate, valueToUpdate string +var removeValue bool + func init() { cmdSettingUpdate.Flags().StringVarP(&settingToUpdate, "setting", "s", "", "Elasticsearch cluster setting to update (required)") @@ -18,12 +20,9 @@ func init() { os.Exit(1) } - cmdSettingUpdate.Flags().StringVarP(&valueToUpdate, "value", "v", "", "Value of the Elasticsearch cluster setting to update (required)") - err = cmdSettingUpdate.MarkFlagRequired("value") - if err != nil { - fmt.Printf("Error binding value configuration flag: %s \n", err) - os.Exit(1) - } + cmdSettingUpdate.Flags().StringVarP(&valueToUpdate, "value", "v", "", "Value of the Elasticsearch cluster setting to update (can't be used with \"--remove\")") + + cmdSettingUpdate.Flags().BoolVar(&removeValue, "remove", false, "Remove provided cluster setting, resetting it to default configuration (can't be used with \"--value|-v\")") cmdSetting.AddCommand(cmdSettingUpdate) rootCmd.AddCommand(cmdSetting) @@ -41,18 +40,36 @@ var cmdSettingUpdate = &cobra.Command{ Long: `This command will update the cluster's settings with the provided value.`, Run: func(cmd *cobra.Command, args []string) { + if cmd.Flags().Changed("value") && cmd.Flags().Changed("remove") { + fmt.Println("Can't set both \"--value|-v\" and \"--remove\" options") + fmt.Print(cmd.UsageString()) + os.Exit(1) + } + if !cmd.Flags().Changed("value") && !cmd.Flags().Changed("remove") { + fmt.Println("Error: requires one of \"--value|-v\" or \"--remove\"") + fmt.Print(cmd.UsageString()) + os.Exit(1) + } v := getClient() - existingValue, newValue, err := v.SetClusterSetting(settingToUpdate, valueToUpdate) + var ptrValueToUpdate *string + + if removeValue { + ptrValueToUpdate = nil + } else { + ptrValueToUpdate = &valueToUpdate + } + + existingValue, newValue, err := v.SetClusterSetting(settingToUpdate, ptrValueToUpdate) if err != nil { - fmt.Printf("Error when trying to update \"%s\" to \"%s\n", settingToUpdate, valueToUpdate) + fmt.Printf("Error when trying to update \"%s\" to \"%s\"\n", settingToUpdate, printableNil(ptrValueToUpdate)) fmt.Printf("Error is: %s\n", err) os.Exit(1) } fmt.Printf("Updated setting %s\n", settingToUpdate) - fmt.Printf("\tOld value: %s\n", existingValue) - fmt.Printf("\tNew value: %s\n", newValue) + fmt.Printf("\tOld value: %s\n", printableNil(existingValue)) + fmt.Printf("\tNew value: %s\n", printableNil(newValue)) }, } From 5640a4c3ddb76dca212729ec03d5d9a26bab232c Mon Sep 17 00:00:00 2001 From: Leo Palmer Sunmo Date: Tue, 14 Jan 2020 10:03:34 +1300 Subject: [PATCH 3/4] Add indirect for module github.com/spf13/pflag go mod why: github.com/github/vulcanizer/pkg/cli github.com/spf13/cobra github.com/spf13/pflag --- go.mod | 1 + go.sum | 2 ++ 2 files changed, 3 insertions(+) diff --git a/go.mod b/go.mod index 1f0a075..f0a35b1 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/pkg/errors v0.8.0 // indirect github.com/smartystreets/goconvey v1.6.4 // indirect github.com/spf13/cobra v0.0.5 + github.com/spf13/pflag v1.0.5 // indirect github.com/spf13/viper v1.3.2 github.com/stretchr/testify v1.4.0 // indirect github.com/tidwall/gjson v1.1.3 diff --git a/go.sum b/go.sum index c5a175d..378d54c 100644 --- a/go.sum +++ b/go.sum @@ -61,6 +61,8 @@ github.com/spf13/jwalterweatherman v1.0.0 h1:XHEdyB+EcvlqZamSM4ZOMGlc93t6AcsBEu9 github.com/spf13/jwalterweatherman v1.0.0/go.mod h1:cQK4TGJAtQXfYWX+Ddv3mKDzgVb68N+wFjFa4jdeBTo= github.com/spf13/pflag v1.0.3 h1:zPAT6CGy6wXeQ7NtTnaTerfKOsV6V6F8agHXFiazDkg= github.com/spf13/pflag v1.0.3/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4= +github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA= +github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/spf13/viper v1.3.2 h1:VUFqw5KcqRf7i70GOzW7N+Q7+gxVBkSSqiXB12+JQ4M= github.com/spf13/viper v1.3.2/go.mod h1:ZiWeW+zYFKm7srdB9IoDzzZXaJaI5eL9QjNiN/DMA2s= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= From 0461d2480a62482e0c047f86b28c437efb928b37 Mon Sep 17 00:00:00 2001 From: Leo Palmer Sunmo Date: Tue, 14 Jan 2020 10:04:16 +1300 Subject: [PATCH 4/4] Check for correct vm.max_map_count in integration Add a check at the beginning of integration check to make sure vm.max_map_count is set correctly as it will cause silent Elasticsearch Docker failures otherwise --- script/integration-test | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/script/integration-test b/script/integration-test index 30f6d8b..27337d6 100755 --- a/script/integration-test +++ b/script/integration-test @@ -1,6 +1,18 @@ #!/bin/bash set -e +# Before we start, check if "vm.max_map_count" is correctly set, or the Docker containers will silently fail and lock the test +maxMap="$(sysctl vm.max_map_count | awk -F "= " '{print $2}')" +if [[ $maxMap -lt 262144 ]] +then + # incorrect config, won't work + echo "You need to increase vm.max_map_count to at least 262144 before Elasticsearch will start" + echo "Currently set to $maxMap" + echo "Run \"sudo sysctl -w vm.max_map_count=262144\" and start the test again." + exit 1 +fi + + # Run regular unit tests first ./script/test