Skip unnecessary node events in config reconciler#1607
Skip unnecessary node events in config reconciler#1607fedepaol merged 2 commits intometallb:mainfrom
Conversation
pperiyasamy
requested review from
fedepaol,
gclawes,
johananl,
oribon,
rata and
russellb
as code owners
| } | ||
|
|
||
| for _, node := range nodes.Items { | ||
| r.Nodes[node.Name] = node |
There was a problem hiding this comment.
I think every round must be deleted and recreated otherwise we won't be able to deal with deletions (or, you must iterate over it and remove nodes not in items, but recreating seems enough)
There was a problem hiding this comment.
correct, but anyway this local cache is no longer needed with new predicate version which overrides UpdateFunc.
| if !ok { | ||
| return true | ||
| } | ||
| err := r.Get(context.Background(), types.NamespacedName{Namespace: newNodeObj.Namespace, |
There was a problem hiding this comment.
I'd use the predicate version with the action instead of doing the get.
| return false | ||
| } | ||
| if existingNodeObj, ok := r.Nodes[newNodeObj.Name]; ok && | ||
| labels.Set(existingNodeObj.Labels).String() == labels.Set(newNodeObj.Labels).String() { |
There was a problem hiding this comment.
will these be always in the right order?
There was a problem hiding this comment.
yes, changed to use labels.Equals method.
|
Can you add a unit test for this scenario? |
pperiyasamy
force-pushed
the
skip-node-unnecessary-event
branch
from
74ab08a to
dbdc588
Compare
tasks.py
Outdated
| run("go test -short -race ./...") | ||
| envtest_asset_dir = os.getcwd() + "/testbin" | ||
| run('sudo rm -rf "%s"' % envtest_asset_dir) | ||
| os.mkdir(envtest_asset_dir) |
There was a problem hiding this comment.
this is for setting up envtest cluster as it is needed for running TestNodeEvent test.
There was a problem hiding this comment.
Can we just add a copy of the file locally (under dev-env/unittests maybe) instead of fetching it every time?
Would it be better (just wondering)?
|
LGTM, just a question about the test setup |
pperiyasamy
force-pushed
the
skip-node-unnecessary-event
branch
from
dbdc588 to
953a05c
Compare
tasks.py
Outdated
| envtest_asset_dir = os.getcwd() + "/dev-env/unittest" | ||
| run("source {}/setup-envtest.sh; fetch_envtest_tools {}; setup_envtest_env {}; go test -short ./...".format(envtest_asset_dir, envtest_asset_dir, envtest_asset_dir), echo=True) | ||
| run("source {}/setup-envtest.sh; fetch_envtest_tools {}; setup_envtest_env {}; go test -short -race ./...".format(envtest_asset_dir, envtest_asset_dir, envtest_asset_dir), echo=True) | ||
| run("sudo rm -rf {}/bin".format(envtest_asset_dir), echo=True) |
There was a problem hiding this comment.
I'd just add it to gitignore and not remove it, otherwise every time we run unit tests (locally) we'll download everything again.
tasks.py
Outdated
| run("go test -short -race ./...") | ||
|
|
||
| envtest_asset_dir = os.getcwd() + "/dev-env/unittest" | ||
| run("source {}/setup-envtest.sh; fetch_envtest_tools {}; setup_envtest_env {}; go test -short ./...".format(envtest_asset_dir, envtest_asset_dir, envtest_asset_dir), echo=True) |
There was a problem hiding this comment.
We can run fetch once I guess, maybe even setup, before running the two tests?
The config controller watches node event to populate l2 and bgp advertisement objects with selected nodes based on node selector labels. But currently a change in the node results in reprocessing the configuration, which results in reprocessing the services. This results in an unnecessary cpu load. Hence this change skips processing unnecessary node events other than node create, node update (if its with new labels) and node delete events. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
pperiyasamy
force-pushed
the
skip-node-unnecessary-event
branch
from
953a05c to
3a55c33
Compare
|
LGTM |
2 participants
The config controller watches node event to populate l2 and bgp advertisement objects with selected nodes based on node selector labels.
But currently a change in the node results in reprocessing the configuration, which results in reprocessing the services. This results in an unnecessary cpu load.
Hence this change skips processing unnecessary node events other than node create, node update (if its with new labels) and node delete events.
Signed-off-by: Periyasamy Palanisamy pepalani@redhat.com