Adds concept documentation for CiliumEndpointSlice #17430

Weil0ng · 2021-09-17T19:14:54Z

See commit msg.

Document CES concept
Document racing issue when identity change, see Identity based batching race condition Issues #17520 (comment)
Document known issue with egress gateway and SRv6, tracked in Egress Gateway doesn't work with CiliumEndpointSlices #17669
Document upgrade impact

pchaigno · 2021-11-09T15:03:18Z

This pull request can only be merged once #17658 has been merged, right?

joestringer

Thanks for documenting this, looks good overall. Just a few minor nits below.

joestringer · 2021-11-09T17:55:32Z

Documentation/gettingstarted/ciliumendpointslice.rst

+A mitigation strategy is to upgrade the operator from non-CES to CES first.
+This will let it start creating CESs right away, while the agents are still
+watching CEP resources.


I think that this needs to be a bit more explicit, at the very least something like this:

Suggested change

A mitigation strategy is to upgrade the operator from non-CES to CES first.

This will let it start creating CESs right away, while the agents are still

watching CEP resources.

A mitigation strategy is to upgrade the operator from non-CES to CES before

upgrading agents on each node.

This will let it start creating CESs right away, while the agents are still

watching CEP resources.

But also the next questions are "how long is long enough to wait after upgrading the operator?" and "What are the concrete steps to ensure that this upgrade occurs in this order?

I think the second one of these questions may take some effort to think through and come up with the concrete steps, which maybe shouldn't be necessary at this point with a beta version.. But also, by the time this goes GA, I will imagine that we want full, reliable instructions in the main upgrade guide for how to implement this upgrade safely.

how long is long enough to wait after upgrading the operator?

There's really no tell in current implementation, but I imagine we could either let the operator output some signal, or better, we could implement a check in the agent to see if CES store has all the CESs required to represent current CEPs, and only then start watching CES.

What are the concrete steps to ensure that this upgrade occurs in this order

I think going GA, what we can do is to employ a 2-step upgrade again here. We put the operator change into some future 1.11 ver and let it create CES by default. Then in 1.12 release, we toggle agent watch by default. If we do this, then maybe there's no point implementing the checks I said above...

oh also took your suggestion, edited text :)

There's really no tell in current implementation, but I imagine we could either let the operator output some signal, or better, we could implement a check in the agent to see if CES store has all the CESs required to represent current CEPs, and only then start watching CES.

I think that regardless of what else we do, this kind of signal would be really nice to have for debugability. Assuming that folks did the upgrade correctly and everything is fine works alright if everything is operating correctly and there's no unexpected bugs or issues in particular environments. But introspecting the actual state at a point in time and being able to make assessments on the health of the migration can be super helpful.

What are the concrete steps to ensure that this upgrade occurs in this order

I think going GA, what we can do is to employ a 2-step upgrade again here. We put the operator change into some future 1.11 ver and let it create CES by default. Then in 1.12 release, we toggle agent watch by default. If we do this, then maybe there's no point implementing the checks I said above...

In principle I like the idea, but I would be a bit concerned about silently turning this on in a future 1.11.x bugfix release. Enabling it in v1.11.0 and finding out that there is some issue with it (eg leaking resources, GC doesn't work, too high CPU, unexpected apiserver interactions, whatever the bugs may be) is not too bad. Introducing that later in the release cycle would increase the risk for upgrades that users will otherwise expect to be low-risk.

I think that regardless of what else we do, this kind of signal would be really nice to have for debugability.

I agree, created #17850 as a follow up item.

Enabling it in v1.11.0 and finding out that there is some issue with it (eg leaking resources, GC doesn't work, too high CPU, unexpected apiserver interactions, whatever the bugs may be) is not too bad.

Makes sense, should we enable operator piece by default now then (maybe in a quick follow up PR before 1.11 cut)?

yeah please do follow up with a dedicated PR after we merge the core implementation and we can have that discussion / gauge the benefit vs. risk.

joestringer · 2021-11-09T17:56:45Z

Documentation/gettingstarted/ciliumendpointslice.rst

+In current implementation, we added a delay (default: 1s) before sending
+out the DELETE event. This should greatly reduce the probability of
+connection disruption in most cases. 


nit, tone:

Suggested change

In current implementation, we added a delay (default: 1s) before sending

out the DELETE event. This should greatly reduce the probability of

connection disruption in most cases.

In current implementation, Cilium adds a delay (default: 1s) before sending

out the DELETE event. This should greatly reduce the probability of

connection disruption in most cases.

joestringer · 2021-11-09T17:58:10Z

Documentation/gettingstarted/ciliumendpointslice.rst

+During this period, new endpoints might experience longer delay before
+their identities will propagate to remote nodes.


I wonder if we can add another sentence after this to describe the impact here, something like "If a newly created pod is affected by this delay, then it may temporarily fail to establish connectivity to remote pods in the cluster."

joestringer · 2021-11-09T17:58:57Z

Documentation/gettingstarted/ciliumendpointslice.rst

+.. code-block:: shell-session
+
+   kubectl get pods -n kube-system -l k8s-app=cilium
+   NAMESPACE     NAME           READY   STATUS    RESTARTS   AGE
+   kube-system   cilium-7tnkq   1/1     Running   0          1d
+
+   kubectl get pods -n kube-system -l io.cilium/app=operator


I think if this is shell-session with combined commands + output then it needs the shell symbols:

Suggested change

.. code-block:: shell-session

kubectl get pods -n kube-system -l k8s-app=cilium

NAMESPACE NAME READY STATUS RESTARTS AGE

kube-system cilium-7tnkq 1/1 Running 0 1d

kubectl get pods -n kube-system -l io.cilium/app=operator

.. code-block:: shell-session

$ kubectl get pods -n kube-system -l k8s-app=cilium

NAMESPACE NAME READY STATUS RESTARTS AGE

kube-system cilium-7tnkq 1/1 Running 0 1d

$ kubectl get pods -n kube-system -l io.cilium/app=operator

(same below, you should be able to see the difference in the Netlify render output or by building the docs locally).

joestringer · 2021-11-09T18:00:13Z

Documentation/gettingstarted/ciliumendpointslice.rst

+  helm install cilium |CHART_RELEASE| \\
+    --set enableCiliumEndpointSlice=true


Is this indented the same level as the first column character of parsed-literal? It looks like the indentation is not enough, which usually causes sphinx formatting issues.

Weil0ng · 2021-11-10T00:13:06Z

This pull request can only be merged once #17658 has been merged, right?

Yes, I think so, thanks for setting the flag :)

joestringer · 2021-11-10T19:05:42Z

Documentation/gettingstarted/ciliumendpointslice.rst

+
+Egress Gateway Won't Work with CES
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+This is a temporary issue until Github Issue #17669 is fixed. If you


There is a standard way to reference github issues in the docs, I think :gh-issue:17669should do the trick. Might need to double-check how that renders in the middle of the sentence but you can also grep around forgh-issue` to understand how we typically reference github issues in other parts of the docs.

joestringer

Just one more nit on linking the github issue for one of these limitations.

Signed-off-by: Weilong Cui <cuiwl@google.com>

Weil0ng · 2021-11-11T23:51:42Z

Blocking PR #17658 has merged.

Weil0ng added the area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. label Sep 17, 2021

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 17, 2021

Weil0ng added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Sep 17, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 17, 2021

Weil0ng mentioned this pull request Sep 17, 2021

Implementation of CiliumEndpointBatch (CFP-2021-08) #16931

Closed

15 tasks

Weil0ng mentioned this pull request Oct 18, 2021

Identity based batching race condition Issues #17520

Merged

Weil0ng mentioned this pull request Oct 26, 2021

Enable CiliumEndpointSlice feature #17658

Merged

Weil0ng force-pushed the cesdoc branch 2 times, most recently from fd3b4f4 to 629e77c Compare November 3, 2021 23:04

pchaigno added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Nov 5, 2021

Weil0ng force-pushed the cesdoc branch 2 times, most recently from cab7136 to d13c98f Compare November 9, 2021 00:39

Weil0ng requested a review from aanm November 9, 2021 00:54

maintainer-s-little-helper bot assigned aanm Nov 9, 2021

Weil0ng requested a review from joestringer November 9, 2021 00:54

maintainer-s-little-helper bot assigned joestringer Nov 9, 2021

Weil0ng marked this pull request as ready for review November 9, 2021 00:54

Weil0ng requested a review from a team as a code owner November 9, 2021 00:54

Weil0ng requested a review from a team November 9, 2021 00:54

pchaigno added the dont-merge/blocked Another PR must be merged before this one. label Nov 9, 2021

joestringer requested changes Nov 9, 2021

View reviewed changes

Weil0ng force-pushed the cesdoc branch from d13c98f to a72b8ca Compare November 10, 2021 00:41

Weil0ng mentioned this pull request Nov 10, 2021

CES introspection #17850

Open

Weil0ng force-pushed the cesdoc branch from a72b8ca to faf9a87 Compare November 10, 2021 18:55

Weil0ng requested a review from joestringer November 10, 2021 18:55

joestringer reviewed Nov 10, 2021

View reviewed changes

joestringer requested changes Nov 10, 2021

View reviewed changes

Adds documentation for CiliumEndpointSlice

e22cfe4

Signed-off-by: Weilong Cui <cuiwl@google.com>

Weil0ng force-pushed the cesdoc branch from faf9a87 to e22cfe4 Compare November 10, 2021 23:18

Weil0ng requested a review from joestringer November 10, 2021 23:18

joestringer approved these changes Nov 10, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned joestringer Nov 10, 2021

Weil0ng removed the dont-merge/blocked Another PR must be merged before this one. label Nov 11, 2021

aanm approved these changes Nov 12, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned aanm Nov 12, 2021

aanm merged commit 0c02e32 into cilium:master Nov 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds concept documentation for CiliumEndpointSlice #17430

Adds concept documentation for CiliumEndpointSlice #17430

Weil0ng commented Sep 17, 2021 •

edited

pchaigno commented Nov 9, 2021

joestringer left a comment

joestringer Nov 9, 2021

Weil0ng Nov 10, 2021

Weil0ng Nov 10, 2021

joestringer Nov 10, 2021

Weil0ng Nov 10, 2021

joestringer Nov 10, 2021

joestringer Nov 9, 2021

Weil0ng Nov 10, 2021

joestringer Nov 9, 2021

Weil0ng Nov 10, 2021

joestringer Nov 9, 2021

joestringer Nov 9, 2021

Weil0ng Nov 10, 2021

joestringer Nov 9, 2021

Weil0ng Nov 10, 2021

Weil0ng commented Nov 10, 2021

joestringer Nov 10, 2021

Weil0ng Nov 10, 2021

joestringer left a comment

Weil0ng commented Nov 11, 2021

		During this period, new endpoints might experience longer delay before
		their identities will propagate to remote nodes.

		helm install cilium \|CHART_RELEASE\| \\
		--set enableCiliumEndpointSlice=true

Adds concept documentation for CiliumEndpointSlice #17430

Adds concept documentation for CiliumEndpointSlice #17430

Conversation

Weil0ng commented Sep 17, 2021 • edited

pchaigno commented Nov 9, 2021

joestringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Weil0ng commented Nov 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joestringer left a comment

Choose a reason for hiding this comment

Weil0ng commented Nov 11, 2021

Weil0ng commented Sep 17, 2021 •

edited