-
Notifications
You must be signed in to change notification settings - Fork 105
add clusterip, nodeport in k8s deployment #335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
add clusterip, nodeport in k8s deployment #335
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for different Kubernetes service types (LoadBalancer, ClusterIP, NodePort) to enable more flexible deployment options for K8s environments. The implementation allows users to choose the appropriate service type based on their deployment scenario - production with external access, internal cluster communication only, or development environments.
Key Changes:
- Extended K8s deployment to support ClusterIP and NodePort service types in addition to the existing LoadBalancer
- Added
--service-typeCLI option with validation using click.Choice - Updated configuration files and documentation with service type examples and access instructions
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/agentscope_runtime/engine/deployers/kubernetes_deployer.py |
Added service type extraction from runtime_config and conditional URL construction logic for different service types |
src/agentscope_runtime/common/container_clients/kubernetes_client.py |
Renamed _create_loadbalancer_service to _create_service with service_type parameter; added helper methods for ClusterIP and NodePort IP retrieval |
src/agentscope_runtime/cli/commands/deploy.py |
Added --service-type CLI option and service-type-specific access instructions in deployment output |
examples/deployments/k8s_deploy/k8s_deploy_config.yaml |
Added service_type configuration with explanatory comments |
examples/deployments/k8s_deploy/k8s_deploy_config.json |
Added service_type field with LoadBalancer default |
examples/deployments/k8s_deploy/app_deploy_to_k8s.py |
Added commented example for service_type configuration |
examples/deployments/k8s_deploy/README.md |
Added comprehensive documentation for service type options and usage examples |
cookbook/zh/cli.md |
Added Chinese documentation for --service-type option with examples |
cookbook/en/cli.md |
Added English documentation for --service-type option with examples |
cookbook/zh/advanced_deployment.md |
Added Chinese service type configuration guide with access examples |
cookbook/en/advanced_deployment.md |
Added English service type configuration guide with access examples |
.gitignore |
Added .qwen and QWEN.md to ignored files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _get_node_external_ip(self): | ||
| """Get external IP of any available node.""" | ||
| try: | ||
| nodes = self.v1.list_node() | ||
| for node in nodes.items: | ||
| for address in node.status.addresses: | ||
| if address.type == "ExternalIP": | ||
| return address.address | ||
| # Fallback to InternalIP if no ExternalIP | ||
| for node in nodes.items: | ||
| for address in node.status.addresses: | ||
| if address.type == "InternalIP": | ||
| return address.address | ||
| return "127.0.0.1" | ||
| except Exception as e: | ||
| logger.error(f"Failed to get node IP: {e}") | ||
| return "127.0.0.1" | ||
|
|
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _get_node_external_ip method is defined but never used in the codebase. For NodePort services, the code returns a special format "nodeport:{port}" instead of using the node IP. Consider either using this method to provide a complete URL with the node IP for NodePort services, or removing it if it's not needed.
| def _get_node_external_ip(self): | |
| """Get external IP of any available node.""" | |
| try: | |
| nodes = self.v1.list_node() | |
| for node in nodes.items: | |
| for address in node.status.addresses: | |
| if address.type == "ExternalIP": | |
| return address.address | |
| # Fallback to InternalIP if no ExternalIP | |
| for node in nodes.items: | |
| for address in node.status.addresses: | |
| if address.type == "InternalIP": | |
| return address.address | |
| return "127.0.0.1" | |
| except Exception as e: | |
| logger.error(f"Failed to get node IP: {e}") | |
| return "127.0.0.1" |
| # - LoadBalancer: External access via cloud provider (default) | ||
| # - ClusterIP: Internal cluster access only, requires port-forward | ||
| # - NodePort: Access via node IP and allocated port | ||
| # service_type="ClusterIP", # Uncomment for cluster-internal access |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment shows a commented-out example with the key name service_type but this key should be nested inside runtime_config based on the actual implementation. The comment should be updated to show the correct usage as a key within the runtime_config dictionary, like:
runtime_config = {
"service_type": "ClusterIP", # Uncomment for cluster-internal access
}or demonstrate setting it directly in the deployment call.
| # service_type="ClusterIP", # Uncomment for cluster-internal access | |
| # "service_type": "ClusterIP", # Uncomment for cluster-internal access |
| def _get_node_port(self, service_name): | ||
| """Get allocated NodePort from service.""" | ||
| try: | ||
| service = self.v1.read_namespaced_service( | ||
| name=service_name, | ||
| namespace=self.namespace, | ||
| ) | ||
| # Return first port's nodePort | ||
| if service.spec.ports and len(service.spec.ports) > 0: | ||
| return service.spec.ports[0].node_port | ||
| return None |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _get_node_port method only returns the first port's node_port when there might be multiple ports configured. This could be problematic for deployments with multiple exposed ports. Consider returning all node ports or documenting this limitation clearly. Alternatively, the method could return a list of all node ports or accept a parameter to specify which port to retrieve.
| def _get_node_port(self, service_name): | |
| """Get allocated NodePort from service.""" | |
| try: | |
| service = self.v1.read_namespaced_service( | |
| name=service_name, | |
| namespace=self.namespace, | |
| ) | |
| # Return first port's nodePort | |
| if service.spec.ports and len(service.spec.ports) > 0: | |
| return service.spec.ports[0].node_port | |
| return None | |
| def _get_node_port(self, service_name, port_index: int = 0): | |
| """ | |
| Get allocated NodePort from service. | |
| By default, this returns the NodePort of the first port defined on the | |
| Service. For Services exposing multiple ports, a specific port can be | |
| selected by providing its index via the ``port_index`` argument. | |
| """ | |
| try: | |
| service = self.v1.read_namespaced_service( | |
| name=service_name, | |
| namespace=self.namespace, | |
| ) | |
| ports = getattr(service.spec, "ports", None) | |
| if not ports: | |
| return None | |
| if port_index < 0 or port_index >= len(ports): | |
| logger.error( | |
| "Requested port_index %s out of range for service %s " | |
| "(available ports: %s)", | |
| port_index, | |
| service_name, | |
| len(ports), | |
| ) | |
| return None | |
| return ports[port_index].node_port |
|
|
||
| - **`k8s_namespace`**: Kubernetes namespace where resources will be deployed | ||
| - Creates the namespace if it doesn't exist | ||
| - **`kubeconfig_path`**: Path to kubeconfig file (None uses default kubectl config at your local require kubectrl running) |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: "kubectrl" should be "kubectl".
| - **`kubeconfig_path`**: Path to kubeconfig file (None uses default kubectl config at your local require kubectrl running) | |
| - **`kubeconfig_path`**: Path to kubeconfig file (None uses default kubectl config at your local require kubectl running) |
Description
Related Issue: Fixes #[issue_number] or Relates to #[issue_number]
#332
Security Considerations: [If applicable, especially for sandbox changes]
Type of Change
Component(s) Affected
Checklist
Testing
[How to test these changes]
Additional Notes
[Optional: any other context]