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

feat(backend): Enable logging for KFP components #10288

Merged
merged 1 commit into from
Jan 5, 2024
Merged

feat(backend): Enable logging for KFP components #10288

merged 1 commit into from
Jan 5, 2024

Conversation

DharmitD
Copy link
Contributor

@DharmitD DharmitD commented Dec 5, 2023

Description of your changes:
Resolves #10105

Adding logging functionality for KFP components so we could enable setting logging levels through manifest parameters and inject through environment variables in all pods.

Testing instructions:

  • Build images for each of the three components using updated dockerfiles: APIServer, ScheduledWorkflow and PersistenceAgent
  • Update the kustomize manifests to add an environment variable that could be used to specify log levels in APIServer, ScheduledWorkflow and PersistenceAgent deployments. For instance:
 name: LOG_LEVEL
 value: info
  • Deploy KFP using these manifests and replace the APISever, persistence agent and scheduled workflow images using the newly created images.

Checklist:

Copy link

Hi @DharmitD. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@DharmitD DharmitD changed the title Enable logging for KFP components feat(backend): Enable logging for KFP components Dec 5, 2023
backend/Dockerfile Outdated Show resolved Hide resolved
@Tomcli
Copy link
Member

Tomcli commented Dec 5, 2023

/ok-to-test

@DharmitD
Copy link
Contributor Author

DharmitD commented Dec 6, 2023

/retest

backend/src/agent/persistence/main.go Outdated Show resolved Hide resolved
backend/Dockerfile Outdated Show resolved Hide resolved
@DharmitD
Copy link
Contributor Author

/retest

@Tomcli
Copy link
Member

Tomcli commented Dec 12, 2023

@DharmitD in my local environment I see your code is failing because there aren't any default log level value in the deployment. I think you would need to define a default log level value for the e2e test to pass.

time="2023-12-12T17:57:21Z" level=info msg="Location: UTC"
flag provided but not defined: -log-level
Usage of /bin/controller:
  -alsologtostderr
    	log to standard error as well as files
  -clientBurst int
    	Maximum burst for throttle from this client. (default 10)
  -clientQPS float
    	The maximum QPS to the master from this client. (default 5)
  -kubeconfig string
    	Path to a kubeconfig. Only required if out-of-cluster.
  -log_backtrace_at value
    	when logging hits line file:N, emit a stack trace
  -log_dir string
    	If non-empty, write log files in this directory
  -logtostderr
    	log to standard error instead of files
  -master string
    	The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.
  -namespace string
    	The namespace name used for Kubernetes informers to obtain the listers.
  -stderrthreshold value
    	logs at or above this threshold go to stderr
  -v value
    	log level for V logs
  -vmodule value
    	comma-separated list of pattern=N settings for file-filtered logging

@DharmitD
Copy link
Contributor Author

DharmitD commented Dec 13, 2023

@DharmitD in my local environment I see your code is failing because there aren't any default log level value in the deployment. I think you would need to define a default log level value for the e2e test to pass.

time="2023-12-12T17:57:21Z" level=info msg="Location: UTC"
flag provided but not defined: -log-level
Usage of /bin/controller:
  -alsologtostderr
    	log to standard error as well as files
  -clientBurst int
    	Maximum burst for throttle from this client. (default 10)
  -clientQPS float
    	The maximum QPS to the master from this client. (default 5)
  -kubeconfig string
    	Path to a kubeconfig. Only required if out-of-cluster.
  -log_backtrace_at value
    	when logging hits line file:N, emit a stack trace
  -log_dir string
    	If non-empty, write log files in this directory
  -logtostderr
    	log to standard error instead of files
  -master string
    	The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.
  -namespace string
    	The namespace name used for Kubernetes informers to obtain the listers.
  -stderrthreshold value
    	logs at or above this threshold go to stderr
  -v value
    	log level for V logs
  -vmodule value
    	comma-separated list of pattern=N settings for file-filtered logging

@Tomcli Do you mean directly providing the default value in the deployment manifests instead of $(LOG_LEVEL)? Or, is there any .env file where you generally specify default values / environment variables that the manifests could utilize?

@DharmitD
Copy link
Contributor Author

/retest

backend/Dockerfile Outdated Show resolved Hide resolved
backend/Dockerfile.persistenceagent Outdated Show resolved Hide resolved
backend/Dockerfile.scheduledworkflow Outdated Show resolved Hide resolved
Copy link
Member

@Tomcli Tomcli left a comment

Choose a reason for hiding this comment

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

thanks @DharmitD
/lgtm

Copy link
Member

@chensun chensun left a comment

Choose a reason for hiding this comment

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

/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

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

@google-oss-prow google-oss-prow bot merged commit 5399585 into kubeflow:master Jan 5, 2024
6 checks passed
@DharmitD
Copy link
Contributor Author

DharmitD commented Jan 5, 2024

Thank you so much @Tomcli @chensun !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Improve Logging in the KFP components
3 participants