-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
PolicyRepository: index and replace rules by resource. #32703
Conversation
/test |
Tagging @aanm for review too; you understand this bit of code somewhat. |
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.
LGTM, very nice cleanup! Do we have any performance benchmark numbers that can tell us how much this improves the startup time?
The stateful test scaffolding was getting in the way of future changes, especially surrounding the shared SelectorCache. Tests were adding identities, which affected other tests. Furthermore, testing the package with --count=2 reliably failed due to left-behind state. This mechanical change aggregates all useful variables behind a single struct. No test logic has been changed. Signed-off-by: Casey Callendrello <cdc@isovalent.com>
UID is not a safe resource for indexing; multiple rules may have the same UID, and UID is not guaranteed to be unique across different resources. Furthermore, informers may coalesce delete + add events in to an update, thus losing the UID edge regardless. Signed-off-by: Casey Callendrello <cdc@isovalent.com>
3dc803d
to
db1df64
Compare
/test |
@christarazi I threw together a quick benchmark:
Good enough for me :-) |
When upserting a CNP or KNP, we identify existing rules in the repository by a set of labels. However, evaluating this set of labels is expensive, especially as we must check against all label selectors every time we want to add or remove a policy. Rather than using label selectors internally, track policies by owning resource, much the way that prefixes are tracked in the ipcache. Then, when upserting policies, the set of existing rules attached to a given resource can be easily retrieved. The existing behavior is preserved, as it is also exposed via the local gRPC API. However, the k8s handlers no longer use it. Signed-off-by: Casey Callendrello <cdc@isovalent.com>
db1df64
to
77b43c5
Compare
Tests caught a flake -- just some test code that expected a certain ordering. Fixed. |
/test |
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.
@squeed I'm 15 mins too late but I left a comment that I believe it should be addressed.
slim_metav1 "github.com/cilium/cilium/pkg/k8s/slim/k8s/apis/meta/v1" | ||
"github.com/cilium/cilium/pkg/labels" | ||
"github.com/cilium/cilium/pkg/lock" | ||
"github.com/cilium/cilium/pkg/option" | ||
"github.com/cilium/cilium/pkg/policy/api" | ||
) | ||
|
||
type ruleKey struct { | ||
resource ipcachetypes.ResourceID | ||
idx uint |
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.
Index in reference to what? A comment here would be good.
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.
sounds good, I'll add a comment in an incoming PR.
I also ran a local benchmark to compare the PR against current main and I didn't see any regressions. |
(Note for reviewers: this also contains a commit fixing some improper use of shared state in tests; it is mostly mechanical but required for this change).
The PolicyRepository is a database of all known network policies (KNP / CNP / CCNP / gRPC Policies). It references policies by Labels, which are an arbitrary set of identifiers for a policy. When an "upstream" policy is updated, a synthetic set of labels is created and used as a key for replacement.
This label-oriented referencing is inefficient. When a policy is upserted, all existing rules must be scanned to se if they are candidates for replacement. Furthermore, this doesn't reflect how policy rules are actually created. Namely, they have an upstream owning resource, which creates one or more downstream rules in the repository.
This change reorients the PolicyRepository to index and replace rules on a per-resource basis. This pattern is already well established within Cilium, most notably the IPCache, and has proven itself. It also means that the standard policy actions do not require a whole-repository scan or evaluating labels.
The existing label-based replacement mechanism must be preserved for policies managed by the local API, so the existing code cannot be entirely removed, but it will seldom be used in the field.
(This PR was inspired by #27163, but takes a different tack).