Skip to content

release-21.2: roachprod: backport changes from master as of 2021-11-11#72641

Merged
craig[bot] merged 26 commits intocockroachdb:release-21.2from
RaduBerinde:roachprod-backport21.2
Nov 17, 2021
Merged

release-21.2: roachprod: backport changes from master as of 2021-11-11#72641
craig[bot] merged 26 commits intocockroachdb:release-21.2from
RaduBerinde:roachprod-backport21.2

Conversation

@RaduBerinde
Copy link
Copy Markdown
Member

This PR backports all changes involving roachprod as of 2021-11-11. There have been large refactorings which we want to backport, or it will make backporting any future necessary roachtest fixes much harder. We also want new upcoming features around multi-tenancy available for 21.2.

CC @cockroachdb/release

roachprod/vm/aws: improve help text for multiple stores

roachprod create ajwerner-test -n1 --clouds aws \
--aws-ebs-volume='{"VolumeType": "io2", "VolumeSize": 213, "Iops": 321}' \
--aws-ebs-volume='{"VolumeType": "io2", "VolumeSize": 213, "Iops": 321}' \
--aws-enable-multiple-stores=true
roachprod stage ajwerner-test cockroach
roachprod start ajwerner-test --store-count 2

The above commands will create a node with multiple stores and start cockroach
on them. Hopefully these minor help changes make that clearer.

Release note: None

roachprod: add stageurl command

Sometimes it is useful to be able to download these artifacts
directly. For example, when trying to bisect a problem. But, the URL
can take a second to remember the format of.

The stageurl command prints the staging URL of the given application.

I've reorganized some of the code to reduce duplication between the
stage and stageurl command. There is still more duplication than I
would like. But I figured I would see if this seems useful to others
before further refactoring.

Release note: None

roachprod: clean up roachprod ssh keys in aws

Many SSH keys created by roachprod are no longer used, and some were created by former employees.

This needed to change because it's a security issue that former employees may exploit.

This patch adds another step to roachprod-gc cronjob to tag any untagged keys created by roachprod in AWS and delete them if they are unused.

Release note: None

roachprod: upgrade Azure Ubuntu image to 20.04

Previously, currently used Ubuntu 18.04 doesn't support systemd-run --same-dir, which is used by some roachprod scripts. Additionally, GCE
and AWS already use Ubuntu 20.04 based images for roachprod.

Updating the base image to Ubuntu 20.04 fixes the issue above and aligns
the version with other cloud providers.

Release note: None

roachprod: update azure SDK

This is a partial backport of the commit below (only the part that
affects roachprod).

metric: Add Alert and Aggregation Rule interface

In this commit, the interfaces for Alert and Aggregation rule
interfaces are outlined. These interfaces will be used
by a new endpoint which will expose these rules in a YAML
format. This endpoint can be used by our end users to
configure alerts/monitoring for CockroachDB clusters.
This commit also updates the prometheus dependency in the
vendor submodule.

Release note: None

roachprod: fix roachprod gc docker build

Previously, the roachprod garbage collector docker image build process
was using the go get approach to build roachprod.

Currently, this method doesn't work, because it doesn't use any pinning,
so the build ends up with all kind of deprecation warnings and failures.

  • Use multi-stage docker build in order to separate build and runtime.
    It also reduces the image size from 1.9G to 700M.
  • Build roachprod using the checked out commit SHA.
  • Use the Bazel build image we use in CI to build roachprod.
  • Use Bazel to build roachprod.
  • Added cloudbuild.yaml to publish the docker image to GCR and use a
    beefier instance type.
  • Modify the entrypoint script to set the default region, required by
    the AWS Go SDK library.
  • Add push.sh to script deployment.

Release note: None

roachprod: correct spelling mistakes

Release note: None

roachprod: install AWS CLI v2 for GC images

Previously, after regenerating the GC docker images, roachprod stopped
listing AWS as an available provider, because Debian ships with AWS CLI
v1, but roachprod doesn't support it.

This patch installs AWS CLI v2.

Release note: None

roachprod: making roachprod subcommands point to a new library

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

roachprod: avoid flaky test due to unused functions

Merging #71660 trigerred a flaky test due to unused functions.

This patch avoids that test by making use of / commenting unused functions.

Release note: None

roachprod: minor cleanup for cloud.Cloud

This change fills in some missing comments from cloud.Cloud and
improves the interface a bit. Some of the related roachprod code is
cleaned up as well.

Release note: None

roachprod: clean up local cluster metadata

The logic around how the local cluster metadata is loaded and saved is
very convoluted. The local provider is using install.Clusters and is
writing directly to the .hosts/local file.

This commit disentangles this logic: it is now up to the main program
to call local.AddCluster() to inject local cluster information. The
main program also provides the implementation for a new
local.VMStorage interface, allowing the code for saving the hosts
file to live where it belongs.

Release note: None

roachprod: clean up local cluster deletion

This change moves the code to destroy the local cluster to the local
provider. The hosts file is deleted through LocalVMStorage.

Release note: None

roachprod: rework clusters cache

This commit changes roachprod from using hosts-style files in
~/.roachprod/hosts for caching clusters to using json files in
~/.roachprod/clusters.

Like before, each cluster has its own file. The main advantage is
that we can now store the entire cluster metadata instead of
manufacturing it based on one-off parsing.

WARNING: after this change, the information in ~/.roachprod/hosts
will no longer be used. If a local cluster exists, the new roachprod
version will not know about it. It is recommended to destroy any local
cluster before using the new version. A local cluster can also be
cleaned up manually using:

killall -9 cockroach
rm -rf ~/.roachprod/local

Release note: None

roachprod: use cloud.Cluster in SyncedCluster

This change stores Cluster metadata directly in SyncedCluster, instead
of making copies of various fields.

roachprod: store ports in vm.VM

This change adds SQLPort and AdminUIPort fields to vm.VM. This
allows us to remove the special hardcoded values for the local
cluster.

Having these fields stored in the clusters cache will allow having
multiple local clusters, each with their own set of ports.

Release note: None

roachprod: support multiple local clusters

This change adds support for multiple local clusters. Local cluster
names must be either "local" or of the form "local-foo".

When the cluster is named "local", the node directories stay in the
same place, e.g. ~/local/1. If the cluster is named "local-foo",
node directories are like ~/local/foo-1.

For local clusters we include the cluster name in the ROACHPROD
variable; this is necessary to distinguish between processes of
different local clusters. The relevant code is cleaned up to
centralize the logic related to the ROACHPROD variable.

Fixes #71945.

Release note: None

meh

roachprod: list VMs in parallel

This commit speeds up the slowest step of roachprod: listing VMs from
all providers. We now list the VMs in parallel across all providers
instead of doing it serially.

Release note: None

roachprod: fix behavior when mixing GCE projects

Currently roachprod has very poor behavior when used with different
projects on the same host. For example:

shell1: GCE_PROJECT=andrei-jepsen roachstress.sh ... // this will run ~forever
sometime later in shell2: roachprod sync (on the default project)

The sync on the default project removes the cached information for the
cluster on andrei-jepsen, which causes roachprod commands against
that cluster (from within the roachstress.sh script) to fail.

We fix this by ignoring any cached clusters that reference a project
that the provider was not configured for - both when loading clusters
into memory and when deleting stale cluster files during sync.

As part of the change, we also improve the output of list to remove
the colon after the cluster name and to include the GCE project:

$ roachprod list --gce-project cockroach-ephemeral,andrei-jepsen
Syncing...
Refreshing DNS entries...
glenn-anarest                  [aws]                      9  (142h41m39s)
glenn-drive                    [aws]                      1  (141h41m39s)
jane-1635868819-01-n1cpu4      [gce:cockroach-ephemeral]  1  (10h41m39s)
lin-ana                        [aws]                      9  (178h41m39s)
local-foo                      [local]                    4  (-)
radu-foo                       [gce:andrei-jepsen]        4  (12h41m39s)
radu-test                      [gce:cockroach-ephemeral]  4  (12h41m39s)

Release note: None

roachprod: don't remove LOCK file

We use a LOCK file during sync. We create the file, acquire an
exclusive lock and at the end remove the file. The removal of the file
will fail if another process was waiting for the lock. Also, there is
a race where we could be deleting the file that is in use by another
process, and that would allow a third process to create the file
again.

To fix these issues, we let the LOCK file be; there is no need to
remove it - we are relying on flock, not on exclusive file creation.

Release note: None

roachprod: fix improperly wrapped errors

Partial backport of this commit:
*: fix improperly wrapped errors

I'm working on a linter that detects errors that are not wrapped
correctly, and it discovered these.

Release note: None

roachprod: fix roachprod start ignoring --binary flag

Merging #71660 introduced a bug where roachprod ignores --binary
flag when running roachprod start.

This patch reverts to the old way of setting config.Binary.

Release note: None

Fixes #72425 #72420 #72373 #72372

roachprod: update doc on local clusters

The behavior changed in
#71970.

Release note: None

pkg/roachprod: allow multiple-stores to be created on GCP

Port an existing flag from the AWS roachprod flags that allows multiple
stores to be created. When this flag is enabled, multiple data
directories are created and mounted as /mnt/data{1..N}.

Standardize the existing ext4 disk creation logic in the GCE setup
script to match the AWS functionality. Interleave the existing ZFS setup
commands based on the --filesystem flag.

Fix a bug introduced in #54986 that will always create multiple data
disks, ignoring the value of the flag. This has the effect of never
creating a RAID 0 array, which is the intended default behavior.

The ability to create a RAID 0 array on GCE VMs is required for the
Pebble write-throughput benchmarks.

Release note: None

roachprod: move quiet determination out of the library

Moving the logic of automatically enabling Quiet in non-terminal
output.

Release note: None

roachprod: clean up use of SyncedCluster

SyncedCluster is currently used to pass the cluster name (with
optional node selector) and the settings. This is a misuse of the type
and complicates things conceptually.

This change separates out the relevant settings into a new struct
ClusterSettings. All commands now pass the cluster name and the
ClusterSettings instead of passing a SyncedCluster.

Release note: None

@RaduBerinde RaduBerinde requested a review from a team as a code owner November 11, 2021 14:23
@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Nov 11, 2021

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Member

@rail rail left a comment

Choose a reason for hiding this comment

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

TYVM!!!
LGTM, :shipit:

ajwerner and others added 22 commits November 15, 2021 09:11
```bash
roachprod create ajwerner-test -n1 --clouds aws \
--aws-ebs-volume='{"VolumeType": "io2", "VolumeSize": 213, "Iops": 321}' \
--aws-ebs-volume='{"VolumeType": "io2", "VolumeSize": 213, "Iops": 321}' \
--aws-enable-multiple-stores=true
roachprod stage ajwerner-test cockroach
roachprod start ajwerner-test --store-count 2
```

The above commands will create a node with multiple stores and start cockroach
on them. Hopefully these minor help changes make that clearer.

Release note: None
Sometimes it is useful to be able to download these artifacts
directly. For example, when trying to bisect a problem. But, the URL
can take a second to remember the format of.

The stageurl command prints the staging URL of the given application.

I've reorganized some of the code to reduce duplication between the
stage and stageurl command. There is still more duplication than I
would like. But I figured I would see if this seems useful to others
before further refactoring.

Release note: None
Many SSH keys created by roachprod are no longer used, and some were created by former employees.

This needed to change because it's a security issue that former employees may exploit.

This patch adds another step to roachprod-gc cronjob to tag any untagged keys created by roachprod in AWS and delete them if they are unused.

Release note: None
Previously, currently used Ubuntu 18.04 doesn't support `systemd-run
--same-dir`, which is used by some roachprod scripts. Additionally, GCE
and AWS already use Ubuntu 20.04 based images for roachprod.

Updating the base image to Ubuntu 20.04 fixes the issue above and aligns
the version with other cloud providers.

Release note: None
This is a partial backport of the commit below (only the part that
affects roachprod).

  metric: Add Alert and Aggregation Rule interface

  In this commit, the interfaces for Alert and Aggregation rule
  interfaces are outlined. These interfaces will be used
  by a new endpoint which will expose these rules in a YAML
  format. This endpoint can be used by our end users to
  configure alerts/monitoring for CockroachDB clusters.
  This commit also updates the prometheus dependency in the
  vendor submodule.

Release note: None
Previously, the roachprod garbage collector docker image build process
was using the `go get` approach to build roachprod.

Currently, this method doesn't work, because it doesn't use any pinning,
so the build ends up with all kind of deprecation warnings and failures.

* Use multi-stage docker build in order to separate build and runtime.
  It also reduces the image size from 1.9G to 700M.
* Build roachprod using the checked out commit SHA.
* Use the Bazel build image we use in CI to build roachprod.
* Use Bazel to build roachprod.
* Added `cloudbuild.yaml` to publish the docker image to GCR and use a
  beefier instance type.
* Modify the entrypoint script to set the default region, required by
  the AWS Go SDK library.
* Add `push.sh` to script deployment.

Release note: None
Previously, after regenerating the GC docker images, roachprod stopped
listing AWS as an available provider, because Debian ships with AWS CLI
v1, but roachprod doesn't support it.

This patch installs AWS CLI v2.

Release note: None
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
Merging cockroachdb#71660 trigerred a flaky test due to unused functions.

This patch avoids that test by making use of / commenting unused functions.

Release note: None
This change fills in some missing comments from `cloud.Cloud` and
improves the interface a bit. Some of the related roachprod code is
cleaned up as well.

Release note: None
The logic around how the local cluster metadata is loaded and saved is
very convoluted. The local provider is using `install.Clusters` and is
writing directly to the `.hosts/local` file.

This commit disentangles this logic: it is now up to the main program
to call `local.AddCluster()` to inject local cluster information. The
main program also provides the implementation for a new
`local.VMStorage` interface, allowing the code for saving the hosts
file to live where it belongs.

Release note: None
This change moves the code to destroy the local cluster to the local
provider. The hosts file is deleted through LocalVMStorage.

Release note: None
This commit changes roachprod from using `hosts`-style files in
`~/.roachprod/hosts` for caching clusters to using json files in
`~/.roachprod/clusters`.

Like before, each cluster has its own file. The main advantage is
that we can now store the entire cluster metadata instead of
manufacturing it based on one-off parsing.

WARNING: after this change, the information in `~/.roachprod/hosts`
will no longer be used. If a local cluster exists, the new `roachprod`
version will not know about it. It is recommended to destroy any local
cluster before using the new version. A local cluster can also be
cleaned up manually using:
```
killall -9 cockroach
rm -rf ~/.roachprod/local
```

Release note: None
This change stores Cluster metadata directly in SyncedCluster, instead
of making copies of various fields.
This change adds `SQLPort` and `AdminUIPort` fields to `vm.VM`. This
allows us to remove the special hardcoded values for the local
cluster.

Having these fields stored in the clusters cache will allow having
multiple local clusters, each with their own set of ports.

Release note: None
This change adds support for multiple local clusters. Local cluster
names must be either "local" or of the form "local-foo".

When the cluster is named "local", the node directories stay in the
same place, e.g. `~/local/1`. If the cluster is named "local-foo",
node directories are like `~/local/foo-1`.

For local clusters we include the cluster name in the ROACHPROD
variable; this is necessary to distinguish between processes of
different local clusters. The relevant code is cleaned up to
centralize the logic related to the ROACHPROD variable.

Fixes cockroachdb#71945.

Release note: None

meh
This commit speeds up the slowest step of roachprod: listing VMs from
all providers. We now list the VMs in parallel across all providers
instead of doing it serially.

Release note: None
Currently roachprod has very poor behavior when used with different
projects on the same host. For example:
```
shell1: GCE_PROJECT=andrei-jepsen roachstress.sh ... // this will run ~forever
sometime later in shell2: roachprod sync (on the default project)
```

The sync on the default project removes the cached information for the
cluster on `andrei-jepsen`, which causes `roachprod` commands against
that cluster (from within the `roachstress.sh` script) to fail.

We fix this by ignoring any cached clusters that reference a project
that the provider was not configured for - both when loading clusters
into memory and when deleting stale cluster files during `sync`.

As part of the change, we also improve the output of `list` to remove
the colon after the cluster name and to include the GCE project:

```
$ roachprod list --gce-project cockroach-ephemeral,andrei-jepsen
Syncing...
Refreshing DNS entries...
glenn-anarest                  [aws]                      9  (142h41m39s)
glenn-drive                    [aws]                      1  (141h41m39s)
jane-1635868819-01-n1cpu4      [gce:cockroach-ephemeral]  1  (10h41m39s)
lin-ana                        [aws]                      9  (178h41m39s)
local-foo                      [local]                    4  (-)
radu-foo                       [gce:andrei-jepsen]        4  (12h41m39s)
radu-test                      [gce:cockroach-ephemeral]  4  (12h41m39s)
```

Release note: None
We use a LOCK file during sync. We create the file, acquire an
exclusive lock and at the end remove the file. The removal of the file
will fail if another process was waiting for the lock. Also, there is
a race where we could be deleting the file that is in use by another
process, and that would allow a third process to create the file
again.

To fix these issues, we let the LOCK file be; there is no need to
remove it - we are relying on `flock`, not on exclusive file creation.

Release note: None
Partial backport of this commit:
  *: fix improperly wrapped errors

  I'm working on a linter that detects errors that are not wrapped
  correctly, and it discovered these.

Release note: None
Merging cockroachdb#71660 introduced a bug where roachprod ignores --binary
flag when running `roachprod start`.

This patch reverts to the old way of setting config.Binary.

Release note: None

Fixes cockroachdb#72425 cockroachdb#72420 cockroachdb#72373 cockroachdb#72372
tbg and others added 4 commits November 15, 2021 09:11
The behavior changed in
cockroachdb#71970.

Release note: None
Port an existing flag from the AWS roachprod flags that allows multiple
stores to be created. When this flag is enabled, multiple data
directories are created and mounted as `/mnt/data{1..N}`.

Standardize the existing ext4 disk creation logic in the GCE setup
script to match the AWS functionality. Interleave the existing ZFS setup
commands based on the `--filesystem` flag.

Fix a bug introduced in cockroachdb#54986 that will always create multiple data
disks, ignoring the value of the flag. This has the effect of never
creating a RAID 0 array, which is the intended default behavior.

The ability to create a RAID 0 array on GCE VMs is required for the
Pebble write-throughput benchmarks.

Release note: None
Moving the logic of automatically enabling Quiet in non-terminal
output.

Release note: None
`SyncedCluster` is currently used to pass the cluster name (with
optional node selector) and the settings. This is a misuse of the type
and complicates things conceptually.

This change separates out the relevant settings into a new struct
`ClusterSettings`. All commands now pass the cluster name and the
`ClusterSettings` instead of passing a `SyncedCluster`.

Release note: None
@RaduBerinde RaduBerinde force-pushed the roachprod-backport21.2 branch from e51d3f6 to 51f3015 Compare November 15, 2021 17:12
@RaduBerinde
Copy link
Copy Markdown
Member Author

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Nov 16, 2021

Build failed:

@RaduBerinde
Copy link
Copy Markdown
Member Author

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Nov 16, 2021

This PR was included in a batch that successfully built, but then failed to merge into release-21.2 (it was a non-fast-forward update). It will be automatically retried.

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Nov 17, 2021

Build succeeded:

@craig craig bot merged commit 774ea54 into cockroachdb:release-21.2 Nov 17, 2021
@RaduBerinde RaduBerinde deleted the roachprod-backport21.2 branch November 18, 2021 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants