-
Notifications
You must be signed in to change notification settings - Fork 2k
Add nomad monitor export command #26178
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
Conversation
3a0e003
to
4a608b0
Compare
b105697
to
e430e42
Compare
68fdc38
to
2ddf97c
Compare
2ddf97c
to
2c0148a
Compare
2c0148a
to
8863e01
Compare
8863e01
to
71a05f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you're not quite done @tehut but I made a first pass over this.
5e7423f
to
18ed5c1
Compare
18ed5c1
to
ec7eb62
Compare
8e0b286
to
a3dc825
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good @tehut! I've also checked this out locally and ran it against a multinode cluster to check things like goroutine leaks, truncated reads, etc. and it looks great!
I've left a little pile of comments and questions but most of it is small stuff at this point.
71769f4
to
1799a42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I've left a small number of suggestions and one last-triple-check question, but once those are resolved we can squash this and merge it!
// Context passed from client to close the cmd and exit the function | ||
Context context.Context | ||
|
||
bufSize int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also unused, looks like. The callers are in other packages, so they can't see this field anyways. But we're not using it in test either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super committed to keeping this in but I wanted to try it as an exported field in light of the point you raised yesterday about the StreamFramer and StreamReader also having a configurable window size. I like how it's currently configured where the producer is pushing 1/2 as much data as each consumer is ready to process but it seems like a dial folks might want to use down the road? Or does that just introduce another thing to keep track of/test/validate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me. 👍
This whole project really points to a need to revisit some of the plumbing for how uni-directional and bi-directional streaming is designed, as it was super painful to add a new endpoint. So having configuration knobs handy will be useful later if we ever find the time to set aside for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
The nomad monitor export command introduces the ability for nomad to export logs a given agent has written to journald or to the nomad log file. Journald logs can be requested for a specific period of time while we just return the agent's entire nomad log file. Introducing this RPC is a prerequisite for adding journald logs to the nomad support bundle.
Pasting a link to a comment deep in the thread regarding fixes for inconsistencies related to ending the stream.
#26178 (comment)
Testing & Reproduction steps
Links
Contributor Checklist
changelog entry using the
make cl
command.ensure regressions will be caught.
and job configuration, please update the Nomad website documentation to reflect this. Refer to
the website README for docs guidelines. Please also consider whether the
change requires notes within the upgrade guide.
Pulled this from the Contributor checklist for new cli commands, I
struck outa few items that didn't seem to fit this use case but I'm happy to revisit them if I was wrong there.CLI Command Checklist
prior art we should match? Arguments, flags, env vars, etc?
command/
or in an existing file if a subcommandcommand/
packagecommand/commands.go
command/agent/<command>_endpoint.go
command/agent/http.go
nomad/<command>_endpoint.go
client/<command>_endpoint.go
(For client endpoints like Filesystem only)nomad/structs/
package Request and Response structs (cstructs, but still)api/
package helper methodsI added a
monitorHelper
function to more cleanly share the existing monitor code in /api/agent.go. I did not add an additional test for the helper or the MonitorExport command. Mostly because I'd have to update the Client Config structs to be able to set theLogFile
value on the api Client. But the only difference between the Monitor & MonitorExport commands inapi/agent.go
is which path is passed to themonitorHelper
and helper is exercised in the existing monitor tests.* [ ] For nested commands make sure all intermediary subcommands exist (forexample,
nomad acl
,nomad acl policy
, andnomad acl policy apply
mustall be valid commands)
* [ ] If the command has astatus
subcommand consider adding a search contextin
nomad/search_endpoint.go
and updatecommand/status.go
* [ ] Implement-json
(returns raw API response)* [ ] Implement-t
(format API response using gotemplate)* [ ] Implement-verbose
(expands truncated UUIDs, adds other detail)* [ ] Implement and test newapi/
package Request and Response structsDocs
Reviewer Checklist
backporting document.
in the majority of situations. The main exceptions are long-lived feature branches or merges where
history should be preserved.
within the public repository.