make workloadmanager build more explicit#264
make workloadmanager build more explicit#264hzxuzhonghu wants to merge 4 commits intovolcano-sh:mainfrom
Conversation
Starting in Kubernetes v1.36, the DRAResourceClaimGranularStatusAuthorization feature gate (beta, on-by-default) enforces fine-grained authorization checks for ResourceClaim status updates. Schedulers that modify status.allocation or status.reservedFor now require additional permissions on the resourceclaims/binding synthetic subresource. This adds the required RBAC rule for the agent scheduler. These permissions are inert on Kubernetes versions prior to v1.36. Ref: kubernetes/kubernetes#138149 Signed-off-by: Cairon <cairon-ab@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR updates build/dev tooling so the WorkloadManager image build is called out explicitly, and removes local “run” Makefile targets (per the stated port-conflict concern), while aligning E2E and developer docs with the new workflow.
Changes:
- Rename the WorkloadManager image build/push targets to
docker-*-workloadmanagerand update E2E script/docs to use them. - Simplify Kind image loading via
make kind-load(now loads workloadmanager/router/picod) and removekind-load-router. - Extend Volcano agent-scheduler (development) RBAC to allow updates/patches to
resourceclaims/binding.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/run_e2e.sh | Use make docker-build-workloadmanager during E2E image build step. |
| test/e2e/README.md | Document updated E2E image build command set. |
| manifests/charts/base/templates/volcano-agent-scheduler-development.yaml | Add RBAC rule for resourceclaims/binding updates/patches. |
| Makefile | Rename WorkloadManager docker build/push targets; remove local run/install targets; make kind-load load all three images. |
| docs/agentcube/docs/developer-guide/local-development.md | Update local dev instructions to the new image build/load commands and remove local “run” section. |
| docker/Dockerfile.workloadmanager | No functional change (appears to be a no-op rewrite). |
| ### Build Specific Components | ||
|
|
||
| - **Workload Manager**: `make build` | ||
| - **Workload Manager**: `make build-workloadmanager` |
There was a problem hiding this comment.
make build-workloadmanager is referenced here, but the Makefile only defines build (which builds the workloadmanager). Either update this doc to reference make build, or add a build-workloadmanager target/alias in the Makefile to match the documentation and the PR goal of making the build more explicit.
| - **Workload Manager**: `make build-workloadmanager` | |
| - **Workload Manager**: `make build` |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #264 +/- ##
==========================================
+ Coverage 35.60% 43.32% +7.71%
==========================================
Files 29 30 +1
Lines 2533 2613 +80
==========================================
+ Hits 902 1132 +230
+ Misses 1505 1358 -147
+ Partials 126 123 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Makefile to streamline build and Docker image management, including renaming docker-build targets for clarity and consolidating kind-load into a single command for all images. It also updates the volcano-agent-scheduler role with additional permissions for resourceclaims/binding. The documentation and e2e tests have been updated to reflect these Makefile changes. Feedback suggests re-introducing multi-architecture build targets without push for local development and adding a docker-build-all target to simplify building all Docker images.
| docker-buildx: | ||
| @echo "Building multi-architecture Docker image..." | ||
| docker buildx build -f docker/Dockerfile --platform linux/amd64,linux/arm64 -t $(WORKLOAD_MANAGER_IMAGE) . | ||
| docker build -f docker/Dockerfile.workloadmanager -t $(WORKLOAD_MANAGER_IMAGE) . |
There was a problem hiding this comment.
The docker-buildx, docker-buildx-router, and docker-buildx-picod targets for building multi-architecture images without pushing have been removed. This removes potentially useful functionality for developers who want to build and test multi-arch images locally without pushing to a registry. Please consider re-adding these targets with the new naming convention, for example:
# Multi-architecture build (supports amd64, arm64)
docker-buildx-workloadmanager:
@echo "Building multi-architecture Docker image..."
docker buildx build -f docker/Dockerfile.workloadmanager --platform linux/amd64,linux/arm64 -t $(WORKLOAD_MANAGER_IMAGE) .Similar targets could be added for router and picod.
| # Picod Docker targets | ||
| docker-build-picod: | ||
| @echo "Building Picod Docker image..." | ||
| docker build -f docker/Dockerfile.picod -t $(PICOD_IMAGE) . |
There was a problem hiding this comment.
To improve developer experience, consider adding a target to build all docker images at once, similar to build-all. This would complement the change I've suggested in docs/agentcube/docs/developer-guide/local-development.md. You could add this after the docker-build-picod target:
docker-build-all: docker-build-workloadmanager docker-build-router docker-build-picod ## Build all docker images| make docker-build-workloadmanager | ||
| make docker-build-router | ||
| make docker-build-picod |
There was a problem hiding this comment.
It would be more convenient for users to build all Docker images with a single command. I've made a suggestion on the Makefile to add a docker-build-all target. If that is implemented, this documentation can be simplified.
| make docker-build-workloadmanager | |
| make docker-build-router | |
| make docker-build-picod | |
| make docker-build-all |
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
Co-authored-by: Zhou Zihang <mail@zzhgo.com> Signed-off-by: Tiger Xu / Zhonghu Xu <xuzhonghu@huawei.com>
Co-authored-by: Zhou Zihang <mail@zzhgo.com> Signed-off-by: Tiger Xu / Zhonghu Xu <xuzhonghu@huawei.com>
| go run ./cmd/router/main.go \ | ||
| --port=8080 \ | ||
| --debug | ||
| build-all: build-workloadmanager build-agentd build-router |
There was a problem hiding this comment.
build-all still depends on the removed build target, so make build-all (and default make all) will fail. Update this dependency to build-workloadmanager (or reintroduce an alias target) to keep builds working.
| @@ -68,7 +69,7 @@ gen-check: gen-all ## Check if generated code is up to date | |||
| .PHONY: build run clean test deps | |||
There was a problem hiding this comment.
The .PHONY declaration still lists build and run, but those targets no longer exist. This can be confusing for maintainers and makes it harder to reason about intended targets; update the .PHONY list to match the current target set.
| .PHONY: build run clean test deps | |
| .PHONY: clean test deps |
| # Multi-architecture build and push | ||
| docker-buildx-push: | ||
| docker-buildx-push-workloadmanager: | ||
| @if [ -z "$(IMAGE_REGISTRY)" ]; then \ | ||
| echo "Error: IMAGE_REGISTRY not set. Usage: make docker-buildx-push IMAGE_REGISTRY=your-registry.com"; \ | ||
| echo "Error: IMAGE_REGISTRY not set. Usage: make docker-buildx-push-workloadmanager IMAGE_REGISTRY=your-registry.com"; \ | ||
| exit 1; \ | ||
| fi | ||
| @echo "Building and pushing multi-architecture Docker image to $(IMAGE_REGISTRY)/$(WORKLOAD_MANAGER_IMAGE)..." | ||
| docker buildx build -f docker/Dockerfile --platform linux/amd64,linux/arm64 \ | ||
| docker buildx build -f docker/Dockerfile.workloadmanager --platform linux/amd64,linux/arm64 \ | ||
| -t $(IMAGE_REGISTRY)/$(WORKLOAD_MANAGER_IMAGE) \ | ||
| --push . |
There was a problem hiding this comment.
The workload manager multi-arch release target was renamed to docker-buildx-push-workloadmanager, but existing automation still invokes make docker-buildx-push (e.g., .github/workflows/build-push-release.yml). Without updating those callers (or keeping a backward-compatible alias), release image publishing will break.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: