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

DRAFT: Reset log levels per pod #1864

Merged
merged 1 commit into from
Feb 14, 2023
Merged

Conversation

jm96441n
Copy link
Member

@jm96441n jm96441n commented Jan 31, 2023

Changes proposed in this PR:

How I've tested this PR:

  • Unit tests
  • ran consul on k8's locally using kind https://gist.github.com/jm96441n/a56cd2ebd53f4484c3b40e725e305372
    • create a cluster using kind: kind create cluster
    • install consul on that cluster using the consul-values.yml file in the above gist: consul-k8s install -config-file=consul-values.yaml -set global.image=hashicorp/consul:1.14.3
    • once that is up apply the server.yaml to your cluster kubectl apply server.yaml
    • once that is setup get one of the server pod names using kubectl get pods
    • in another terminal window set up port forwarding for that pod kubectl port-forward <POD_NAME> 19000:19000 (where 19000 is the default admin port for the envoy proxy)
    • in the original terminal run consul-k8s proxy log <POD_NAME> -l warning and then consul-k8s proxy log <POD_NAME> -level grpc:error,kafka:critical,testing:off,admin:debug to set all loggers to warning, and then modify a few others to different levels
    • then run consul-k8s proxy log <POD_NAME> -r to reset the levels and observe that all are set to info

How I expect reviewers to test this PR:

  • code review
  • can run the above steps to recreate

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@jm96441n jm96441n force-pushed the reset-all-loggers-in-pod-to-default branch from c5d48da to 759f50e Compare February 2, 2023 02:50
@jm96441n jm96441n force-pushed the reset-all-loggers-in-pod-to-default branch from 759f50e to 87f9a38 Compare February 6, 2023 14:29
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -26,6 +26,8 @@ IMPROVEMENTS:
* Control-Plane
* Add support for the annotation `consul.hashicorp.com/use-proxy-health-check`. When this annotation is used by a service, it configures a readiness endpoint on Consul Dataplane and queries it instead of the proxy's inbound port which forwards requests to the application. [[GH-1824](https://github.com/hashicorp/consul-k8s/pull/1824)], [[GH-1841](https://github.com/hashicorp/consul-k8s/pull/1841)]
* Add health check for synced services based on the status of the Kubernetes readiness probe on synced pod. [[GH-1821](https://github.com/hashicorp/consul-k8s/pull/1821)]
* CLI:
* Add `consul-k8s proxy log podname` command for displaying and mofidying Envoy log levels for a given Pod. [GH-1844](https://github.com/hashicorp/consul-k8s/pull/1844), [GH-1849](https://github.com/hashicorp/consul-k8s/pull/1849), [GH-1864](https://github.com/hashicorp/consul-k8s/pull/1864)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great!

Copy link
Contributor

Choose a reason for hiding this comment

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

If these all merge into a single feature branch, I would just include the one feature branch that targets main instead of all 3. Just for tidiness.

Copy link
Member Author

Choose a reason for hiding this comment

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

so each of these PR's is targeting main, right now this PR is targeting the set-envoy... branch because it's dependent on some of the work in there

Copy link
Contributor

@curtbushko curtbushko Feb 14, 2023

Choose a reason for hiding this comment

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

You have a spelling mistake 'mofidying' 😃

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch! lemme fix that

Base automatically changed from set-envoy-log-level-per-pod to main February 13, 2023 20:10
@jm96441n jm96441n force-pushed the reset-all-loggers-in-pod-to-default branch from 1c6d116 to cfb3d58 Compare February 13, 2023 21:59
@jm96441n jm96441n marked this pull request as ready for review February 13, 2023 22:15
@t-eckert t-eckert self-requested a review February 14, 2023 17:12
Copy link
Contributor

@t-eckert t-eckert left a comment

Choose a reason for hiding this comment

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

Great work!

@jm96441n jm96441n force-pushed the reset-all-loggers-in-pod-to-default branch from a4c81fc to 5b14be4 Compare February 14, 2023 17:51
Move format parsing into envoy package

Move enovy to common package, move param parsing to calling package

Added changelog for proxy log command

Cleaning up usage message and error message from pr review

clean up issue with exported constant used in test, fmt'd/linted

export loggers

undo autoformatting

last fix to changelog to clean up from auto format

set level on reset for envoy log level to info

Fix spelling mistake in changelog
@jm96441n jm96441n force-pushed the reset-all-loggers-in-pod-to-default branch from 5b14be4 to 3dc73a6 Compare February 14, 2023 17:53
@jm96441n jm96441n merged commit 767f242 into main Feb 14, 2023
@jm96441n jm96441n deleted the reset-all-loggers-in-pod-to-default branch February 14, 2023 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants