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

debug: remove the CLI check for debug_enabled #10273

Merged
merged 3 commits into from
May 27, 2021
Merged

debug: remove the CLI check for debug_enabled #10273

merged 3 commits into from
May 27, 2021

Conversation

dhiaayachi
Copy link
Collaborator

@dhiaayachi dhiaayachi commented May 20, 2021 •
edited by dnephin
Loading

Fixes #10208

The API allows collecting profiles even when debug_enabled=false, as long as ACLs are enabled. This PR remove the check for debug_enabled from the CLI so that users do not need to set debug_enabled=true for no reason.

Also fixes the API client to return errors on non-200 status codes for debug endpoints and improves the failure messages when pprof data can not be collected.

The fix to the API client exposed the fact that most of these tests were actually failing silently. They were never checking the actual output, and all of these tests were hitting 500 errors on the server (see comment below for why). So this PR also fixes the tests.

@dnephin @dhiaayachi
debug: remove the CLI check for debug_enabled
8811b27 
The API allows collecting profiles even debug_enabled=false as long as
ACLs are enabled. Remove this check from the CLI so that users do not
need to set debug_enabled=true for no reason.

Also:
- fix the API client to return errors on non-200 status codes for debug
  endpoints
- improve the failure messages when pprof data can not be collected

Co-Authored-By: Dhia Ayachi <[email protected]>
@github-actions github-actions bot added theme/api Relating to the HTTP API interface theme/cli Flags and documentation for the CLI interface labels May 20, 2021
@dhiaayachi
remove parallel test runs
3b95fa8 
parallel runs create a race condition that fail the debug tests
@vercel vercel bot temporarily deployed to Preview – consul May 20, 2021 20:27 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging May 20, 2021 20:27 Inactive
@dhiaayachi
Copy link
Collaborator Author

@dnephin I think this is now ready.
The reason tests were failing is that this kind of tests cannot run in parallel is that this use (among others) the pprof go package to collect debug data and this can be activated only once within a runtime. In case of parallel testing it gets activated multiple times within the same runtime (the test runtime). the straight forward solution is to not run tests in parallel in that case.

@dnephin
Copy link
Contributor

dnephin commented May 21, 2021

Ah yes, makes sense! In general I have been removing t.Parlallel as much as possible. I think we should only need it for tests in the agent and agent/consul package because there are a large number of slow tests in those packages. Every other package should not need to use t.Parallel.

I'll add a changelog entry and then I think we should be ready for someone to review.

@vercel vercel bot temporarily deployed to Preview – consul May 21, 2021 16:12 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging May 21, 2021 16:12 Inactive
@dnephin dnephin requested a review from a team May 21, 2021 16:36
@dhiaayachi dhiaayachi merged commit 4c7f5f3 into master May 27, 2021
@dhiaayachi dhiaayachi deleted the dnephin/improve-consul-debug branch May 27, 2021 13:41
@hc-github-team-consul-core
Copy link
Collaborator

🍒 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/376360.

@hc-github-team-consul-core
Copy link
Collaborator

🍒 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/377975.

@hc-github-team-consul-core
Copy link
Collaborator

🍒✅ Cherry pick of commit 4c7f5f3 onto release/1.10.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request May 31, 2021
@dhiaayachi @dnephin @hc-github-team-consul-core
debug: remove the CLI check for debug_enabled (#10273)
db23df8 
* debug: remove the CLI check for debug_enabled

The API allows collecting profiles even debug_enabled=false as long as
ACLs are enabled. Remove this check from the CLI so that users do not
need to set debug_enabled=true for no reason.

Also:
- fix the API client to return errors on non-200 status codes for debug
  endpoints
- improve the failure messages when pprof data can not be collected

Co-Authored-By: Dhia Ayachi <[email protected]>

* remove parallel test runs

parallel runs create a race condition that fail the debug tests

* Add changelog

Co-authored-by: Daniel Nephin <[email protected]>
@hc-github-team-consul-core
Copy link
Collaborator

🍒❌ Cherry pick of commit 4c7f5f3 onto release/1.9.x failed! Build Log

@hc-github-team-consul-core
Copy link
Collaborator

🍒❌ Cherry pick of commit 4c7f5f3 onto release/1.8.x failed! Build Log

@hc-github-team-consul-core
Copy link
Collaborator

🍒❌ Cherry pick of commit 4c7f5f3 onto release/1.7.x failed! Build Log

dnephin added a commit that referenced this pull request May 31, 2021
@dhiaayachi @dnephin
debug: remove the CLI check for debug_enabled (#10273)
4249ac9 
* debug: remove the CLI check for debug_enabled

The API allows collecting profiles even debug_enabled=false as long as
ACLs are enabled. Remove this check from the CLI so that users do not
need to set debug_enabled=true for no reason.

Also:
- fix the API client to return errors on non-200 status codes for debug
  endpoints
- improve the failure messages when pprof data can not be collected

Co-Authored-By: Dhia Ayachi <[email protected]>

* remove parallel test runs

parallel runs create a race condition that fail the debug tests

* Add changelog

Co-authored-by: Daniel Nephin <[email protected]>
@dnephin dnephin mentioned this pull request May 31, 2021
Merged
dnephin added a commit that referenced this pull request May 31, 2021
@dhiaayachi @dnephin
debug: remove the CLI check for debug_enabled (#10273)
7c1d7ae 
* debug: remove the CLI check for debug_enabled

The API allows collecting profiles even debug_enabled=false as long as
ACLs are enabled. Remove this check from the CLI so that users do not
need to set debug_enabled=true for no reason.

Also:
- fix the API client to return errors on non-200 status codes for debug
  endpoints
- improve the failure messages when pprof data can not be collected

Co-Authored-By: Dhia Ayachi <[email protected]>

* remove parallel test runs

parallel runs create a race condition that fail the debug tests

* Add changelog

Co-authored-by: Daniel Nephin <[email protected]>
@dnephin dnephin mentioned this pull request May 31, 2021
Merged
@kisunji kisunji mentioned this pull request Oct 25, 2022
Merged
3 tasks
This was referenced Oct 27, 2022
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyhavlov kyhavlov kyhavlov approved these changes

Assignees
No one assigned
Labels
theme/api Relating to the HTTP API interface theme/cli Flags and documentation for the CLI interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cli: consul debug should not require enable_debug for profiling
4 participants
@dhiaayachi @dnephin @hc-github-team-consul-core @kyhavlov