Implement bulk updates to IP sets #1357
Conversation
|
retest this please |
fasaxc
force-pushed
the
ipsets-perf
branch
4 times, most recently
from
f733534
to
1ec7f09
Compare
| members set.Set | ||
|
|
||
| // pendingReplace is either nil to indicate that there is no pending replace or a set | ||
| // containing all the entries that we want to write. |
There was a problem hiding this comment.
Can you extend this comment to provide similar info for pendingAdds and pendingDeletions?
ipsets/ipset_defs.go
Outdated
| pendingDeletions set.Set | ||
| } | ||
|
|
||
| // IPVersionConfig wraps up the metadata for a particular IP version. It can be used by other |
There was a problem hiding this comment.
Delete "other" at end of previous line.
ipsets/ipset_defs.go
Outdated
| versionedPrefixes = append(versionedPrefixes, prefix+version) | ||
| } | ||
| versionedPrefixes = append(versionedPrefixes, extraUnversionedIPSets...) | ||
| ourNamesPattern := "^(" + strings.Join(versionedPrefixes, "|") + ")" |
There was a problem hiding this comment.
Should really regexp-quote each element of versionedPrefixes, here.
ipsets/ipset_defs.go
Outdated
| // NameForMainIPSet converts the given IP set ID (example: "qMt7iLlGDhvLnCjM0l9nzxbabcd"), to | ||
| // a name for use in the dataplane. The return value will have the configured prefix and is | ||
| // guaranteed to be short enough to use as an ipset name (example: | ||
| // "cali6ts:qMt7iLlGDhvLnCjM0l9nzxb"). |
There was a problem hiding this comment.
cali6-s:qMt7iLlGDhvLnCjM0l9nzxb
ipsets/ipsets.go
Outdated
| "github.com/projectcalico/felix/set" | ||
| ) | ||
|
|
||
| // IPSets manages a whole "plane" of IP sets, i.e. all the IPv4 or IPv6 IP sets. |
There was a problem hiding this comment.
I'm being picky, but someone could still misunderstand that. I think it could be made idiot-proof as "all the IPv4 IP sets, or all the IPv6 IP sets."
ipsets/ipsets.go
Outdated
| line := scanner.Text() | ||
| if line == "" { | ||
| // End of members | ||
| ipSetName = "" |
There was a problem hiding this comment.
Don't reset ipSetName here; instead do it at the close of the for loop, so as also to catch the EOF case.
ipsets/ipsets.go
Outdated
| return nil | ||
| } | ||
| logCxt.Info("Calculating deltas to IP set") | ||
| return s.writeDeltas(ipSet, w) |
There was a problem hiding this comment.
Pass down (and use) the logCxt here?
ipsets/ipsets.go
Outdated
| // - membersInDataplane nil | ||
| // - pendingAdds/Deletions empty. | ||
| logCxt.Info("Doing full IP set rewrite") | ||
| return s.writeFullRewrite(ipSet, w) |
ipsets/ipsets.go
Outdated
| } | ||
|
|
||
| // ApplyDeletions tries to delete any IP sets that are no longer needed. | ||
| // Failures are ignored, deletions will be retried the next time AttemptCleanup() is called. |
There was a problem hiding this comment.
s/AttemptCleanup/ApplyDeletions/
ipsets/ipsets.go
Outdated
| return set.RemoveItem | ||
| }) | ||
|
|
||
| s.gaugeNumIpsets.Set(float64(len(s.ipSetIDToIPSet))) |
There was a problem hiding this comment.
Presumably s.ipSetIDToIPSet should be decremented in this code path, but I don't see where that is happening.
There was a problem hiding this comment.
I was just using this as a point in the code where we know we've got no updates in flight. Added a comment.
- Move ipset restore logic to IPSets "manager" class so multiple writes can be grouped together. - Do writes in a streaming fashion to minimise need for buffering in memory. - Canonicalise IP set members when adding to IPset in preparation for impelmenting resync.
- Load IP sets from datatplane and convert members to canonical form. - Compare with expected values and queue up updates to repair.
fasaxc
force-pushed
the
ipsets-perf
branch
2 times, most recently
from
148b510
to
9a780a5
Compare
|
@neiljerram Think I've addressed all your comments now. Back to you for re-review. |
|
retest this please |
1 similar comment
|
retest this please |
Implement bulk updates to IP sets
- Improve IP sets update performance by batching multiple set updates into single ipset restore calls.
- Avoid buffering large amounts of input/output by doing streaming reads/writes from pipes to the child process.
- Replace `AttemptCleanup()` start-of-day cleanup routine with a resync mechanism that reads back the contents of the dataplane. Works around / Fixes #1347.
Design:
- Move write function to `Registry`, renamed as `IPSets`.
- Reduce IPSet struct to a data struct rather than a fully fledged object to avoid having an intricate coupling between the two objects.
- To support comparison during resync, convert IPs/CIDRs to canonical form and store them as ip.Addr/ip.CIDR objects instead.
- Remove IP set existence cache since logic is now rolled into resync.
AttemptCleanup()start-of-day cleanup routine with a resync mechanism that reads back the contents of the dataplane. Works around / Fixes #1347.Design:
Registry, renamed asIPSets.TODO: