[occm] remove SGs from non-existent nodes ports on Update #2696
Conversation
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
|
/cc dulek mdbooth |
| @@ -63,7 +63,7 @@ func applyNodeSecurityGroupIDForLB(network *gophercloud.ServiceClient, svcConf * | |||
| continue | |||
| } | |||
|
|
|||
| listOpts := neutronports.ListOpts{DeviceID: serverID} | |||
| listOpts := neutronports.ListOpts{} | |||
There was a problem hiding this comment.
IMO this is an expensive request. I wonder if we can do better. The reason why I did it this way is to limit the number of API calls, but now this list might be really big in some deployments.
An alternative could be to make two queries: one that lists by SG and one that list by server ID.
There was a problem hiding this comment.
This will definitely increase the load
There was a problem hiding this comment.
an alternative could be another loop where we filter by SGs and update the ports... but it's also some load.
| @@ -75,6 +75,23 @@ func applyNodeSecurityGroupIDForLB(network *gophercloud.ServiceClient, svcConf * | |||
| continue | |||
| } | |||
|
|
|||
| if port.DeviceID != serverID { | |||
There was a problem hiding this comment.
should we have a check similar to line 101?
There was a problem hiding this comment.
Good point. I actually think we can move the new block after L101, which will help to leave the loop earlier if the port isn't part of the LB's subnet.
What this PR does / why we need it:
Update
applyNodeSecurityGroupIDForLB()to also lookup ports that have the SG tag and detach the SG from the portsthat don't belong to the ports of the current list of Nodes.
This will ensure ports are cleaned up if a Node is being removed from a Loadbalancer.
Which issue this PR fixes(if applicable):
fixes #2695
Release note: