Pilot scoped namespaces

[harveyxia]

Add ability to scope pilot discovery to a specified set of namespaces in order to improve performance on both the control-plane (process fewer services/pods/endpoints) and data-plane (reduce both size and frequency of xDS updates to proxies).

For more context, view the design document(shared in the Istio networking group drive) and the associated issue, #26679.

And to help us figure out who should review this PR, please
put an X in all the areas that this PR affects.

[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[X] Networking
[X] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

Pull Request Attributes

Please check any characteristics that apply to this pull request.

[ ] Does not have any changes that may affect Istio users.

---

Performance profiles:

The testing methodology consists of generating service churn by creating 100 services in a specified namespace and profiling CPU usage on an envoy proxy.

For Istio 1.8.1:

With the changes in this PR, in a namespace not labeled for discovery:

We observe that xDS updates are no longer issued for Service events in the omitted namespace.

[google-cla]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[istio-testing]

Hi @harveyxia. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[howardjohn]

/ok-to-test

[howardjohn]

While the current label is istio-injection, its mostly that out of legacy. Any new labels should be *.istio.io/* format to conform to k8s best practice

[harveyxia]

Any opinions on the exact label string? Would discovery.istio.io/enable be conventional?

[frankbu]

There was some discussion about allowing more than one istiod to managed (namespace-filtered) meshes in the same cluster. This implementation doesn't seem to support that. If it is required, the value of this label needs to be a meshId, not just boolean.

[venkatgattupalli]

If I understand correctly, the recently proposed discoverySelectors option should enable multiple istiod's to manage mesh in the same cluster with non overlapping selectors.
@harveyxia: NewNamespaceController should also honor this selector configuration and insert istio-ca-root-cert config map to only filtered namespaces.

[l8huang]

NamespaceController should also use DiscoveryNamespacesFilter
@harveyxia do you plan to update NamespaceController?

[harveyxia]

@l8huang The original motivation for this implementation was primarily for performance improvement (filtering out irrelevant services, pods, and endpoints) , so we kept the DiscoveryNamespacesFilter scoped to the main service registry controller. With some refactoring (making DiscoveryNamespacesFilter self contained and exposing methods for registering handlers for meshConfig and namespace events) it can be extended for use with other components.

However, before we do any implementation, we should determine whether this feature is the right abstraction for enabling multiple istiod deployments (which is an orthogonal concern to performance), or if there are other designs / existing components that would be better suited for this use case. @howardjohn @costinm any thoughts?

[howardjohn]

do you have any profiles on istiod side? to see if the filtering is taking any noticeable CPU/mem usage?

[harveyxia]

Without the filter:

With the filter:

There appears to be less CPU utilization (50ms -> 20ms) by the k8s watch when adding the filter

[hzxuzhonghu]

It seems the cost is very big, what if we remove the filter when get/list

[harveyxia]

@hzxuzhonghu Can you elaborate on where you're seeing the cost incurred by the filtering itself? Zooming in on the second profile I posted above (the one with the filter applied), it seems the additional cost of the filter consists of map accesses and string equality comparisons. But overall the filter appears to be conserving CPU usage, since there are fewer resources by processed.

With respect to removing the filter from get/list, for correctness we want the filter to be applied to all reads, and there are numerous usages of List() and GetByKey() throughout the kube service registry

[howardjohn]

I don't think the profiler can give accurate numbers at such low load

[harveyxia]

I conducted another profile using pilot-load to generate load against pilot. With the cache-backed filter approach implemented in commit a3d6016, the performance impact of the filtering itself should be nominal since each invocation of the filter only requires constant time access into a map (as opposed to the previous implementation which required iteration of all namespaces).

Without filtering:

With filtering enabled and no labeled namespaces:

With filtering enabled and 10 labeled namespaces:

P.S. I have each of these profiles saved, so we can examine in greater detail if needed

[hzxuzhonghu]

When namespace updated(labels added/removed), a new full push should be triggered.

[hzxuzhonghu]

need to resolve, otherwise very confusing

[harveyxia]

When namespace updated(labels added/removed), a new full push should be triggered.

Discussed offline, full pushes will be triggered by the relevant resource event handlers (e.g. service event handler triggers a full push here, pod event handler triggers a full push here. These resource specific event handlers are invoked by the new namespace controller introduced in this PR when namespaces are newly labeled or delabled

[hzxuzhonghu]

For reviewers, this should be closely looked at.

[mfanjie]

Writing to every resource in the cluster seems problematic for security, api server load, and latency (delays to add the label). We have a pretty strong assumption of being read-only.
…
On Wed, Jan 27, 2021, 6:53 PM Jesse Meng @.***> wrote: We need similar feature to support very large scale, but we still facing challenge if using primary-remote deployment as in each cluster, we have 15k~20k pods. Even this PR introduces informer level filter, the client still watch all object changes, which would high possible to cause problem. Is it possible to do it in such way: 1. add some built-in labels in namespace, similar like the service exportTo annotation, e.g. networking.istio.io/managedBy. 2. add a controller to propagate the labels from namespace to all related objects in the namespace, including service, ep, istio networking objects. 3. define the discoverySelector in meshConfig but only accept the predefined keys 4. in Polit code, discover all object by the label filters. With this approach, all the filters are done in server side, then it is possible to setup mesh with controlled scale in any scale of Kubernetes clusters — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#29802 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEYGXMN3OJUBQML36TV5T3S4DGUDANCNFSM4VNLTZMA .

Then the label propagation step can be left to users, Isito only to provide a capability to do xDS based on some label filters.

[howardjohn]

If we just did that it would be pretty simple right? just adding a 1 line label selector to all the watches?

…

On Wed, Jan 27, 2021, 7:02 PM Jesse Meng ***@***.***> wrote: Writing to every resource in the cluster seems problematic for security, api server load, and latency (delays to add the label). We have a pretty strong assumption of being read-only. … <#m_-5932500428813014275_> On Wed, Jan 27, 2021, 6:53 PM Jesse Meng *@*.***> wrote: We need similar feature to support very large scale, but we still facing challenge if using primary-remote deployment as in each cluster, we have 15k~20k pods. Even this PR introduces informer level filter, the client still watch all object changes, which would high possible to cause problem. Is it possible to do it in such way: 1. add some built-in labels in namespace, similar like the service exportTo annotation, e.g. networking.istio.io/managedBy. 2. add a controller to propagate the labels from namespace to all related objects in the namespace, including service, ep, istio networking objects. 3. define the discoverySelector in meshConfig but only accept the predefined keys 4. in Polit code, discover all object by the label filters. With this approach, all the filters are done in server side, then it is possible to setup mesh with controlled scale in any scale of Kubernetes clusters — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#29802 (comment) <#29802 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEYGXMN3OJUBQML36TV5T3S4DGUDANCNFSM4VNLTZMA . Then the label propagation step can be left to users, Isito only to provide a capability to do xDS based on some label filters. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#29802 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXMRF3WQPFP4IE6TJCDS4DHWBANCNFSM4VNLTZMA> .

[mfanjie]

@howardjohn, right, it's a small change in the code, and it resolves the scalability issue we are facing.
We don't want to have different code base with upstream, so I am here to check if upstream can have similar or better solution.

[hzxuzhonghu]

We can consider this option, but we could not enforce all users do that.

[mfanjie]

We can consider this option, but we could not enforce all users do that.

IMHO, we can keep the discoverySelector configuration which is introduced by this PR, by default, no selector is defined, and Isito behaves exactly same with now, which discovery all configuration and status.
If user define discoverySelector in meshConfig, then it indicates user want to filter the objects in the Kubernetes cluster, so the istio objects to be managed must have labels match the selector.
It's all up to mesh configuration and user's operation.

[harveyxia]

/retest

[harveyxia]

@howardjohn @hzxuzhonghu The istio/api version has been bumped, this PR is ready for a final review

[christian-posta]

@howardjohn @linsun @nrjpoddar can we get final sign off on this? need one more approval

[linsun]

Can you specify the option (DiscoverySelectors?) here?

As a follow up - Would love to have a some doc that teaches user how to use this. The guideline is to put doc on istio wiki for experimental feature (I assume this is).

[linsun]

also wonder if it is useful to ask users to visualize the mesh (like using kiali) to determine the correct configuration for the discovery selectors.

[harveyxia]

I'm happy to follow up with documentation on how to utilize this feature, I'll follow up offline to discuss

[linsun]

Any consideration for upgrade? I believe this is not enabled by default, but if it is enabled, users would need to ensure the discoverySelectors config is correct else apps could be broken.

[harveyxia]

this is indeed disabled by default—should I still include an upgrade note describing usage of discoverySelectors?

[linsun]

Since it is disabled, no. Would be good to have it in your docs on consideration when enabling this. :)

[linsun]

I didn't look at code closely but top level changes LGTM

Added some comment for readme, would love to have them addressed or knowing a plan to address them before approving.

[howardjohn]

DiscoveryNamespaces (namespaces I watch) and "namespaces in the mesh" are distinct concerns and should not be mixed. Typically the former is a superset of the latter, as we want to reach pods outside the mesh but in the cluster.

…

On Thu, Feb 25, 2021 at 6:16 AM Harvey Xia ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pilot/pkg/serviceregistry/kube/controller/filter/discoverynamespaces.go <#29802 (comment)>: > +// limitations under the License. + +package filter + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/util/sets" + v1 "k8s.io/client-go/listers/core/v1" + + "istio.io/pkg/log" +) + +const ( + // PilotDiscoveryLabelName is the name of the label used to indicate a namespace for discovery + PilotDiscoveryLabelName = "istio-discovery" @l8huang <https://github.com/l8huang> The original motivation for this implementation was primarily for performance improvement (filtering out irrelevant services, pods, and endpoints) , so we kept the DiscoveryNamespacesFilter scoped to the main service registry controller. With some refactoring (making DiscoveryNamespacesFilter self contained and exposing methods for registering handlers for meshConfig and namespace events) it can be extended for use with other components. However, before we do any implementation, we should determine whether this feature is the right abstraction for enabling multiple istiod deployments (which is an orthogonal concern to performance), or if there are other designs / existing components that would be better suited for this use case. @howardjohn <https://github.com/howardjohn> @costinm <https://github.com/costinm> any thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29802 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXKVBCRPNXBQTRCRGX3TAZLTNANCNFSM4VNLTZMA> .