Skip to content

[PRA-391] Add log forwarding feature - #633

Open
welpaolo wants to merge 5 commits into
mainfrom
PRA-391
Open

welpaolo wants to merge 5 commits into
mainfrom
PRA-391

Conversation

@welpaolo

@welpaolo welpaolo commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@ckfbot ckfbot added backport track/0.17 Backport to track/0.17 backport track/0.15 Backport to track/0.15 backport track/0.14 Backport to track/0.14 labels Sep 3, 2026
@ckfbot

ckfbot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

🤖 Backport labels populated

Labels to this pull request were added automatically by the populate-labels.yaml action.

When the PR is merged, backport PRs according to the labels will be automatically created. To skip the backport creation, remove any unneeded labels before merging the PR.

Copilot AI left a comment

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.

🟡 Changes recommended

Unresolved moderate findings affect relation-payload handling, image selection, and image provenance.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds Loki endpoint propagation and log-forwarding coverage for vLLM workloads.

Changes:

  • Handles Loki relation data and injects LOKI_URL.
  • Adds unit and integration observability tests.
  • Updates the default vLLM image configuration.
File summaries
File Summary Final findings
tests/integration/test_observability.py Verifies Loki forwarding end to end. Nit (2 votes): Use the KSERVE_LLMISVC_IMAGES mapping instead of a hard-coded image.
tests/integration/helpers/cos.py Adds workload environment lookup support. None.
charms/kserve-llmisvc/tests/unit/test_loki.py Tests Loki endpoint handling and rendering. None.
charms/kserve-llmisvc/src/templates/llmisvc_configs_manifests.yaml.j2 Injects LOKI_URL into workload containers. None.
charms/kserve-llmisvc/src/default-custom-images.json Updates the vLLM image entry. Moderate (1 vote): The vllm entry is not consumed by the scheduler. Moderate (1 vote): The mutable external image tag weakens provenance and reproducibility.
charms/kserve-llmisvc/src/charm.py Processes Loki relation endpoints. Moderate (3 votes): Valid JSON scalars can cause .get() to raise; only read url when the decoded payload is a dictionary.
Review details

Suppressed comments (2)

charms/kserve-llmisvc/src/default-custom-images.json:5

  • This vllm entry is not consumed by the charm: _scheduler_context still selects images_context["llm_workload"] (and config.yaml exposes only the llm_* keys). Consequently, the default LLMInferenceServiceConfig resources continue to use ghcr.io/llm-d/llm-d-cuda:v0.4.0, so changing this image does not make the default workloads use the log-forwarding image. Wire the forwarding image into the key rendered by the scheduler, or change the scheduler/template image selection accordingly.
    "vllm": "ghcr.io/welpaolo/vllm-cpu:0.19.0",

charms/kserve-llmisvc/src/default-custom-images.json:5

  • This changes the image used by the integration workload from a build-specific project image to a mutable tag in an externally controlled GHCR namespace. That weakens artifact provenance and reproducibility for CI and any consumers of this default; publish the log-forwarding image through a trusted project registry and pin an immutable digest, or keep the override local to this test.
    "vllm": "ghcr.io/welpaolo/vllm-cpu:0.19.0",
  • Files reviewed: 6/6 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +205 to +206
url = json.loads(endpoint).get("url")
except (json.JSONDecodeError, TypeError):
IMAGE_CONTEXT = {
"storage_initializer_image": KSERVE_CONTROLLER_IMAGES["configmap__storageInitializer"],
"vllm_image": KSERVE_LLMISVC_IMAGES["vllm"],
"vllm_image": "ghcr.io/welpaolo/vllm-cpu:0.19.0",
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport track/0.14 Backport to track/0.14 backport track/0.15 Backport to track/0.15 backport track/0.17 Backport to track/0.17

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants