Skip to content

Check the CONTAINER_TOOL value and use it in the e2e test script#364

Closed
hdefazio wants to merge 7 commits into
llm-d:mainfrom
hdefazio:container_tool_val
Closed

Check the CONTAINER_TOOL value and use it in the e2e test script#364
hdefazio wants to merge 7 commits into
llm-d:mainfrom
hdefazio:container_tool_val

Conversation

@hdefazio
Copy link
Copy Markdown
Contributor

@hdefazio hdefazio commented Oct 1, 2025

Description

  • Add a more robust check for the check-container-tool make target to catch empty CONTAINER_TOOL values
  • Change the run_e2e.sh script to use the CONTAINER_TOOL value, defaulting to docker if it is unset

How Has This Been Tested?

$ make check-container-tool 
✅ Container tool 'podman' found.

(this is correct for my system)

$ make test-e2e
✅ Container tool 'podman' found.
==== Building Docker image ghcr.io/llm-d/llm-d-inference-scheduler:dev ====
podman build \

Other cases:
Manually set CONTAINER_TOOL := ""

$ make check-container-tool 
❌ Error: No container tool detected. Please install docker or podman.
make: *** [Makefile:297: check-container-tool] Error 1

Manually set CONTAINER_TOOL := "docker"

$ make check-container-tool 
❌ Error: 'docker' is not installed or not in your PATH.
🔧 Try: sudo apt install docker OR brew install docker
make: *** [Makefile:297: check-container-tool] Error 1

(this is correct for my system)

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Summary by CodeRabbit

  • New Features

    • Allow selection of container tool via CONTAINER_TOOL, propagated to sub-commands.
    • Added check-builder target to validate and report the selected builder.
  • Improvements

    • Enhanced container tool checks with clearer validation and guidance when the tool is missing or not in PATH.
  • Tests

    • E2E script now honors CONTAINER_TOOL (defaulting to docker), prints the chosen tool, and uses it for image operations.

Signed-off-by: Hannah DeFazio <h2defazio@gmail.com>
Signed-off-by: Hannah DeFazio <h2defazio@gmail.com>
Copy link
Copy Markdown
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

/lgtm

/cc @nirrozenbaum

@shmuelk
Copy link
Copy Markdown
Collaborator

shmuelk commented Oct 5, 2025

Please rebase this PR

@pierDipi
Copy link
Copy Markdown
Member

pierDipi commented Oct 9, 2025

/cc @shmuelk @nirrozenbaum

@hdefazio
Copy link
Copy Markdown
Contributor Author

@shmuelk @nirrozenbaum PTAL

@shmuelk
Copy link
Copy Markdown
Collaborator

shmuelk commented Oct 19, 2025

@hdefazio Please rebase your branch

@elevran elevran moved this to In review in llm-d-router Oct 19, 2025
@hdefazio
Copy link
Copy Markdown
Contributor Author

@shmuelk Done!

Copy link
Copy Markdown
Collaborator

@elevran elevran left a comment

Choose a reason for hiding this comment

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

@hdefazio - thanks! please see question regarding L300 (not a blocker for merging).

/lgtm
/approve

Comment thread Makefile
@if [ -z "$(CONTAINER_TOOL)" ]; then \
echo "❌ Error: No container tool detected. Please install docker or podman."; \
exit 1; \
elif ! command -v $(CONTAINER_TOOL) >/dev/null 2>&1; then \
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Q: Since L40 sets CONTAINER_TOOL using command -v ..., can the check in L300 ever fail?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes that's fair - this was the original check for the make target so I didn't want to remove it. Without this, the check basically just becomes "Is CONTAINER_TOOL an empty string"?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it is fine without the check on L300, since the value is set and confirmed using the same command.
Seems redundant to me but can keep or remove - both are fine.
@pierDipi - any thoughts?

@elevran elevran requested a review from pierDipi October 26, 2025 12:06
@hdefazio
Copy link
Copy Markdown
Contributor Author

closing in favor of #406

@hdefazio hdefazio closed this Oct 29, 2025
@github-project-automation github-project-automation Bot moved this from In review to Done in llm-d-router Oct 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants