Skip to content

feat(helm): Adds log_level support for storage, scheduler and worker. - #422

Merged
sitaowang1998 merged 3 commits into
y-scope:mainfrom
sitaowang1998:helm-log-level
Jul 30, 2026
Merged

feat(helm): Adds log_level support for storage, scheduler and worker.#422
sitaowang1998 merged 3 commits into
y-scope:mainfrom
sitaowang1998:helm-log-level

Conversation

@sitaowang1998

@sitaowang1998 sitaowang1998 commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Description

This PR adds support for setting log level in storage, scheduler and worker services by:

  • Adding log_level field in values.yaml for all three services, default to INFO.
  • Adding RUST_LOG in env for all three services using the corresponding log_level.
  • Bumping chart version to satisfy chart linting workflow.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • Run e2e test on test branch and observer that:
    • RUST_LOG set to INFO for all three pods by default.
    • Info level logs are available using kubectl logs.
  • GitHub workflows pass.

Summary by CodeRabbit

  • New Features

    • Added configurable log levels for scheduler, storage, and execution manager services.
    • Added default INFO log-level settings to the deployment configuration.
    • Preserved support for additional worker environment variables.
  • Chores

    • Updated the deployment chart version.

@sitaowang1998
sitaowang1998 requested a review from a team as a code owner July 30, 2026 16:10
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Helm defaults now define log levels for execution-manager, scheduler, and storage containers. Deployment templates expose those values through RUST_LOG, worker extra environment rendering is retained within its environment block, and the chart version is bumped.

Changes

Helm logging configuration

Layer / File(s) Summary
Logging value defaults
tools/deployment/spider-helm/values.yaml
Adds INFO defaults for execution-manager, scheduler, and storage log levels.
Deployment logging wiring
tools/deployment/spider-helm/templates/*-deployment.yaml, tools/deployment/spider-helm/Chart.yaml
Injects configured log levels as RUST_LOG, keeps worker extra environment values in the environment block, and updates the chart version.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • y-scope/spider#415: Updates the worker Helm environment construction involving RUST_LOG and extra_envs.
  • y-scope/spider#411: Modifies the execution-manager worker deployment configuration and values wiring.
  • y-scope/spider#384: Updates storage deployment environment configuration in the same Helm chart.

Suggested reviewers: junhaoliao

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adding log_level support for storage, scheduler, and worker in the Helm chart.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment on lines +26 to +28
- name: "RUST_LOG"
value: {{ .Values.spiderConfig.execution_manager.log_level | quote }}
{{- with .Values.spiderConfig.worker.extra_envs }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

env and extra_evns?

Let's discuss, I think I am okay with the change in this PR, but might worth some refactoring for the extra_envs, extra_volumn_mounts and extra_volumn

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We reached the following conclusion during offline discussion:

  • Storage and scheduler should not accept extra environment variables for safety.
  • Worker should have a standalone log_level instead of using extra_envs to:
    • Keep the same form as with storage and scheduler.
    • Avoid user accidentally overwriting the log level when adding new environment variables.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with the decision to split the log level to the default env.

@LinZhihao-723 LinZhihao-723 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Before merge: I think you should make the scope consistent with other PRs. Looks like we were always using feat(helm), not feat(spider-helm).

@sitaowang1998 sitaowang1998 changed the title feat(spider-helm): Adds log_level support for storage, scheduler and worker. feat(helm): Adds log_level support for storage, scheduler and worker. Jul 30, 2026
@sitaowang1998
sitaowang1998 merged commit 5b70e24 into y-scope:main Jul 30, 2026
15 checks passed
@sitaowang1998
sitaowang1998 deleted the helm-log-level branch July 30, 2026 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants