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

[controller] Merge cmd subpackage into main #80

Merged
merged 11 commits into from
Sep 10, 2024

Conversation

astef
Copy link
Contributor

@astef astef commented Sep 4, 2024

Description

Scope of the change is sds-local-volume-scheduler-extender. My goal is refactoring, not changing the behavior, but a few edge case scenarios has been fixed.

  • simplify project structure by merging cmd subpackage into main
  • extend sds-local-volume-scheduler-extender/pkg/logger with options to remove repetetive [subMain] logger name prefix
    • this refactoring was reverted, it will be proposed separately
  • use one-line error checks (if err := foo(); err != nil {) where possible
  • add error logs, where they were missed
  • fix error handling code (sometimes we were not exiting after logging the error)
  • replace os.Exit(1) with errors, since former doesn't execute defer (may lead to accidental resource leakage in future)
  • configure cobra's SilenceErrors = true to avoid printing error twice (once by us and another one by cobra)
  • refactor graceful shutdown:
    • configure OS signal listener earlier - in main, in order to react to quick interruptions
    • support SIGINT signal, for shutdown to work from terminal
  • specify WriteTimeout for http.Server to avoid indefinite wait for slow clients during shutdown
  • provide BaseContext to Runnables managed by the Manager

Why do we need it, and what problem does it solve?

Code becomes cleaner, and a few minor edge-case problems has been fixed.

What is the expected result?

Behavior should not change

Checklist

  • Changes were tested in the Kubernetes cluster manually.

@astef astef self-assigned this Sep 4, 2024
@astef astef marked this pull request as draft September 4, 2024 13:15
@astef astef added the enhancement New feature or request label Sep 5, 2024
Signed-off-by: Alexandr Stefurishin <[email protected]>
Signed-off-by: Alexandr Stefurishin <[email protected]>
Signed-off-by: Alexandr Stefurishin <[email protected]>
Signed-off-by: Alexandr Stefurishin <[email protected]>
Signed-off-by: Alexandr Stefurishin <[email protected]>
@astef astef force-pushed the refactor-scheduler-extender-cmd branch from 99f25b4 to 7a681b5 Compare September 5, 2024 08:37
Signed-off-by: Alexandr Stefurishin <[email protected]>
@astef astef marked this pull request as ready for review September 5, 2024 14:18
@astef astef changed the title merge cmd subpackage into main [controller] Merge cmd subpackage into main Sep 5, 2024
Signed-off-by: Alexandr Stefurishin <[email protected]>
Signed-off-by: Alexandr Stefurishin <[email protected]>
@astef astef force-pushed the refactor-scheduler-extender-cmd branch from ead916f to e289a27 Compare September 10, 2024 08:37
Signed-off-by: Alexandr Stefurishin <[email protected]>
Signed-off-by: Alexandr Stefurishin <[email protected]>
This reverts commit 6967d2a.

Revert "fix trivy"

This reverts commit 5a4ece3.

Signed-off-by: Alexandr Stefurishin <[email protected]>
@AleksZimin AleksZimin merged commit a9d5158 into main Sep 10, 2024
7 of 8 checks passed
@AleksZimin AleksZimin deleted the refactor-scheduler-extender-cmd branch September 10, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants