DaanHoogland commented on code in PR #105:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/105#discussion_r3949558531
##########
cloudstack_loadbalancer.go:
##########
@@ -109,6 +114,12 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context,
clusterName string, s
return nil, err
}
+ // Converge on one rule per name before reconciling, so a duplicate
left by
+ // an earlier partial run does not persist for the life of the service.
Review Comment:
I am confused by this comment; reconciling?
##########
cloudstack.go:
##########
@@ -128,7 +128,14 @@ func (cs *CSCloud) getManagementServerVersion()
(semver.Version, error) {
return semver.Version{}, errors.New("no management servers
found")
}
version := msServersResp.ManagementServersMetrics[0].Version
- v, err := semver.ParseTolerant(strings.Join(strings.Split(version,
".")[0:3], "."))
+ // Trim to major.minor.patch. Slicing blindly would panic on a version
+ // string with fewer than three components, crashing the controller at
+ // startup instead of reporting a parse failure.
+ parts := strings.Split(version, ".")
+ if len(parts) > 3 {
+ parts = parts[:3]
+ }
+ v, err := semver.ParseTolerant(strings.Join(parts, "."))
Review Comment:
how about 2 or less? should we guard that as well?
##########
cloudstack_loadbalancer.go:
##########
@@ -61,14 +61,19 @@ const (
type loadBalancer struct {
*cloudstack.CloudStackClient
- name string
- algorithm string
- hostIDs []string
- ipAddr string
- ipAddrID string
- networkID string
- projectID string
- rules map[string]*cloudstack.LoadBalancerRule
+ name string
+ algorithm string
+ hostIDs []string
+ ipAddr string
+ ipAddrID string
+ networkID string
+ projectID string
+ rules map[string]*cloudstack.LoadBalancerRule
+ // duplicateRules holds rules whose name collides with one already in
+ // rules. CloudStack does not enforce unique load balancer rule names,
so a
+ // name-keyed map can only ever manage one of them; without tracking the
+ // rest here they would survive EnsureLoadBalancerDeleted and leak.
Review Comment:
I am a bit averse to these kinds of comments `duplicateRules` is a rather
clear name, so if the use of the field is clear as well, why this comment?
--
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]
