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.

Sign up for GitHub

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

Jump to bottom

cilium statedb dump command & bugtool #26256

Merged
merged 5 commits into from Jun 22, 2023
Merged

cilium statedb dump command & bugtool #26256

merged 5 commits into from Jun 22, 2023

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Jun 15, 2023 •
edited

As requested by :manager::

Adds the API handler /statedb/dump and implements "cilium statedb dump"
on top to dump the contents of StateDB as JSON.

Extends sysdump to call "cilium statedb dump" to include the output in sysdump.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 15, 2023
@joamaki joamaki added the release-note/misc This PR makes changes that have no direct user impact. label Jun 16, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 16, 2023
@joamaki joamaki force-pushed the pr/joamaki/statedb-sysdump branch from cb1eb83 to a9e5a49 Compare June 16, 2023 13:44
bimmlerd
bimmlerd approved these changes Jun 16, 2023
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

yay, this'll make :manager: happy

daemon/cmd/statedb.go Show resolved Hide resolved
@joamaki
statedb: Improve StateDB JSON output
c6b9b18 
Make the JSON output easier for humans to read.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki
api/v1: Add statedb dump endpoint
166bbbf 
To support simple dumping of the whole statedb contents for sysdump
purposes this adds a simple /statedb/dump API that serializes the
whole database as JSON.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki
daemon: Implement statedb dump handler
74a96a4 
Once cilium#25000 is merged
this handler can be moved into pkg/statedb.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki force-pushed the pr/joamaki/statedb-sysdump branch from a9e5a49 to c1620cb Compare June 16, 2023 14:06
@joamaki
cilium: Add statedb dump command
e57d3f1 
Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki
bugtool: Dump StateDB contents
eb3c987 
Dump the contents of StateDB as JSON in bugtool for
inspecting the internal cilium-agent state.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki requested a review from aanm June 16, 2023 15:21
@joamaki joamaki force-pushed the pr/joamaki/statedb-sysdump branch from c1620cb to eb3c987 Compare June 16, 2023 16:26
@joamaki
Copy link
Contributor Author

joamaki commented Jun 16, 2023

/test

@joamaki joamaki marked this pull request as ready for review June 16, 2023 16:44
@joamaki joamaki requested review from a team as code owners June 16, 2023 16:44
@joamaki joamaki changed the title [DRAFT] Cilium statedb dump command cilium statedb dump command & bugtool Jun 16, 2023
@bimmlerd bimmlerd added kind/enhancement This would improve or streamline existing functionality. area/bugtool Impacts gathering of data for debugging purposes. area/cli Impacts the command line interface of any command in the repository. sig/agent Cilium agent related. labels Jun 19, 2023
qmonnet
qmonnet approved these changes Jun 19, 2023
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good

joestringer
joestringer approved these changes Jun 20, 2023
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Looks pretty trivially correct and will help with debugging the l2 responder / announcer feature in 1.14.

@joestringer joestringer added the release-blocker/1.14 This issue will prevent the release of the next version of Cilium. label Jun 20, 2023
bimmlerd
bimmlerd approved these changes Jun 21, 2023
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

foundations changes lgtm

@bimmlerd
Copy link
Member

@aanm is on PTO for a bit, api hence covered mostly by Nate - changing the review request to him.

@bimmlerd bimmlerd requested review from nathanjsweet and removed request for aanm June 21, 2023 06:46
pippolo84
pippolo84 approved these changes Jun 21, 2023
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

derailed
derailed reviewed Jun 21, 2023
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@joamaki Nice work! LGTM

Might be no opt here so feel free to disregard!
Not sure about the volumes at play but we're intermixing writeByte, writeString.
If memory serves the later is a bit slower and given we're looping, figured I'll flag.
Given our sysdump perf issues on some clusters might be worth seeing if there is a diet plan and put this on the bench?

pkg/statedb/db.go
@@ -64,22 +64,25 @@ func (db *stateDB) WriteJSON(w io.Writer) error {
if err != nil {
return err
}
buf.WriteString("\"" + table + "\": [\n")
buf.WriteString(" \"" + table + "\": [\n")
obj := iter.Next()
Copy link
Contributor

Choose a reason for hiding this comment

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

Might make sense to roll this initializer in the for loop below.

@bimmlerd
Copy link
Member

intermixing writeByte, writeString. If memory serves the later is a bit slower and given we're looping, figured I'll flag.

I'm not sure I follow your comment with writeByte vs writeString - writing the runes of the string individually seems slower, and casting to []byte{} would potentially break runes apart? What's your suggestion instead?

In any case, we're writing to a buffered writer, afaik the writes should amortize to be comparably fast to a raw memcpy; the volume of data is not expected to be huge at this time, since there's just one statedb table.

Given our sysdump perf issues on some clusters might be worth seeing if there is a diet plan and put this on the bench?

I agree that performance hasn't been the foremost concern of sysdumps. I'm reading your comment as a suggestion to put the collection of the statedb information behind a flag? I'm not opposed in general, but so far we've just slurped as much information as possible into a sysdump -- changing that will imo confuse debugging efforts of people unaware, due to missing information. If we want to do this, I'd advocate for doing it in some coordinated fashion over all of the sysdump information. I'd rather not have to go back and forth with requesting sysdumps with different configurations to get to the information needed to debug an issue, and usually I don't know which component is to blame before looking at a sysdump...

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 22, 2023
@derailed
Copy link
Contributor

@bimmlerd Thank you! I was not advocating introducing a flag, just a general comment on sysdump efficiency and benchmarking to optimize outputs.

derailed
derailed approved these changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

LGTM

@joestringer joestringer merged commit 1bddc87 into cilium:main Jun 22, 2023
61 of 62 checks passed
@joestringer joestringer mentioned this pull request Jun 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derailed derailed derailed approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@joestringer joestringer joestringer approved these changes

@pippolo84 pippolo84 pippolo84 approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@qmonnet qmonnet qmonnet approved these changes

Assignees
No one assigned
Labels
area/bugtool Impacts gathering of data for debugging purposes. area/cli Impacts the command line interface of any command in the repository. kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-note/misc This PR makes changes that have no direct user impact. sig/agent Cilium agent related.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@joamaki @bimmlerd @derailed @nathanjsweet @joestringer @pippolo84 @qmonnet