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

allocator: safe format allocator log messages #116295

Merged
merged 8 commits into from
Dec 20, 2023

Conversation

kvoli
Copy link
Collaborator

@kvoli kvoli commented Dec 13, 2023

See individual commit messages.

Note stringer tests are added in earlier PRs in order to assert against regressions.

Resolves: #102948
Release note: None

@kvoli kvoli self-assigned this Dec 13, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@kvoli kvoli force-pushed the 231212.safe-string-allocator branch from 036fd68 to 2fd0b11 Compare December 13, 2023 17:34
@kvoli kvoli changed the title allocator: unredact allocator log messages allocator: safe format allocator log messages Dec 14, 2023
@kvoli kvoli force-pushed the 231212.safe-string-allocator branch from 2fd0b11 to e6dc7e7 Compare December 14, 2023 01:24
Introduce a string method for `storeStatus`, which previously printed
ordinals.

Add testing for both the store list and store pool string methods.

Part of: cockroachdb#102948
Release note: None
Add testing for the string output of candidates and load. These are
later used as regression tests for implementing safe formatting.

Part of: cockroachdb#102948
Release note: None
Implement `SafeValue` for `LeaseTransferOutcome` to unredact the
transfer outcome in logs. Note this is currently used only in draining
when shedding leases.

Release note: None
Part of: cockroachdb#102948
@kvoli kvoli force-pushed the 231212.safe-string-allocator branch 3 times, most recently from b1224ac to e793b32 Compare December 14, 2023 14:47
Implement `SafeFormat` for `Load` to unredact load messages in logs.

Release note: None
Part of: cockroachdb#102948
`AllocatorAction`, `TargetReplicaType` and `ReplicaStatus` were
previously hidden in redacted logs. Implement `redact.SafeValue` to
unredact. Also, implement `SafeFormatError`  for allocator errors.

Release note: None
Part of: cockroachdb#102948
`raftRangeProgress` is used to format the raft progress for a range.
Update `raftRangeProgress` to return a redactable string to show in
redacted logs.

Release note: None
Part of: cockroachdb#102948
Implement `SafeFormat` for `StorePool` and `StoreList` to unredact log
messages.

Release note: None
Part of: cockroachdb#102948
Clean up inconsistent parentheses and make range rebalance logging
consistent with lease rebalance logging.

Epic: none
Release note: None
@kvoli kvoli force-pushed the 231212.safe-string-allocator branch from e793b32 to ea43cc4 Compare December 14, 2023 14:49
@kvoli kvoli marked this pull request as ready for review December 14, 2023 21:04
@kvoli kvoli requested a review from a team as a code owner December 14, 2023 21:04
@kvoli kvoli requested a review from andrewbaptist December 14, 2023 21:04
Copy link
Collaborator

@andrewbaptist andrewbaptist left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this all up! Only once question/concern about whether constraints are considered sensitive, otherwise it all looks good.

Reviewed 4 of 4 files at r1, 4 of 4 files at r2, 2 of 2 files at r3, 5 of 5 files at r4, 4 of 4 files at r5, 2 of 2 files at r6, 5 of 5 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @kvoli)


pkg/kv/kvserver/allocator/allocatorimpl/allocator.go line 442 at r5 (raw file):

		}
		b.SafeRune('{')
		b.Print(ae.constraints[i])

I'm concerned that constraints may be considered sensitive data. We consider database and table names as sensitive data, but this is a little less clear. Who is the right person to make this call?


pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer_test.go line 1677 at r2 (raw file):

		cl = append(cl, candidate{
			store:           roachpb.StoreDescriptor{StoreID: roachpb.StoreID(i)},
			valid:           i%2 == 0,

nit: No need to change this, but it might have been easier for all these i%2 cases to instead have 12 different candidates, and each of them only change one parameter (e.g. - i%12 == 5). The advantage of that approach would be if there were "cross-wired" mappings it would be very obvious.

Copy link
Collaborator Author

@kvoli kvoli left a comment

Choose a reason for hiding this comment

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

TYFTR -- replied below re: constraints being considered sensitive.

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


pkg/kv/kvserver/allocator/allocatorimpl/allocator.go line 442 at r5 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

I'm concerned that constraints may be considered sensitive data. We consider database and table names as sensitive data, but this is a little less clear. Who is the right person to make this call?

These will be redacted -- as they do not implement a SafeFormat or SafeValue method which Print here checks for.

func (c ConstraintsConjunction) String() string {

This behavior hasn't changed, just that the parts around the constraints won't be redacted now.

@kvoli
Copy link
Collaborator Author

kvoli commented Dec 20, 2023

Should be good for another review round @andrewbaptist.

@andrewbaptist andrewbaptist self-requested a review December 20, 2023 20:26
Copy link
Collaborator

@andrewbaptist andrewbaptist left a comment

Choose a reason for hiding this comment

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

:lgtm:

@kvoli
Copy link
Collaborator Author

kvoli commented Dec 20, 2023

TYFTR

bors r=andrewbaptist

@craig craig bot merged commit 611607c into cockroachdb:master Dec 20, 2023
8 of 9 checks passed
@craig
Copy link
Contributor

craig bot commented Dec 20, 2023

Build succeeded:

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.

allocator: implement safe format for all logging
3 participants