-
Notifications
You must be signed in to change notification settings - Fork 16
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
WIP: Harvester load balancer enhancement #8
Conversation
2a11e16
to
7395c59
Compare
bc82dd8
to
66f3219
Compare
} | ||
logrus.Infof("VMI %s has been changed", vmi.Name) | ||
|
||
lbs, err := h.lbCache.List("", labels.Everything()) |
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.
only list ! lbv1.Cluster or lbv1.VM will be efficiency
return nil, nil | ||
} | ||
|
||
lbs, err := h.lbCache.List("", labels.Everything()) |
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.
similar
|
||
for _, lb := range lbs { | ||
// remove the backend server from the load balancers | ||
if err := h.removeServerFromLB(vmi, lb); err != nil { |
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.
re-use matchBackendServers to check if this vmi is in the list at first ?
potential efficiency problem here:
(1) the vmi may have no relationship with any lb, but all lb (all endpoints) will be checked; if later m.endpointSliceCache.Get(lb.Namespace, lb.Name) fail, it will return error, but this lb may be irrelevant at all
(2) when there are many lbs...
return nil, err | ||
} | ||
|
||
// add or update the backend server to the matched load balancer |
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.
vmi changes frequently; for most of the time, it is not related to lb
the lb checks vmi via iterating endpointslice, each time
it seems need a fast way, e.g. lb looks record info of vmi (e.g. id, ip), to have a one step match check
pkg/controller/ippool/controller.go
Outdated
} | ||
|
||
if ipPool.Spec.Ranges == nil || len(ipPool.Spec.Ranges) == 0 { | ||
return nil, fmt.Errorf("invalid ip pool, range could not be empty") |
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.
As a controller, this file lacks of essential log
918cd2a
to
c55533a
Compare
82d0c86
to
fd49f07
Compare
7f65b07
to
a82e037
Compare
24163c1
to
044ecd8
Compare
10860cc
to
0f59a4f
Compare
Need to update vendor after dependency PRs merged. |
- Add IP Pool module to support ipam for the load balancer - Support two workload type load balancer, including VM and cluster
close because replaced by PR #13. |
Related issue:
harvester/harvester#1762
harvester/harvester#1580
design document:
harvester/harvester#1913
Dependency:
harvester/harvester#3776
harvester/webhook#3