Skip to content

testcluster: minor logging improvements#56346

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
andreimatei:small.testcluster-quiesce-log
Nov 13, 2020
Merged

testcluster: minor logging improvements#56346
craig[bot] merged 1 commit intocockroachdb:masterfrom
andreimatei:small.testcluster-quiesce-log

Conversation

@andreimatei
Copy link
Copy Markdown
Contributor

Log when TestCluster quiescing starts, and add a node log tag to each
node's quiescing ctx so messages from different nodes can be
disambiguated.

Release note: None

@andreimatei andreimatei requested a review from tbg November 5, 2020 23:54
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

defer wg.Done()
if s != nil {
s.Quiesce(context.TODO())
ctx := logtags.AddTag(ctx, "n", i)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's no n0. Isn't there a tc.Servers[i].NodeID() you can use?

// Create a closer that will stop the individual server stoppers when the
// cluster stopper is stopped.
tc.stopper.AddCloser(stop.CloserFn(tc.stopServers))
tc.stopper.AddCloser(stop.CloserFn(func() { tc.stopServers(context.TODO()) }))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this will never become anything but TODO, so might as well .Background() it

@andreimatei andreimatei force-pushed the small.testcluster-quiesce-log branch from 29f8a9f to 6cf73a0 Compare November 6, 2020 19:55
Copy link
Copy Markdown
Contributor Author

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @tbg)


pkg/testutils/testcluster/testcluster.go, line 101 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

There's no n0. Isn't there a tc.Servers[i].NodeID() you can use?

done


pkg/testutils/testcluster/testcluster.go, line 288 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

this will never become anything but TODO, so might as well .Background() it

well closer functions should well take contexts. I'll keep the hope alive.

Log when TestCluster quiescing starts, and add a node log tag to each
node's quiescing ctx so messages from different nodes can be
disambiguated.

Release note: None
@andreimatei andreimatei force-pushed the small.testcluster-quiesce-log branch from 6cf73a0 to 8949b6e Compare November 12, 2020 16:48
Copy link
Copy Markdown
Contributor Author

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

craig bot pushed a commit that referenced this pull request Nov 12, 2020
55325: ui: Transactions link between Sessions and Statements r=dhartunian a=elkmaster

Resolves: #55131, #56244

Release note (admin ui change): Link to the Transactions page is now shown
between the Sessions and Statements links in the left hand navigation. This more
clearly reflects the hierarchy between the 3 concepts.

56346: testcluster: minor logging improvements r=andreimatei a=andreimatei

Log when TestCluster quiescing starts, and add a node log tag to each
node's quiescing ctx so messages from different nodes can be
disambiguated.

Release note: None

56437: cli, ui: dismiss release notes signup banner per environment variable r=knz,dhartunian a=nkodali

Previously, the signup banner could only be dismissed manually.
For internal testing purposes, this banner is unnecessary. This
change provides a way to dismiss the signup banner upon start of
a cluster via the cli by setting the environment variable
COCKROACH_UI_RELEASE_NOTES_SIGNUP_DISMISSED=true.

Resolves #46998

Release note: none

56533: backupccl: add feature flag support for BACKUP, RESTORE r=otan a=angelapwen

Follow-up to the RFC at #55778. This addresses the SRE use case mentioned in #51643 — instead of moving forward with a global denylist as the RFC indicated, we are prototyping feature flags via cluster settings to turn on/off requested features. The first part of the prototype will be done on `BACKUP` and `RESTORE` commands.

See [this doc](https://docs.google.com/document/d/1nZSdcK7YprL0P4TAuseY-mvlYnd82IaJ_ptAQDoWB6o/edit?) for further details. 

Note that the logic test under `ccl/backupccl/testdata/backup-restore/feature-flags` can be tested with the command `make test PKG=./pkg/ccl/backupccl TESTS='TestBackupRestoreDataDriven'`

— Commit message below — 

Adds a cluster setting to toggle a feature flag for the BACKUP and
RESTORE commands off and on; as well as a broad category for
Bulk IO commands. Currently disabling the cluster setting for Bulk
IO will only disable BACKUP and RESTORE jobs, but other types may
be included in this category in the future..

The feature is being introduced to address a Cockroach Cloud SRE
use case: needing  to disable certain categories of features in
case of cluster failure.

Release note (enterprise change): Adds cluster settings to enable/
disable the BACKUP and RESTORE commands. If a user attempts to use
these features while they are disabled, an error indicating that
the database administrator has disabled the feature is surfaced.

Example usage for the database administrator:
SET CLUSTER SETTING feature.bulkio.backup.enabled = FALSE;
SET CLUSTER SETTING feature.bulkio.backup.enabled = TRUE;
SET CLUSTER SETTING feature.bulkio.restore.enabled = FALSE;
SET CLUSTER SETTING feature.bulkio.restore.enabled = TRUE;
SET CLUSTER SETTING feature.bulkio.enabled = FALSE;
SET CLUSTER SETTING feature.bulkio.enabled = TRUE;

56591: ui: fix Overview screen in OSS builds r=dhartunian a=davepacheco

Previously, when using OSS builds (created with `make buildoss`), when
you loading the DB Console in your browser, you'd get "Page Not Found".
The route for the overview page was missing the leading '/'.  This bug
appears to have been introduced in
722c932.

Release note (admin ui change): This fixes a bug where users of
the OSS builds of CockroachDB would see "Page Not Found" when loading
the Console.

56600: roachpb: remove SetInner in favor of MustSetInner r=nvanbenschoten a=tbg

As of a recent commit, `ErrorDetail.SetInner` became unused, and
we can switch to a `MustSetInner` pattern for `ErrorDetail`. Since
the codegen involved is shared with {Request,Response}Union, those
lose the `SetInner` setter as well; we were always asserting on
the returned bool there anyway so this isn't changing anything.

Release note: None



Co-authored-by: Vlad Los <carrott9@gmail.com>
Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
Co-authored-by: Namrata Kodali <namrata@cockroachlabs.com>
Co-authored-by: angelapwen <angelaw@cockroachlabs.com>
Co-authored-by: Joshua M. Clulow <josh@sysmgr.org>
Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Nov 12, 2020

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Nov 13, 2020

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Nov 13, 2020

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Nov 13, 2020

Build succeeded:

@craig craig bot merged commit 2d34be7 into cockroachdb:master Nov 13, 2020
@andreimatei andreimatei deleted the small.testcluster-quiesce-log branch January 21, 2022 17:59
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.

3 participants