Fix providerID & getZone Implementation #85
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #58 by correcting the providerID format and improving the GetZone implementation for CloudStack cloud provider integration with Kubernetes. The changes address two key problems: (1) providerID was being set to just the provider name instead of the full "provider://instanceID" format, and (2) GetZone was using os.Hostname() which returns the pod name in containerized environments rather than the actual node name.
- Added helper functions to convert between provider IDs and instance IDs in the correct "provider://instanceID" format
- Modified GetZone to query the Kubernetes API for the node name when the cloud controller manager runs in a pod
- Added RBAC permissions for the cloud controller manager to read pod information
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| deployment.yaml | Adds RBAC permissions (list, get) for pods resource to allow the cloud controller manager to query pod information |
| cloudstack_instances.go | Implements helper functions for providerID format conversion and updates all *ByProviderID methods to use the correct instance ID extraction |
| cloudstack.go | Adds clientBuilder field, implements getNodeNameFromPod to retrieve node name from pod spec via Kubernetes API, and updates GetZone to use this new method |
Comments suppressed due to low confidence (1)
cloudstack.go:196
- The
cs.zonefield is being read and written without synchronization in theGetZonemethod. IfGetZoneis called concurrently from multiple goroutines whencs.zoneis empty, there's a race condition where multiple goroutines could simultaneously query the CloudStack API and overwritecs.zone. Consider using a sync.Once or mutex to ensure thread-safe initialization of the zone field.
if cs.zone == "" {
// In Kubernetes pods, os.Hostname() returns the pod name, not the node hostname.
// We need to get the node name from the pod's spec.nodeName using the Kubernetes API.
nodeName, err := cs.getNodeNameFromPod(ctx)
if err != nil {
return zone, fmt.Errorf("failed to get node name for retrieving the zone: %v", err)
}
instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByName(
nodeName,
cloudstack.WithProject(cs.projectID),
)
if err != nil {
if count == 0 {
return zone, fmt.Errorf("could not find CloudStack instance with name %s for retrieving the zone: %v", nodeName, err)
}
return zone, fmt.Errorf("error getting instance for retrieving the zone: %v", err)
}
cs.zone = instance.Zonename
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parts := strings.Split(providerID, "://") | ||
| if len(parts) == 1 { | ||
| return providerID | ||
| } | ||
| return parts[1] |
There was a problem hiding this 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 "://".
| 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] | ||
| } |
There was a problem hiding this 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.
vishesh92
force-pushed
the
fix-provider-id
branch
from
67dc886 to
4e36f6c
Compare
vishesh92
force-pushed
the
fix-provider-id
branch
from
4e36f6c to
6db6b95
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
==========================================
- Coverage 15.09% 14.40% -0.70%
==========================================
Files 4 4
Lines 808 847 +39
==========================================
Hits 122 122
- Misses 684 723 +39
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kiranchavala
left a comment
kiranchavala
left a comment
There was a problem hiding this comment.
LGTM
Enable autoscaling on the cks cluster ,
On the new nodes that get added to the cks cluster during scaling
Execute the following command
kubectl taint nodes <nodename> node.cloudprovider.kubernetes.io/uninitialized=true:NoSchedule
kubectl get node cks2-node-19b0bf63dad --show-labels
NAME STATUS ROLES AGE VERSION LABELS
cks2-node-19b0bf63dad Ready <none> 55m v1.33.1 beta.kubernetes.io/arch=amd64,beta.kubernetes.io/instance-type=CKS,beta.kubernetes.io/os=linux,failure-domain.beta.kubernetes.io/region=ref-trl-10334-k-Mol8-kiran-chavala,failure-domain.beta.kubernetes.io/zone=ref-trl-10334-k-Mol8-kiran-chavala,kubernetes.io/arch=amd64,kubernetes.io/hostname=cks2-node-19b0bf63dad,kubernetes.io/os=linux,node.kubernetes.io/instance-type=CKS,topology.kubernetes.io/region=ref-trl-10334-k-Mol8-kiran-chavala,topology.kubernetes.io/zone=ref-trl-10334-k-Mol8-kiran-chavala
4 participants
Fix #58