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

fix: Removed spurious error logging #91

Merged
merged 1 commit into from
Nov 30, 2022
Merged

fix: Removed spurious error logging #91

merged 1 commit into from
Nov 30, 2022

Conversation

ellistarn
Copy link
Contributor

@ellistarn ellistarn commented Nov 30, 2022 •
edited
Loading

Fixes #2902

Description

In general, this PR targets noisy/confusing logging. Reclassified a few logs from info->debug, removed from spurious error logging, and lower cased a few statements.

  • Cleaned up some dead code in the batcher that we were no longer using after the controller refactor
  • Requeue after an empty batch instead of waiting forever to refresh batch size settings
  • Removed dead code from the provisioner
  • WaitForClusterSync was hanging, even when context.Done was triggered. Use controller runtime for retries and bail if not synced
  • Simplified the singletoncontroller by removing the WaitUntil factorization.

How was this change tested?

  • make test
  • make e2etests

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

@ellistarn ellistarn requested a review from a team as a code owner November 30, 2022 00:23
@ellistarn ellistarn requested a review from engedaam November 30, 2022 00:23
@ellistarn ellistarn marked this pull request as draft November 30, 2022 00:23
@ellistarn ellistarn force-pushed the 2902 branch 2 times, most recently from 35613ae to 1931c03 Compare November 30, 2022 05:10
@coveralls
Copy link

coveralls commented Nov 30, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 3587788061

  • 31 of 66 (46.97%) changed or added relevant lines in 7 files are covered.
  • 9 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.5%) to 76.169%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/operator/settingsstore/settingsstore.go 6 7 85.71%
pkg/controllers/provisioning/controller.go 5 7 71.43%
pkg/controllers/provisioning/batcher.go 6 9 66.67%
pkg/controllers/provisioning/provisioner.go 10 16 62.5%
pkg/operator/controller/singleton.go 0 23 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/provisioning/controller.go 1 68.18%
pkg/controllers/deprovisioning/helpers.go 2 82.85%
pkg/operator/controller/singleton.go 2 0%
pkg/controllers/node/controller.go 4 59.7%
Totals Coverage Status
Change from base Build 3586724668: 0.5%
Covered Lines: 5312
Relevant Lines: 6974
💛 - Coveralls

@ellistarn ellistarn marked this pull request as ready for review November 30, 2022 05:29
@ellistarn ellistarn enabled auto-merge (squash) November 30, 2022 23:01
jonathan-innis
jonathan-innis approved these changes Nov 30, 2022
View reviewed changes
Copy link
Member

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@ellistarn ellistarn merged commit 21d81d7 into kubernetes-sigs:main Nov 30, 2022
@ellistarn ellistarn deleted the 2902 branch November 30, 2022 23:12
@ellistarn ellistarn mentioned this pull request Dec 1, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathan-innis jonathan-innis jonathan-innis approved these changes

@engedaam engedaam Awaiting requested review from engedaam engedaam was automatically assigned from aws/karpenter

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Webhook Errors on Clean Install
3 participants
@ellistarn @coveralls @jonathan-innis