debug: rename cluster.json -> members.json and fix handling of Interrupt Signal #10804
Conversation
github-actions
bot
added
pr/dependencies
| @@ -450,8 +442,7 @@ func (c *cmd) captureGoRoutines(timestampDir string) error { | |||
| return fmt.Errorf("failed to collect goroutine profile: %w", err) | |||
| } | |||
|
|
|||
| err = ioutil.WriteFile(fmt.Sprintf("%s/goroutine.prof", timestampDir), gr, 0644) | |||
| return err | |||
| return ioutil.WriteFile(fmt.Sprintf("%s/goroutine.prof", timestampDir), gr, 0644) | |||
There was a problem hiding this comment.
I fix these to use filepath.Join in a follow up commit.
| if resp.StatusCode != 200 { | ||
| return nil, generateUnexpectedResponseCodeError(resp) | ||
| } | ||
| return resp.Body, nil |
There was a problem hiding this comment.
What's the use case for returning the resp.Body raw vs ioutil.ReadAll-ing and returning []byte?
There was a problem hiding this comment.
Good question! That is indeed a change from the previous methods.
The nice thing about returning an io.ReadCloser is that we don't have to buffer the entire response in memory. We can write the response to a file as a stream, so the memory of the CLI process should stay relatively constant.
With ioutil.ReadAll we have to read the entire profile into the CLI process memory, which could be quite large. I don't know if that's actually a probably in this case, but in general using an io.Reader or io.ReadCloser will give the caller the option to either stream or buffer, where as returning a []byte forces them to use the fully buffered response.
There was a problem hiding this comment.
Thank you for those improvements @dnephin. LGTM!!
I have a minor comment about the fact that the file and the subcommand have different names now and the possibility to rename the subcommand as well but feel free to merge it as is and save that for a future PR.
| fs.WithFile("agent.json", "", fs.MatchAnyFileContent), | ||
| fs.WithFile("host.json", "", fs.MatchAnyFileContent), | ||
| fs.WithFile("index.json", "", fs.MatchAnyFileContent), | ||
| fs.WithFile("members.json", "", fs.MatchAnyFileContent), |
There was a problem hiding this comment.
The target name is cluster, I think we should rename both for a better user experience. I'm not sure though if renaming a debug subcommand is considered a breaking change.
Also, I believe we have a reference to it as a cluster target in the documentation
There was a problem hiding this comment.
Good question! I believe I have handled that in the second commit, specifically here: 58af70c#diff-e4c25813d9467f20b9374e5dd16394ccf77aaf1db8fee3d1c7bba52b716b3cdeR551-R552
The way it should work now is that:
- both
clusterandmembersshould be accepted values for the-captureflag, and they should both do the same thing - the help text should say
membersnow
So I believe this handles backwards compatibility and ensures the flag values match the filename.
But good call on the docs! I missed an update to https://www.consul.io/commands/debug#capture-targets
I'll add a commit to this PR to make that change.
The API is called members. Using the same name as the API should help describe the contents of the file.
Use gotest.tools/v3/fs to make better assertions about the files Remove the TestAgent from TestDebugCommand_Prepare_ValidateTiming, since we can test that validation without making any API calls.
Some previous changes broke interrupting the debug on SigInterupt. This change restores the original behaviour by passing a context to requests. Since a new API client function was required to pass the context, I had it also return an io.ReadCloser, so that output can be streamed to files instead of fully buffering in process memory.
the cluster target was renamed to members.
dnephin
force-pushed
the
dnephin/debug-filenames
branch
from
22bbd7c to
26ef0df
Compare
|
test failures are known flakes, and I confirmed they pass locally. |
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/430041. |
Related to #10320
Best viewed by individual commit.
Renames
cluster.jsontomembers.jsonto be more consistent with the API and internal method names we use for this data.While working on this I noticed that handling of SIGINT was broken due to other changes we made for #10320. I fixed that problem by using a context, and changing the signal handler to cancel the context instead of only closing a channel.