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

Comprehensive logging improvements #535

Merged
merged 1 commit into from
Jul 27, 2021
Merged

Comprehensive logging improvements #535

merged 1 commit into from
Jul 27, 2021

Conversation

ellistarn
Copy link
Contributor

Issue, if available:

Description of changes:

  • Migrates to knative style logging
  • Logging contexts are now propagated via ctx
  • Logging configuration is now controlled via configmap
  • Logging level can be dynamically adjusted by updating the configmap.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@njtran njtran 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. This is some really nice improvements to our logging. Could you include a change to show how to adjust the logging in the configmaps, considering that's two of the changes? (forgive me if you did and I missed it)

- get
- list
- watch
- apiGroups: [""]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you changed the format on this one, but not any of the others in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's a lot easier to read when condensed like this. It's following the tekton/knative projects' example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change the rest of the file to match this format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cmd/controller/main.go Outdated Show resolved Hide resolved
pkg/apis/provisioning/v1alpha3/provisioner_defaults.go Outdated Show resolved Hide resolved
@netlify
Copy link

netlify bot commented Jul 23, 2021

✔️ Deploy Preview for karpenter-docs-prod canceled.

🔨 Explore the source changes: 2946ba0

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/60ff1b3542c0580008769174

@ellistarn ellistarn force-pushed the logging branch 10 times, most recently from 007373f to 55e7e41 Compare July 26, 2021 16:44
@ellistarn ellistarn force-pushed the logging branch 2 times, most recently from 655c2ef to c59ce1a Compare July 26, 2021 20:21
@@ -43,7 +43,6 @@ licenses: ## Verifies dependency licenses and requires GITHUB_TOKEN to be set
golicense hack/license-config.hcl karpenter

apply: ## Deploy the controller into your ~/.kube/config cluster
kubectl create ns karpenter || true
helm template karpenter charts/karpenter --namespace karpenter \
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we need --create-namespace for a fresh install to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This workflow assumes that the namespace isn't created or deleted. I think it's much safer to just assume that the developer manages this namespace since there are so many ways it could be created (helm, eksctl, etc) that have bitten us. Worst case, a new dev needs to run kubectl create namespace karpenter

Copy link
Contributor

@njtran njtran left a comment

Choose a reason for hiding this comment

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

Quite a lengthy one. Good work!

@ellistarn ellistarn merged commit cb6d8e9 into aws:main Jul 27, 2021
@ellistarn ellistarn deleted the logging branch July 27, 2021 00:31
gfcroft pushed a commit to gfcroft/karpenter-provider-aws that referenced this pull request Nov 25, 2023
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.

None yet

3 participants