Workloadmanager Improvements#68
Conversation
Summary of ChangesHello @acsoto, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the project by renaming its core component from Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 accomplishes a significant renaming of the project from agentcube-apiserver to workloadmanager and introduces several enhancements, including a new sandbox initialization flow using JWT and more flexible image pulling configurations. My review focuses on ensuring the renaming is applied consistently across the codebase, especially for Kubernetes resource names and labels that were missed. I've also provided suggestions to improve code correctness and maintainability, such as using unbiased random number generation, replacing hardcoded values with constants, and improving logging for better debuggability. Overall, the changes are well-structured and significantly improve the project's clarity and functionality.
There was a problem hiding this comment.
Pull request overview
This pull request performs a comprehensive renaming of the project from agentcube-apiserver to workloadmanager while introducing JWT-based sandbox initialization functionality and new container image configuration options. The changes span build scripts, deployment manifests, Docker configurations, and core application code.
Key Changes:
- Complete project renaming from
agentcube-apiservertoworkloadmanageracross all binaries, images, manifests, and documentation - New JWT-based authentication system for sandbox initialization with RSA key pair generation and storage in Kubernetes secrets
- Enhanced CodeInterpreter sandbox configuration with
ImagePullPolicyandImagePullSecretsfields for better image management
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| Dockerfile | Updated binary name from agentcube-apiserver to workloadmanager |
| Makefile | Renamed build targets and image names to use workloadmanager |
| k8s/workloadmanager.yaml | Renamed all Kubernetes resources (ServiceAccount, ClusterRole, Deployment, Service) to use workloadmanager |
| cmd/workload-manager/main.go | Updated startup log message to reference workloadmanager |
| test/e2e/run_e2e.sh | Updated test script to use new workloadmanager naming |
| example/README.md | Updated documentation to reference workloadmanager instead of agentcube-apiserver |
| example/pcap-analyzer/deployment.yaml | Updated service URL to use workloadmanager service name |
| images/sandbox/README.md | Updated documentation references to workloadmanager |
| pkg/workloadmanager/utils.go | Added RandString utility for generating random strings (used in sandbox naming) |
| pkg/workloadmanager/utils_test.go | Added basic tests for RandString function |
| pkg/workloadmanager/jwt.go | New JWT manager for generating tokens and managing RSA key pairs |
| pkg/workloadmanager/sandbox_init.go | New sandbox initialization logic with JWT-authenticated HTTP requests |
| pkg/workloadmanager/server.go | Integrated JWT manager and added enableAuth field for optional authentication |
| pkg/workloadmanager/auth.go | Added conditional authentication check based on enableAuth flag |
| pkg/workloadmanager/handlers.go | Enhanced sandbox creation with initialization flow and conditional auth logic |
| pkg/workloadmanager/workload_builder.go | Fixed SandboxClaim TypeMeta, added sessionID labels, updated sandbox naming, injected JWT public key volume |
| pkg/workloadmanager/k8s_client.go | Enhanced pod IP lookup with pod name parameter, added JWT secret storage |
| pkg/workloadmanager/garbage_collection.go | Added SandboxClaim cleanup support and improved logging |
| pkg/workloadmanager/codeinterpreter_controller.go | Added ImagePullPolicy and ImagePullSecrets support with JWT volume mounting |
| pkg/redis/client.go | Removed unused bidirectional mapping methods, added loadSandboxesByIDs helper |
| pkg/redis/client_test.go | Removed tests for deleted session lock functionality |
| pkg/common/types/sandbox.go | Added optional PublicKey field to CreateSandboxRequest |
| pkg/apis/runtime/v1alpha1/codeinterpreter_types.go | Added ImagePullPolicy and ImagePullSecrets fields to template spec |
| go.mod | Added github.com/golang-jwt/jwt/v5 dependency and promoted k8s.io/utils to direct dependency |
| go.sum | Updated checksums for new JWT dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 27 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
please update |
tjucoder
force-pushed
the
workload-mgr
branch
from
16297b0 to
98a5cfe
Compare
tjucoder
force-pushed
the
workload-mgr
branch
from
98a5cfe to
1c41bc6
Compare
rebase and solve conflicts |
Signed-off-by: Zhou Zihang <z@mcac.cc> Signed-off-by: tjucoder <chinesecoder@foxmail.com>
Signed-off-by: tjucoder <chinesecoder@foxmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: tjucoder <chinesecoder@foxmail.com>
Signed-off-by: tjucoder <chinesecoder@foxmail.com>
Signed-off-by: tjucoder <chinesecoder@foxmail.com>
Signed-off-by: Zhou Zihang <z@mcac.cc>
tjucoder
force-pushed
the
workload-mgr
branch
from
3d00607 to
9145290
Compare
Signed-off-by: tjucoder <chinesecoder@foxmail.com>
Signed-off-by: tjucoder <chinesecoder@foxmail.com>
Signed-off-by: tjucoder <chinesecoder@foxmail.com>
Signed-off-by: tjucoder <chinesecoder@foxmail.com>
hzxuzhonghu
left a comment
hzxuzhonghu
left a comment
There was a problem hiding this comment.
StoreJWTPublicKeyInSecret is still not handled correctly, when you create secret, but it returns already exist error
pkg/workloadmanager/handlers.go
Outdated
| if externalInfo.NeedInitialization == true { | ||
| // Code Interpreter sandbox created, init code interpreter | ||
| // Find the /init endpoint from entryPoints | ||
| var initEndpoint string | ||
| for _, access := range storeCacheInfo.EntryPoints { | ||
| if access.Path == "/init" { | ||
| initEndpoint = fmt.Sprintf("%s://%s", access.Protocol, access.Endpoint) | ||
| break | ||
| } | ||
| } | ||
|
|
||
| // If no /init path found, use the first entryPoint endpoint fallback | ||
| if initEndpoint == "" { | ||
| initEndpoint = fmt.Sprintf("%s://%s", storeCacheInfo.EntryPoints[0].Protocol, | ||
| storeCacheInfo.EntryPoints[0].Endpoint) | ||
| } |
There was a problem hiding this comment.
Feel this is too hacky, it requires that user must specify /init entry point. I think we can make /init as default.
| // NeedInitialization specifies if CodeInterpreter need initialization | ||
| // default true if NeedInitialization is nil | ||
| // +optional | ||
| NeedInitialization *bool `json:"needInitialization,omitempty"` |
There was a problem hiding this comment.
This field is limited to work only with picod, maybe at first we can remove this, and do initialization as we do now.
We can extend the init configuration later if needed, it will need not only a bool.
Whatever picod should be a must now.
pkg/common/types/sandbox.go
Outdated
| ) | ||
|
|
||
| type SandboxRedis struct { | ||
| type SandboxStore struct { |
There was a problem hiding this comment.
SandboxStore looks like a class, while here we want to express a object
Maybe call SandboxInfo
Signed-off-by: tjucoder <chinesecoder@foxmail.com>
Signed-off-by: tjucoder <chinesecoder@foxmail.com>
Signed-off-by: tjucoder <chinesecoder@foxmail.com>
|
Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages. The list of commits with invalid commit messages:
DetailsInstructions 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. I understand the commands that are listed here. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: hzxuzhonghu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This pull request primarily renames the project and all related components from
agentcube-apiservertoworkloadmanager, ensuring consistent naming across the codebase, documentation, Docker, Kubernetes manifests, and build scripts. Additionally, it introduces new configuration options for sandbox image pulling and updates dependencies.Project-wide renaming and configuration updates:
Major rename of binaries, scripts, and manifests:
agentcube-apiservertoworkloadmanagerfor clarity and consistency. This includes updates in theDockerfile,Makefile, deployment YAMLs, README files, and log messages. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20]Sandbox image configuration enhancements:
ImagePullPolicyandImagePullSecretsfields toCodeInterpreterSandboxTemplateto allow more flexible and secure configuration of how container images are pulled for sandboxes.Dependency updates:
github.com/golang-jwt/jwt/v5and updatedk8s.io/utilsdependency ingo.modfor improved JWT handling and Kubernetes utilities. [1] [2] [3]API and Redis client interface changes:
PublicKeyfield toCreateSandboxRequestfor improved session creation security.