diff --git a/go.mod b/go.mod index e4950635a..afc88a77b 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/go-kit/log v0.2.1 github.com/google/go-cmp v0.7.0 github.com/google/go-github/v29 v29.0.3 + github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 github.com/nelkinda/health-go v0.0.1 github.com/oklog/run v1.2.0 github.com/prometheus/alertmanager v0.31.1 diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 92e764a53..d244ed4ba 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -127,17 +127,37 @@ func RetryUntilTrue(name string, retryCount int, fn func() (bool, error)) error return fmt.Errorf("Request for '%v' hasn't completed after retrying %d times", name, retryCount) } +// prometheusArgs builds a YAML flow-sequence injection string starting with +// baseArg, followed by any space-separated flags from extraFlags. The returned +// value is intended to be placed inside a double-quoted YAML string: the +// embedded "," separators will break into additional flow-sequence items after +// template rendering, e.g. "--log.level=info","--flag1","--flag2". +func prometheusArgs(baseArg, extraFlags string) string { + parts := []string{baseArg} + for _, f := range strings.Fields(extraFlags) { + parts = append(parts, strings.ReplaceAll(f, `"`, `\"`)) + } + return strings.Join(parts, `","`) +} + // applyTemplateVars applies golang templates to deployment files. func applyTemplateVars(content []byte, deploymentVars map[string]string) ([]byte, error) { fileContentParsed := bytes.NewBufferString("") t := template.New("resource").Option("missingkey=error") t = t.Funcs(template.FuncMap{ - // k8s objects can't have dots(.) se we add a custom function to allow normalising the variable values. + // k8s objects can't have dots(.) so we add a custom function to allow normalising the variable values. "normalise": func(t string) string { return strings.ReplaceAll(t, ".", "-") }, - "split": func(rangeVars, separator string) []string { - return strings.Split(rangeVars, separator) + // prArgs returns a YAML flow-sequence injection string for the PR prometheus + // instance, including --log.level=info and any PR_EXTRA_FLAGS. + "prArgs": func(vars map[string]string) string { + return prometheusArgs("--log.level=info", vars["PR_EXTRA_FLAGS"]) + }, + // mainArgs returns a YAML flow-sequence injection string for the main/release + // prometheus instance, including --log.level=info and any MAIN_EXTRA_FLAGS. + "mainArgs": func(vars map[string]string) string { + return prometheusArgs("--log.level=info", vars["MAIN_EXTRA_FLAGS"]) }, }) if err := template.Must(t.Parse(string(content))).Execute(fileContentParsed, deploymentVars); err != nil { diff --git a/prombench/Makefile b/prombench/Makefile index b3a537146..e88b507ed 100644 --- a/prombench/Makefile +++ b/prombench/Makefile @@ -95,6 +95,8 @@ resource_apply: -v CLUSTER_NAME:${CLUSTER_NAME} \ -v PR_NUMBER:${PR_NUMBER} -v RELEASE:${RELEASE} -v DOMAIN_NAME:${DOMAIN_NAME} \ -v GITHUB_ORG:${GITHUB_ORG} -v GITHUB_REPO:${GITHUB_REPO} \ + $(if $(PR_EXTRA_FLAGS),-v "PR_EXTRA_FLAGS:$(PR_EXTRA_FLAGS)") \ + $(if $(MAIN_EXTRA_FLAGS),-v "MAIN_EXTRA_FLAGS:$(MAIN_EXTRA_FLAGS)") \ -f ${PROMBENCH_DIR}/${BENCHMARK_DIRECTORY}/benchmark # Required because namespace and cluster-role are not part of the created nodes diff --git a/prombench/manifests/cluster-infra/7a_commentmonitor_configmap_noparse.yaml b/prombench/manifests/cluster-infra/7a_commentmonitor_configmap_noparse.yaml index 53a59fc35..e59c0d813 100644 --- a/prombench/manifests/cluster-infra/7a_commentmonitor_configmap_noparse.yaml +++ b/prombench/manifests/cluster-infra/7a_commentmonitor_configmap_noparse.yaml @@ -18,6 +18,8 @@ data: * See the details [here](https://github.com/prometheus/test-infra/tree/master/prombench/manifests/prombench#benchmarking-from-the-different-directory), defaults to `manifests/prombench`. * `--bench.version=` * See the details [here](https://github.com/prometheus/test-infra/tree/master/prombench/manifests/prombench#benchmarking-from-the-custom-test-infra-commitbranch), defaults to `master`. + * `--pr.extra-flags=` - extra flags passed to the PR Prometheus instance. + * `--main.extra-flags=` - extra flags passed to the main/release Prometheus instance. **Examples:** * `/prombench v3.0.0` @@ -37,6 +39,8 @@ data: flag_args: bench.directory: BENCHMARK_DIRECTORY bench.version: BENCHMARK_VERSION + pr.extra-flags: PR_EXTRA_FLAGS + main.extra-flags: MAIN_EXTRA_FLAGS comment_template: | ⏱️ Welcome (again) to Prometheus Benchmarking Tool. ⏱️ @@ -59,7 +63,7 @@ data: - [Parca profiles (e.g. in-use memory)](http://{{ index . "DOMAIN_NAME" }}/profiles?expression_a=memory%3Ainuse_space%3Abytes%3Aspace%3Abytes%7Bpr_number%3D%22{{ index . "PR_NUMBER" }}%22%7D&time_selection_a=relative:minute|15) **Available Commands:** - * To restart benchmark: `/prombench restart {{ index . "RELEASE" }}{{ if index . "BENCHMARK_VERSION" }} --bench.version={{ index . "BENCHMARK_VERSION" }}{{ end }}{{ if index . "BENCHMARK_DIRECTORY" }} --bench.directory={{ index . "BENCHMARK_DIRECTORY" }}{{ end }}` + * To restart benchmark: `/prombench restart {{ index . "RELEASE" }}{{ if index . "BENCHMARK_VERSION" }} --bench.version={{ index . "BENCHMARK_VERSION" }}{{ end }}{{ if index . "BENCHMARK_DIRECTORY" }} --bench.directory={{ index . "BENCHMARK_DIRECTORY" }}{{ end }}{{ if index . "PR_EXTRA_FLAGS" }} --pr.extra-flags={{ index . "PR_EXTRA_FLAGS" }}{{ end }}{{ if index . "MAIN_EXTRA_FLAGS" }} --main.extra-flags={{ index . "MAIN_EXTRA_FLAGS" }}{{ end }}` * To stop benchmark: `/prombench cancel` * To print help: `/prombench help` @@ -70,6 +74,8 @@ data: flag_args: bench.directory: BENCHMARK_DIRECTORY bench.version: BENCHMARK_VERSION + pr.extra-flags: PR_EXTRA_FLAGS + main.extra-flags: MAIN_EXTRA_FLAGS label: prombench comment_template: | ⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️ @@ -93,6 +99,6 @@ data: - [Parca profiles (e.g. in-use memory)](http://{{ index . "DOMAIN_NAME" }}/profiles?expression_a=memory%3Ainuse_space%3Abytes%3Aspace%3Abytes%7Bpr_number%3D%22{{ index . "PR_NUMBER" }}%22%7D&time_selection_a=relative:minute|15) **Available Commands:** - * To restart benchmark: `/prombench restart {{ index . "RELEASE" }}{{ if index . "BENCHMARK_VERSION" }} --bench.version={{ index . "BENCHMARK_VERSION" }}{{ end }}{{ if index . "BENCHMARK_DIRECTORY" }} --bench.directory={{ index . "BENCHMARK_DIRECTORY" }}{{ end }}` + * To restart benchmark: `/prombench restart {{ index . "RELEASE" }}{{ if index . "BENCHMARK_VERSION" }} --bench.version={{ index . "BENCHMARK_VERSION" }}{{ end }}{{ if index . "BENCHMARK_DIRECTORY" }} --bench.directory={{ index . "BENCHMARK_DIRECTORY" }}{{ end }}{{ if index . "PR_EXTRA_FLAGS" }} --pr.extra-flags={{ index . "PR_EXTRA_FLAGS" }}{{ end }}{{ if index . "MAIN_EXTRA_FLAGS" }} --main.extra-flags={{ index . "MAIN_EXTRA_FLAGS" }}{{ end }}` * To stop benchmark: `/prombench cancel` * To print help: `/prombench help` diff --git a/prombench/manifests/prombench/benchmark/5_prometheus-test-pr_deployment.yaml b/prombench/manifests/prombench/benchmark/5_prometheus-test-pr_deployment.yaml index 4278bee38..2fd02a175 100644 --- a/prombench/manifests/prombench/benchmark/5_prometheus-test-pr_deployment.yaml +++ b/prombench/manifests/prombench/benchmark/5_prometheus-test-pr_deployment.yaml @@ -814,7 +814,7 @@ spec: "--web.external-url=http://{{ .DOMAIN_NAME }}/{{ .PR_NUMBER }}/prometheus-pr", "--storage.tsdb.path=/prometheus", "--config.file=/etc/prometheus/prometheus.yml", - "--log.level=info" + "{{prArgs .}}" ] resources: requests: diff --git a/prombench/manifests/prombench/benchmark/5_prometheus-test-release_deployment.yaml b/prombench/manifests/prombench/benchmark/5_prometheus-test-release_deployment.yaml index dda0cd38d..ea98d5353 100644 --- a/prombench/manifests/prombench/benchmark/5_prometheus-test-release_deployment.yaml +++ b/prombench/manifests/prombench/benchmark/5_prometheus-test-release_deployment.yaml @@ -789,7 +789,7 @@ spec: "--web.external-url=http://{{ .DOMAIN_NAME }}/{{ .PR_NUMBER }}/prometheus-release", "--storage.tsdb.path=/prometheus", "--config.file=/etc/prometheus/prometheus.yml", - "--log.level=info" + "{{mainArgs .}}" ] resources: requests: diff --git a/tools/comment-monitor/internal/command.go b/tools/comment-monitor/internal/command.go index 28430aa4a..756c6439f 100644 --- a/tools/comment-monitor/internal/command.go +++ b/tools/comment-monitor/internal/command.go @@ -22,6 +22,7 @@ import ( "strings" "text/template" + "github.com/google/shlex" "gopkg.in/yaml.v2" ) @@ -167,7 +168,13 @@ func ParseCommand(cfg *Config, comment string) (_ *Command, ok bool, err *Comman i = len(comment) } cmdLine := comment[:i] - rest := strings.Split(strings.TrimSpace(cmdLine[len(prefix.Prefix):]), " ") + rest, tErr := shlex.Split(strings.TrimSpace(cmdLine[len(prefix.Prefix):])) + if tErr != nil { + return nil, false, &CommandParseError{ + error: fmt.Errorf("tokenizing command line failed for %v: %w", cmdLine, tErr), + help: fmt.Sprintf("Incorrect `%v` syntax; %v.\n\n%s", prefix.Prefix, tErr.Error(), prefix.Help), + } + } if len(rest) == 0 || rest[0] == "" { return nil, false, &CommandParseError{ error: fmt.Errorf("no matching command found for comment line: %v", cmdLine), @@ -261,21 +268,20 @@ func ParseCommand(cfg *Config, comment string) (_ *Command, ok bool, err *Comman } func parseFlags(rest []string, cfg *CommandConfig, cmd *Command) error { - // TODO(bwplotka: Naive flag parsing, make it support quoting, spaces etc later. for _, flag := range rest { if !strings.HasPrefix(flag, "--") { return fmt.Errorf("expected flag (starting with --), got %v", flag) } - parts := strings.Split(flag, "=") - if len(parts) != 2 { + eqIdx := strings.Index(flag, "=") + if eqIdx == -1 { return fmt.Errorf("expected flag format '--=', got %v", flag) } - argName, ok := cfg.FlagArgs[strings.TrimPrefix(parts[0], "--")] + argName, ok := cfg.FlagArgs[strings.TrimPrefix(flag[:eqIdx], "--")] if !ok { return fmt.Errorf("flag %v is not supported", flag) } - cmd.Args[argName] = parts[1] + cmd.Args[argName] = flag[eqIdx+1:] } return nil } diff --git a/tools/comment-monitor/internal/command_test.go b/tools/comment-monitor/internal/command_test.go index a0014b934..00df62e38 100644 --- a/tools/comment-monitor/internal/command_test.go +++ b/tools/comment-monitor/internal/command_test.go @@ -146,6 +146,22 @@ func TestParseCommand(t *testing.T) { comment: "/prombench v3.0.0 --bench.version=yolo --bench.directory=dir1", expect: testCommand(eventTypeStart, map[string]string{"RELEASE": "v3.0.0", "BENCHMARK_VERSION": "yolo", "BENCHMARK_DIRECTORY": "dir1"}), }, + { + comment: "/prombench v3.0.0 --pr.extra-flags=--enable-feature=foo", + expect: testCommand(eventTypeStart, map[string]string{"RELEASE": "v3.0.0", "PR_EXTRA_FLAGS": "--enable-feature=foo"}), + }, + { + comment: "/prombench v3.0.0 --main.extra-flags=--enable-feature=bar", + expect: testCommand(eventTypeStart, map[string]string{"RELEASE": "v3.0.0", "MAIN_EXTRA_FLAGS": "--enable-feature=bar"}), + }, + { + comment: "/prombench restart v3.0.0 --pr.extra-flags=--enable-feature=foo --main.extra-flags=--enable-feature=bar", + expect: testCommand(eventTypeRestart, map[string]string{"RELEASE": "v3.0.0", "PR_EXTRA_FLAGS": "--enable-feature=foo", "MAIN_EXTRA_FLAGS": "--enable-feature=bar"}), + }, + { + comment: "/prombench v3.0.0 --pr.extra-flags='--enable-feature=foo --enable-feature=bar'", + expect: testCommand(eventTypeStart, map[string]string{"RELEASE": "v3.0.0", "PR_EXTRA_FLAGS": "--enable-feature=foo --enable-feature=bar"}), + }, // Text at the end is generally accepted, after \n. { comment: "/prombench v3.0.0\n", @@ -262,6 +278,22 @@ func TestParseCommand_ProdCommentMonitorConfig(t *testing.T) { comment: "/prombench v3.0.0 --bench.version=mybranch --bench.directory=manifests/prombench", expect: testCommand(eventTypeStart, map[string]string{"RELEASE": "v3.0.0", "BENCHMARK_VERSION": "mybranch", "BENCHMARK_DIRECTORY": "manifests/prombench"}), }, + { + comment: "/prombench v3.0.0 --pr.extra-flags=--enable-feature=foo", + expect: testCommand(eventTypeStart, map[string]string{"RELEASE": "v3.0.0", "PR_EXTRA_FLAGS": "--enable-feature=foo"}), + }, + { + comment: "/prombench v3.0.0 --main.extra-flags=--enable-feature=bar", + expect: testCommand(eventTypeStart, map[string]string{"RELEASE": "v3.0.0", "MAIN_EXTRA_FLAGS": "--enable-feature=bar"}), + }, + { + comment: "/prombench restart v3.0.0 --pr.extra-flags=--enable-feature=foo --main.extra-flags=--enable-feature=bar", + expect: testCommand(eventTypeRestart, map[string]string{"RELEASE": "v3.0.0", "PR_EXTRA_FLAGS": "--enable-feature=foo", "MAIN_EXTRA_FLAGS": "--enable-feature=bar"}), + }, + { + comment: "/prombench v3.0.0 --pr.extra-flags='--enable-feature=foo --enable-feature=bar'", + expect: testCommand(eventTypeStart, map[string]string{"RELEASE": "v3.0.0", "PR_EXTRA_FLAGS": "--enable-feature=foo --enable-feature=bar"}), + }, // Not matching cases. {comment: ""}, {comment: "How to start prombench? I think it was something like:\n\n /prombench main\n\nYolo"}, diff --git a/tools/comment-monitor/internal/testdata/testconfig.yaml b/tools/comment-monitor/internal/testdata/testconfig.yaml index 28aca4c62..323504095 100644 --- a/tools/comment-monitor/internal/testdata/testconfig.yaml +++ b/tools/comment-monitor/internal/testdata/testconfig.yaml @@ -27,6 +27,8 @@ prefixes: flag_args: bench.directory: BENCHMARK_DIRECTORY bench.version: BENCHMARK_VERSION + pr.extra-flags: PR_EXTRA_FLAGS + main.extra-flags: MAIN_EXTRA_FLAGS - name: "" # start is a default (empty command). event_type: prombench_start @@ -35,4 +37,6 @@ prefixes: flag_args: bench.directory: BENCHMARK_DIRECTORY bench.version: BENCHMARK_VERSION + pr.extra-flags: PR_EXTRA_FLAGS + main.extra-flags: MAIN_EXTRA_FLAGS label: prombench