Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements CLI tool for rule management #7712

Merged
merged 5 commits into from
Mar 29, 2021
Merged

Conversation

falun
Copy link
Contributor

@falun falun commented Mar 18, 2021

Description

This PR introduces rulesctl as a command line tool to interface with the filecustomrule method of encoding/configuring the query rules. It initially implements list, remove-rule, and a simplistic version of add-rule.

This makes a few minor refactors but no behavioral to the rules/tablet server code.

Usage Examples

[~/go/src/vitess.io/vitess] (master)
falun@vitess-build-host $ ./rulesctl -f ~/rules.json list
[
  {
    "Description": "This is a test rule",
    "Name": "basic_rule",
    "Query": ".*some_bad_query_identifier.*",
    "Plans": [
      "Select",
      "Insert"
    ],
    "TableNames": [
      "table1",
      "table2"
    ],
    "Action": "FAIL"
  },
  {
    "Description": "A second basic rule",
    "Name": "basic_rule 2",
    "Query": ".*",
    "Plans": [
      "Select"
    ],
    "Action": "FAIL"
  }
]

falun@vitess-build-host $ ./rulesctl -f ~/rules.json list --names-only
[
  "basic_rule",
  "basic_rule 2"
]

falun@vitess-build-host $ ./rulesctl -f ~/rules.json remove-rule -n "basic_rule 2"

falun@vitess-build-host $ ./rulesctl -f ~/rules.json list --names-only
[
  "basic_rule"
]

falun@vitess-build-host $ ./rulesctl -f ~/new-rules.json add-rule -n 'example_rule' -a fail -e 'a new rule' -p select

falun@vitess-build-host $ ./rulesctl -f ~/new-rules.json list
[
  {
    "Description": "a new rule",
    "Name": "example_rule",
    "Query": "",
    "Plans": [
      "Select"
    ],
    "Action": "FAIL"
  }
]

Related Issue(s)

No open issues, relevant channel in community slack #feat-query-block.

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

N/a

Impacted Areas in Vitess

No impact to Vitess deployments. This is a command line tool to facilitate safely constructing config files.

Signed-off-by: falun <635538+falun@users.noreply.github.com>
Signed-off-by: falun <635538+falun@users.noreply.github.com>
Signed-off-by: falun <635538+falun@users.noreply.github.com>
Signed-off-by: falun <635538+falun@users.noreply.github.com>
Copy link
Contributor

@ajm188 ajm188 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing!! I have some minor nitpicks around wording, and then a couple code suggestions, which I think are totally fine to take-or-leave!

go/cmd/rulesctl/add.go Outdated Show resolved Hide resolved
go/cmd/rulesctl/explain.go Outdated Show resolved Hide resolved
go/cmd/rulesctl/main.go Outdated Show resolved Hide resolved
go/cmd/rulesctl/main.go Outdated Show resolved Hide resolved
go/cmd/rulesctl/explain.go Outdated Show resolved Hide resolved
go/cmd/rulesctl/main.go Outdated Show resolved Hide resolved
go/vt/vttablet/customrule/filecustomrule/filecustomrule.go Outdated Show resolved Hide resolved
@derekperkins
Copy link
Member

derekperkins commented Mar 25, 2021

I honestly have no idea what a filecustomrule is, so I'm not qualified to comment on the functionality or usefulness of the new binary, but this goes against what was roughly agreed upon two meetups ago as a goal to reduce the number of binaries #7471.

If we felt like this functionality was important enough, maybe we could stub out the framework for a new vitess binary and let this be the first subcommand? Maybe we could merge this experimentally before doing that, with the understanding that it would likely be the first binary removed as we take an incremental approach to binary consolidation?

Signed-off-by: falun <635538+falun@users.noreply.github.com>
@falun
Copy link
Contributor Author

falun commented Mar 27, 2021

Made requested changes from @ajm188.

@derekperkins While I'm interested in the outcome of the single-binary conversation I don't want to litigate/block as part of this PR. So I took the approach of making everything well factored to be pulled into a universal binary but also build independently as-is.

I think we can also use this approach as a model of what it would look like for the sub-commands off a global vitess; the parent binary's main would basically be:

package main

import (
	"log"

	rulesctl "vitess.io/vitess/go/cmd/rulesctl/cmd"
)

func main() {
	rootCmd := &cobra.Command{
		Name: "vitess",
		...
	}

	rootCmd.AddCommand(vtgate.Main())
	rootCmd.AddCommand(vtctld.Main())
	rootCmd.AddCommand(vttablet.Main())
	rootCmd.AddCommand(rulesctl.Main())
	if err := rootCmd.Execute(); err != nil {
		log.Printf("%v", err)
	}
}

Which will allow specific binaries to be ported in isolation and pulled into a parent vitess binary as they're completed.

I think this should be a reasonable path forward for "we don't currently ship a single binary but may in the future." Thoughts?

@derekperkins
Copy link
Member

I think that looks awesome as a rough framework, and keeping the binary itself well isolated is a good example for how we should isolate the rest of the binaries, regardless of the uni-binary outcome.

@deepthi
Copy link
Member

deepthi commented Mar 29, 2021

I think that looks awesome as a rough framework, and keeping the binary itself well isolated is a good example for how we should isolate the rest of the binaries, regardless of the uni-binary outcome.

+1
@ajm188 can you re-review and approve/merge if all feedback has been addressed?

Copy link
Contributor

@ajm188 ajm188 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lg2m!

@ajm188 ajm188 merged commit 67b3f33 into vitessio:master Mar 29, 2021
@askdba askdba added this to the v10.0 milestone Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants