Add annotation sidecar.istio.io/disableIPEarlyDemux

[luksa]

Ref: istio/istio#38982

[istio-policy-bot]

😊 Welcome @luksa! This is either your first contribution to the Istio api repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

[istio-testing]

Hi @luksa. 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]

What is the user experience we want here? A per-pod annotation doesn't seem like the right approach. When should a user set it or not set it?

It seems like either:

• There are pros and cons to setting it => we need to understand what those are to decide where this should live, and explain it in the user documentation
• Its universally better => we don't need controls; we can add a (temporary?) env var opt out in case we hit unknown-unknowns and a user needs to opt-out

[luksa]

The reason I took the per-pod approach is because I am not sure how change affects performance. The early demux feature was supposed to be an optimization, but in some cases leads to a reduction of throughput (see https://patchwork.ozlabs.org/project/netdev/patch/[email protected]/), which is why the sysctl option was then introduced.

I feel like users will want to use the disableIPEarlyDemux annotation only on pods that expose more than one port. I don't feel I can get a definitive answer on whether making this change globally would be okay, hence the per-pod option, just to be safe.

IMHO, this option should only be a (temporary) workaround, as I still think this is a Kernel bug that should be fixed (optimizations shouldn't break stuff).

[bleggett]

Reading the linked issues - yeah - mostly end up echoing @howardjohn on this:

1. How do we plan to surface this setting to people and advise them to use it?
2. Given that TCP behavior is effectively broken if we don't disable it - should we just flat-out disable it? I think we should.

I don't think this makes much sense as a pod annotation, I think this should be a global config for the CNI - effectively saying just take istio/istio#46457 and not make a pod-level API surface change - or at least we shouldn't offer pod granularity yet when there's no indication people need it at that level and describing to people how they should configure it at that level seems messy. It can/should just be a simple toggle for the CNI daemonset itself.

We should do a perftest to find out if there's a negligible impact to perf or not, and if there is not, just turn it on so we get consistent behavior by default.

Even if the perftest says the difference is not negligible, I think it should probably still be a CNI daemonset-level flag.

(maybe default-off for now, until we have a better idea of how it ends up affecting perf for real traffic with users who know what this setting is for and when they need it)

[howardjohn]

One other question I had - there is also tcp_ and udp_ variants. Does setting only the TCP work for us?

FWIW I sent a message to the google kernel networking team to get advice, haven't heard back yet

[linsun]

Any movement on this? It does sound per pod is reasonable given the reasonings @luksa outlined - this is only needed for pods with 1+ ports

[linsun]

would be good to explain when this should be considered?

[linsun]

/ok-to-test

[istio-testing]

@luksa: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
release-notes_api	7a6042a	link	false	/test release-notes
gencheck_api	7a6042a	link	true	/test gencheck

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. I understand the commands that are listed here.

[istio-testing]

PR needs rebase.

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.