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: consistently set executor log options #12979

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix: consistently set executor log options #12979

wants to merge 7 commits into from

Conversation

agilgur5
Copy link
Member

@agilgur5 agilgur5 commented Apr 24, 2024 •
edited

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
fix: consistently set executor log options
de8c72b 
- 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 <agilgur5@gmail.com>
@agilgur5
return string
e004ec6 
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 added area/controller Controller issues, panics area/executor labels Apr 24, 2024
@agilgur5 agilgur5 mentioned this pull request Apr 24, 2024
Merged
@agilgur5
try conditional gloglevel?
f71f76c 
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5
empty string not nil
f146b07 
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5
lookup can return nil?
f318305 
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5
fix accidental resource action removal
2b9973f 
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5
manual formatting since gofmt didn't do anything?
f94d4b8 
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 added the area/agent Argo Agent that runs for HTTP and Plugin templates label Apr 26, 2024
tczhao
tczhao reviewed May 1, 2024
View reviewed changes
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
Member 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tczhao tczhao tczhao left review comments

@terrytangyuan terrytangyuan Awaiting requested review from terrytangyuan

@sarabala1979 sarabala1979 Awaiting requested review from sarabala1979

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/agent Argo Agent that runs for HTTP and Plugin templates area/controller Controller issues, panics area/executor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JSON logging does not work for resource template
2 participants
@agilgur5 @tczhao