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

fix: consistently set executor log options #12979

Merged
merged 7 commits into from
Nov 11, 2024

Conversation

agilgur5
Copy link

@agilgur5 agilgur5 commented Apr 24, 2024

Fixes #12788, Fixes #12929 (comment)

Motivation

  • loglevel, gloglevel, and log format were inconsistently set around the codebase for executor containers
    • meaning that some had partial settings and some had no settings, depending on the command run etc

Modifications

  • for all commands, including emissary, agent, resource, and data

    • set loglevel, gloglevel, and log format
  • replace getExecutorLogLevel helper with getExecutorLogOpts helper

  • add a helper in util/cmd/glog.go to get the gloglevel

Verification

Existing tests pass

@agilgur5 agilgur5 added area/controller Controller issues, panics area/executor labels Apr 24, 2024
@agilgur5 agilgur5 added the area/agent Argo Agent that runs for HTTP and Plugin templates label Apr 26, 2024
Copy link
Member

@tczhao tczhao left a comment

Choose a reason for hiding this comment

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

Shall we also comment in the CLIs
that gloglevel is k8s go-client logging level range 0-10

@agilgur5
Copy link
Author

agilgur5 commented May 1, 2024

You mean making this line more specific?

I didn't change the option in this PR, just made it actually used, but could improve that for sure, especially as it is for the klog fork

@agilgur5
Copy link
Author

Merged main for new checks per #13027

Anton Gilgur added 7 commits October 28, 2024 16:48
- for all commands, including `emissary`, `agent`, `resource`, and `data`
  - set loglevel, gloglevel, and log format

- replace `getExecutorLogLevel` helper with `getExecutorLogOpts` helper
  - also add a helper in `/util/cmd` to get the gloglevel

Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
@agilgur5
Copy link
Author

agilgur5 commented Oct 28, 2024

Rebased and fixed a good chunk of merge conflicts since this has been waiting for review 6+ months now 😕

@tczhao @terrytangyuan could one of you review this? The PR has 3 upvotes now too

You mean making this line more specific?

I didn't change the option in this PR, just made it actually used, but could improve that for sure, especially as it is for the klog fork

I can also do this in a separate PR, it's a bit orthogonal to this fix

@agilgur5 agilgur5 requested a review from tczhao October 28, 2024 20:55
@agilgur5 agilgur5 added the prioritized-review For members of the Sustainability Effort label Oct 28, 2024
@tczhao tczhao merged commit 217b598 into argoproj:main Nov 11, 2024
28 checks passed
@agilgur5 agilgur5 deleted the fix-exec-log-opts branch November 11, 2024 03:20
Joibel pushed a commit that referenced this pull request Nov 22, 2024
Signed-off-by: Anton Gilgur <[email protected]>
(cherry picked from commit 217b598)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/agent Argo Agent that runs for HTTP and Plugin templates area/controller Controller issues, panics area/executor prioritized-review For members of the Sustainability Effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON logging does not work for resource template
2 participants