roachprod: making roachprod subcommands point to a new library

[healthy-pod]

Previously, roachprod binary interfaced directly with roachorod's functionality
and there was no way for another tool to make use of that functionality.

This needed to change to create a library that can be used by roachprod binary
and also other tools.

This patch migrates the subcommands functionality to a new library and makes
the binary point to the new library.

Release note: None

[cockroach-teamcity]

This change is 

[rail]

Wooo! Sounds very exciting. This is my first pass.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @healthy-pod)

---

pkg/cmd/roachprod/main.go, line 628 at r1 (raw file):

	Args:  cobra.NoArgs,
	Run: wrap(func(cmd *cobra.Command, args []string) error {
		cachedHosts, err := roachprod.CachedHosts(cachedHostsCluster)

Looks like you moved the filtering logic from the CLI to the library. I like the idea, and think it would be even better if you can make the filtering part more flexible. In other words, let's not hardvcode the filtering logic and limit it to "teamcity" - we may change the CI or the naming schema at some point. Maybe add another parameter (optional?) where you pass either the prefix you want to exclude, or a function that would decide whether a hostname should be accepted or not. What do you think?

---

pkg/cmd/roachprod/main.go, line 686 at r1 (raw file):

	Args: cobra.RangeArgs(0, 1),
	Run: wrap(func(cmd *cobra.Command, args []string) error {
		return roachprod.List(quiet, listMine, listDetails, listJSON, args)

A nit: roachprod.SetupSSH() uses quiet as its last argument. I'd keep it consistent. Also it caries the least amount of mental load, so let's move it to the end. :)

Also, it feels like List() should return something that you print here, not only an error. Otherwise the library consumers would need to parse the output. In other words, the library shouldn't print anything, just let the consumers do that.

---

pkg/cmd/roachprod/main.go, line 851 at r1 (raw file):

	Args: cobra.ExactArgs(1),
	Run: wrap(func(cmd *cobra.Command, args []string) error {
		newClusterOpts := roachprod.NewClusterOpts{

Hmm, newClusterOpts sounds like something for a new cluster. In other words, I was expecting to have something different for existing clusters. Maybe remove the "New" prefix? Or am I missing something?

---

pkg/cmd/roachprod/main.go, line 856 at r1 (raw file):

			NodeEnv: nodeEnv, NumRacks: numRacks, MaxConcurrency: maxConcurrency,
		}
		return roachprod.Extend(newClusterOpts, extendLifetime, username)

Same here. The printing part should be done in main().

---

pkg/cmd/roachprod/main.go, line 1038 at r1 (raw file):

			NodeEnv: nodeEnv, NumRacks: numRacks, MaxConcurrency: maxConcurrency,
		}
		return roachprod.Monitor(newClusterOpts, monitorIgnoreEmptyNodes, monitorOneShot)

Same here. Print stuff here.

---

pkg/cmd/roachprod/main.go, line 1277 at r1 (raw file):

			NodeEnv: nodeEnv, NumRacks: numRacks, MaxConcurrency: maxConcurrency,
		}
		return roachprod.SQL(newClusterOpts, args[1:])

Feels like this operation should return something, not quite sure what format we should use though...

---

pkg/cmd/roachprod/main.go, line 1293 at r1 (raw file):

			NodeEnv: nodeEnv, NumRacks: numRacks, MaxConcurrency: maxConcurrency,
		}
		return roachprod.PgURL(newClusterOpts, external)

Something similar here. The library prints things instead of main().

---

pkg/cmd/roachprod/main.go, line 1454 at r1 (raw file):

			NodeEnv: nodeEnv, NumRacks: numRacks, MaxConcurrency: maxConcurrency,
		}
		return roachprod.AdminUrl(newClusterOpts, adminurlIPs, adminurlOpen, adminurlPath)

Same same. Print stuff here :)

---

pkg/roachprod/roachprod.go, line 89 at r1 (raw file):

	// Acquire a filesystem lock so that two concurrent synchronizations of
	// roachprod state don't clobber each other.
	f, err := os.Create(lockFile)

Hmm, maybe we should also clean up this file after the operation is done. Apparently I have it :)

---

pkg/roachprod/roachprod.go, line 138 at r1 (raw file):

	if refreshDNS {
		if !quiet {
			fmt.Println("Refreshing DNS entries...")

I'd probably search for every fmt in this file and either replace it with logging or make sure that the printed value is returned to the caller.

---

pkg/roachprod/roachprod.go, line 207 at r1 (raw file):

	if listJSON {
		if listDetails {
			return errors.New("--json cannot be combined with --detail")

Can you move this check before you call Sync(). It'd be better to fail faster, without any network operations.

---

pkg/roachprod/utils.go, line 23 at r1 (raw file):

	"github.com/cockroachdb/cockroach/pkg/cmd/roachprod/config"
	"github.com/cockroachdb/cockroach/pkg/cmd/roachprod/install"
	"github.com/cockroachdb/cockroach/pkg/cmd/roachprod/vm"

I think this is OK for now, but it feels that a library shouldn't use anything from pkg/cmd in its final version.

Also, I'm not a big fan of file names like "util", "common", "misc", etc. :) Not sure how to name this one, but we can probably move some parts of it to other files.

[healthy-pod]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @healthy-pod and @rail)

---

pkg/cmd/roachprod/main.go, line 628 at r1 (raw file):

Previously, rail (Rail Aliiev) wrote…

Looks like you moved the filtering logic from the CLI to the library. I like the idea, and think it would be even better if you can make the filtering part more flexible. In other words, let's not hardvcode the filtering logic and limit it to "teamcity" - we may change the CI or the naming schema at some point. Maybe add another parameter (optional?) where you pass either the prefix you want to exclude, or a function that would decide whether a hostname should be accepted or not. What do you think?

Changed the library implementation to return unfiltered hosts to give users more flexibility with their filtering logic.

---

pkg/cmd/roachprod/main.go, line 686 at r1 (raw file):

Previously, rail (Rail Aliiev) wrote…

A nit: roachprod.SetupSSH() uses quiet as its last argument. I'd keep it consistent. Also it caries the least amount of mental load, so let's move it to the end. :)

Also, it feels like List() should return something that you print here, not only an error. Otherwise the library consumers would need to parse the output. In other words, the library shouldn't print anything, just let the consumers do that.

quiet is no longer passed as a parameter to roachprod.SetupSSH() as I realized it's part of ClusterOpts struct (or install.SyncedCluster).

---

pkg/cmd/roachprod/main.go, line 851 at r1 (raw file):

Previously, rail (Rail Aliiev) wrote…

Hmm, newClusterOpts sounds like something for a new cluster. In other words, I was expecting to have something different for existing clusters. Maybe remove the "New" prefix? Or am I missing something?

Realized that NewClusterOpts type is just a subset of install.SyncedCluster so used that instead.