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

Add support for --format flag for 'keys' #306

Merged
merged 4 commits into from
Dec 16, 2022

Conversation

ptabor
Copy link
Contributor

@ptabor ptabor commented Dec 20, 2021

This allows to print keys as 'hex', especially useful in context of etcd revisions:
./bbolt keys --format=hex ./foo/db key

Copy link
Member

@spzala spzala left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @ptabor

cmd/bbolt/main.go Show resolved Hide resolved
@ahrtr
Copy link
Member

ahrtr commented Dec 1, 2022

@ptabor could you please rebase this PR?

@ptabor
Copy link
Contributor Author

ptabor commented Dec 1, 2022

[scheduling this on my todo list]

@ptabor ptabor force-pushed the 20211220-bbolt_cmd_format branch 2 times, most recently from 2a359bc to 3da2606 Compare December 7, 2022 21:34
This allows to print keys as 'hex', especially useful in context of etcd revisions:
  ./bbolt keys --format=hex ./foo/db key

Signed-off-by: Piotr Tabor <ptab@google.com>
Signed-off-by: Piotr Tabor <ptab@google.com>
Signed-off-by: Piotr Tabor <ptab@google.com>
@ahrtr
Copy link
Member

ahrtr commented Dec 7, 2022

A quick comment: the test is broken https://github.com/etcd-io/bbolt/actions/runs/3643030691/jobs/6150802557

@ptabor
Copy link
Contributor Author

ptabor commented Dec 8, 2022

@serathius FYI

@ahrtr ahrtr self-requested a review December 8, 2022 09:13
overflowN := p.overflow
if overflowN > 100*1024 {
Copy link
Member

Choose a reason for hiding this comment

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

It means bbolt doesn't support saving object > 400MB (assuming pageSize is 4K)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addresses in the other PR: #354

db.go Outdated
const magic uint32 = 0xED0CDAED
const Magic uint32 = 0xED0CDAED
Copy link
Member

Choose a reason for hiding this comment

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

The Magic should be invisible to users/client applications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in the other PR: #354

In general we are missing an internal library that bbolt cli can use and share with the core implementation instead of having copy-paste of the data-structures and constants.

Comment on lines 1840 to 1842
if m.magic != bolt.Magic {
return 0, fmt.Errorf("the page has wrong (unexpected) magic")
}
Copy link
Member

Choose a reason for hiding this comment

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

Since the command is supposed to be executed by users. They need to use check command to validate the DB. In other words, even the db is corrupted, they should also run some command, such as dumping page items.

Suggested change
if m.magic != bolt.Magic {
return 0, fmt.Errorf("the page has wrong (unexpected) magic")
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #354.

We need safe-guards even with bbolt tool. Allocating 1TBs of memory becouse it consider random garbage from meta-page as block-size is not good and risk --> especially if diagnostic needs to be performed within production environment in order to avoid moving customer's data.
If this would become a real problem -> we would need a 'force' flag to ignore such safe-guards.

@ahrtr
Copy link
Member

ahrtr commented Dec 8, 2022

This PR mentions Add support for --format flag for 'keys', but I do not see where the flag --format is added.

@ptabor ptabor marked this pull request as draft December 9, 2022 13:27
@ptabor
Copy link
Contributor Author

ptabor commented Dec 9, 2022

Marking as draft. I used this branch to keep some changes to diagnose data corruption, and the last commit incidentally reverted the main changes. Some of the changes will go to a separate branch.

@ptabor ptabor marked this pull request as ready for review December 15, 2022 14:38
@ptabor
Copy link
Contributor Author

ptabor commented Dec 15, 2022

I brought back original scope of this request. Moving the discussion about 'Magic' to a separate PR i will follow with.

@ptabor ptabor requested a review from ahrtr December 15, 2022 21:59
if val == nil {
return ErrKeyNotFound
return fmt.Errorf("Error %w for key: %q hex: \"%x\"", ErrKeyNotFound, key, string(key))
Copy link
Member

Choose a reason for hiding this comment

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

Suggest to output the key in exactly the same format as inputted by user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to provide very explicit feedback. In particular usually we (etcd) want to pass \x00\x00... starting strings... and char-0 does not passes well through linux's vexec command so the strings are truncated.

Both format's allow to easily cross check against bbolt page db 5

$ echo $'aaa\x00aaa' | hexdump 

0000000 6161 0a61                              
0000004

var parseFormat string
var format string
fs.StringVar(&parseFormat, "parse-format", "ascii-encoded", "Output format. One of: ascii-encoded|hex")
fs.StringVar(&format, "format", "bytes", "Output format. One of: ascii-encoded|hex|bytes (default: bytes)")
Copy link
Member

Choose a reason for hiding this comment

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

Please also update (cmd *GetCommand) Usage()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Signed-off-by: Piotr Tabor <ptab@google.com>
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

@ptabor ptabor merged commit 1d5a2b0 into etcd-io:master Dec 16, 2022
@ahrtr ahrtr added this to the 1.3.7 milestone Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants