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

Allow modifying log verbosity in CDI #2882

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

alromeros
Copy link
Collaborator

What this PR does / why we need it:

This PR aims to make the debugging process of CDI easier, allowing changes in log verbosity from the CDI config.

It adds a new field in the CDIConfig API to allow specifying a different log verbosity level to re-initialize all loggers. Changes in this field will restart all CDI components to initialize all loggers with the new value.

Special notes for your reviewer:

The new API looks like this:

apiVersion: cdi.kubevirt.io/v1beta1
Kind: CDI
# ...
spec:
  config:
      logVerbosity: 4

I opted for a simpler API compared to the one used in kubevirt. I think the kubevirt API is overkill for CDI, but I can always change it if preferred.

Release note:

Allow modifying log verbosity in CDI

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Aug 31, 2023
@alromeros
Copy link
Collaborator Author

@akalenyu wondering if it'd be useful to have individual verbosity levels for all components as they have in kubevirt, wdyt? At first glance seems overkill for CDI.

@akalenyu
Copy link
Collaborator

@akalenyu wondering if it'd be useful to have individual verbosity levels for all components as they have in kubevirt, wdyt? At first glance seems overkill for CDI.

Yeah, I think we can start off just with the global verbosity value, just have to make sure we're
fine for additions in the future.. @mhenriks WDYT? would a struct with .globalVerbosity be better for that?

@alromeros
Copy link
Collaborator Author

@akalenyu wondering if it'd be useful to have individual verbosity levels for all components as they have in kubevirt, wdyt? At first glance seems overkill for CDI.

Yeah, I think we can start off just with the global verbosity value, just have to make sure we're fine for additions in the future.. @mhenriks WDYT? would a struct with .globalVerbosity be better for that?

Yeah I like the idea of having a struct so it's more maintainable, Let's see what others think.

@alromeros alromeros changed the title [WIP] Allow modifying log verbosity in CDI Allow modifying log verbosity in CDI Aug 31, 2023
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2023
@mhenriks
Copy link
Member

mhenriks commented Sep 1, 2023

I think this is fine for infra components but what about worker pods?

@awels
Copy link
Member

awels commented Sep 1, 2023

The workloads get their verbosity from the controllers that create them. So the import controller creates pods and grabs the verbosity from the cdi-deployment. For example the import controller https://github.com/kubevirt/containerized-data-importer/blob/main/pkg/controller/import-controller.go#L1191 gets the verbosity from the reconciler field, which is set when the controller is started.

@mhenriks
Copy link
Member

mhenriks commented Sep 5, 2023

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2023
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2023
@awels
Copy link
Member

awels commented Sep 5, 2023

We should probably discuss if and how far we want to backport this as it could be really helpful with debugging customer issues.

@alromeros
Copy link
Collaborator Author

@mhenriks @awels are we ok with having a single verbosity value? Or do you prefer to use a struct as proposed by @akalenyu so we can add more fields in the future?
/hold

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2023
@awels
Copy link
Member

awels commented Sep 6, 2023

I am fine with a single verbosity value. I am not sure there is much value in a more granular approach.

@alromeros
Copy link
Collaborator Author

/unhold

@kubevirt-bot kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 6, 2023
This commit adds a new field in the cdiConfig API to allow specifying a log verbosity level to initialize all loggers.

Changes in this field will mean restarting all CDI components to initialize all loggers.

Signed-off-by: Alvaro Romero <alromero@redhat.com>
Signed-off-by: Alvaro Romero <alromero@redhat.com>
@kubevirt-bot kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 7, 2023
@awels
Copy link
Member

awels commented Sep 7, 2023

/test pull-containerized-data-importer-e2e-hpp-latest

@awels
Copy link
Member

awels commented Sep 8, 2023

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2023
@kubevirt-bot kubevirt-bot merged commit 030846a into kubevirt:main Sep 8, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants