Copilot commented on code in PR #85:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/85#discussion_r2565563718
##########
cloudstack_instances.go:
##########
@@ -201,10 +207,22 @@ func (cs *CSCloud) InstanceMetadata(ctx context.Context,
node *corev1.Node) (*cl
}
return &cloudprovider.InstanceMetadata{
- ProviderID: cs.ProviderName(),
+ ProviderID: cs.getProviderIDFromInstanceID(instanceID),
InstanceType: instanceType,
NodeAddresses: addresses,
Zone: cs.zone,
Region: zone.Region,
}, nil
}
+
+func (cs *CSCloud) getProviderIDFromInstanceID(instanceID string) string {
+ return fmt.Sprintf("%s://%s", cs.ProviderName(), instanceID)
+}
+
+func (cs *CSCloud) getInstanceIDFromProviderID(providerID string) string {
+ parts := strings.Split(providerID, "://")
+ if len(parts) == 1 {
+ return providerID
+ }
+ return parts[1]
Review Comment:
The `getInstanceIDFromProviderID` function could produce incorrect results
if the providerID contains multiple "://" separators. The current
implementation using `strings.Split` would create more than 2 parts, and
`parts[1]` would only return the segment between the first and second "://"
rather than everything after the first one. Use `strings.SplitN(providerID,
"://", 2)` instead to split only on the first occurrence of "://".
##########
cloudstack.go:
##########
@@ -238,3 +246,54 @@ func (cs *CSCloud) GetZoneByNodeName(ctx context.Context,
nodeName types.NodeNam
return zone, nil
}
+
+// getNodeNameFromPod gets the node name where this pod is running by querying
the Kubernetes API.
+// It uses the pod's name and namespace (from environment variables or
hostname) to look up the pod
+// and retrieve its spec.nodeName field.
+func (cs *CSCloud) getNodeNameFromPod(ctx context.Context) (string, error) {
+ if cs.clientBuilder == nil {
+ return "", fmt.Errorf("clientBuilder not initialized, cannot
query Kubernetes API")
+ }
+
+ client, err := cs.clientBuilder.Client("cloud-controller-manager")
+ if err != nil {
+ return "", fmt.Errorf("failed to get Kubernetes client: %v",
err)
+ }
+
+ // Get pod name and namespace
+ // In Kubernetes, the pod name is available as HOSTNAME environment
variable
+ // or we can use os.Hostname() which returns the pod name
+ podName := os.Getenv("HOSTNAME")
+ if podName == "" {
+ var err error
+ podName, err = os.Hostname()
+ if err != nil {
+ return "", fmt.Errorf("failed to get pod name: %v", err)
+ }
+ }
+
+ // Get namespace from environment variable or default to kube-system
for CCM
+ namespace := os.Getenv("POD_NAMESPACE")
+ if namespace == "" {
+ // Try reading from service account namespace file (available
in pods)
+ if data, err :=
os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err ==
nil {
+ namespace = string(data)
+ } else {
+ // Default namespace for cloud controller manager
+ namespace = "kube-system"
+ }
+ }
+
+ // Get the pod object from Kubernetes API
+ pod, err := client.CoreV1().Pods(namespace).Get(ctx, podName,
metav1.GetOptions{})
+ if err != nil {
+ return "", fmt.Errorf("failed to get pod %s/%s from Kubernetes
API: %v", namespace, podName, err)
+ }
+
+ if pod.Spec.NodeName == "" {
+ return "", fmt.Errorf("pod %s/%s does not have a nodeName
assigned yet", namespace, podName)
+ }
+
+ klog.V(4).Infof("found node name %s for pod %s/%s", pod.Spec.NodeName,
namespace, podName)
+ return pod.Spec.NodeName, nil
+}
Review Comment:
The new `getNodeNameFromPod` function, which queries the Kubernetes API to
get pod information, lacks test coverage. Since other parts of this codebase
have test coverage (see `cloudstack_test.go`), this new functionality should
also include tests to verify correct behavior in various scenarios (e.g.,
missing HOSTNAME env var, missing namespace file, pod not scheduled yet).
##########
cloudstack.go:
##########
@@ -238,3 +246,54 @@ func (cs *CSCloud) GetZoneByNodeName(ctx context.Context,
nodeName types.NodeNam
return zone, nil
}
+
+// getNodeNameFromPod gets the node name where this pod is running by querying
the Kubernetes API.
+// It uses the pod's name and namespace (from environment variables or
hostname) to look up the pod
+// and retrieve its spec.nodeName field.
+func (cs *CSCloud) getNodeNameFromPod(ctx context.Context) (string, error) {
+ if cs.clientBuilder == nil {
+ return "", fmt.Errorf("clientBuilder not initialized, cannot
query Kubernetes API")
+ }
+
+ client, err := cs.clientBuilder.Client("cloud-controller-manager")
+ if err != nil {
+ return "", fmt.Errorf("failed to get Kubernetes client: %v",
err)
+ }
+
+ // Get pod name and namespace
+ // In Kubernetes, the pod name is available as HOSTNAME environment
variable
+ // or we can use os.Hostname() which returns the pod name
+ podName := os.Getenv("HOSTNAME")
+ if podName == "" {
+ var err error
+ podName, err = os.Hostname()
+ if err != nil {
+ return "", fmt.Errorf("failed to get pod name: %v", err)
+ }
+ }
+
+ // Get namespace from environment variable or default to kube-system
for CCM
+ namespace := os.Getenv("POD_NAMESPACE")
+ if namespace == "" {
+ // Try reading from service account namespace file (available
in pods)
+ if data, err :=
os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err ==
nil {
+ namespace = string(data)
Review Comment:
The namespace trimming logic doesn't handle potential whitespace. When
reading from `/var/run/secrets/kubernetes.io/serviceaccount/namespace`, the
data might include trailing newlines or whitespace. Consider using
`strings.TrimSpace(string(data))` instead of just `string(data)` to ensure
clean namespace values.
##########
cloudstack_instances.go:
##########
@@ -201,10 +207,22 @@ func (cs *CSCloud) InstanceMetadata(ctx context.Context,
node *corev1.Node) (*cl
}
return &cloudprovider.InstanceMetadata{
- ProviderID: cs.ProviderName(),
+ ProviderID: cs.getProviderIDFromInstanceID(instanceID),
InstanceType: instanceType,
NodeAddresses: addresses,
Zone: cs.zone,
Region: zone.Region,
}, nil
}
+
+func (cs *CSCloud) getProviderIDFromInstanceID(instanceID string) string {
+ return fmt.Sprintf("%s://%s", cs.ProviderName(), instanceID)
+}
+
+func (cs *CSCloud) getInstanceIDFromProviderID(providerID string) string {
+ parts := strings.Split(providerID, "://")
+ if len(parts) == 1 {
+ return providerID
+ }
+ return parts[1]
+}
Review Comment:
The new helper functions `getProviderIDFromInstanceID` and
`getInstanceIDFromProviderID` are missing documentation comments. Following Go
conventions, exported or internal helper functions should have comments
explaining their purpose, parameters, and return values. Add documentation
comments to clarify what these functions do and their expected input/output
formats.
##########
cloudstack_instances.go:
##########
@@ -201,10 +207,22 @@ func (cs *CSCloud) InstanceMetadata(ctx context.Context,
node *corev1.Node) (*cl
}
return &cloudprovider.InstanceMetadata{
- ProviderID: cs.ProviderName(),
+ ProviderID: cs.getProviderIDFromInstanceID(instanceID),
InstanceType: instanceType,
NodeAddresses: addresses,
Zone: cs.zone,
Region: zone.Region,
}, nil
}
+
+func (cs *CSCloud) getProviderIDFromInstanceID(instanceID string) string {
+ return fmt.Sprintf("%s://%s", cs.ProviderName(), instanceID)
+}
+
+func (cs *CSCloud) getInstanceIDFromProviderID(providerID string) string {
+ parts := strings.Split(providerID, "://")
+ if len(parts) == 1 {
+ return providerID
+ }
+ return parts[1]
+}
Review Comment:
The new helper functions `getProviderIDFromInstanceID` and
`getInstanceIDFromProviderID` lack test coverage. These are critical functions
for parsing and constructing provider IDs. Since the codebase has existing test
coverage (see `cloudstack_test.go`), these utilities should include unit tests
to verify correct behavior with various input formats (e.g., with "://",
without "://", empty strings, etc.).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
