Update xDS docs to clarify ordering #206

rshriram · 2017-10-25T14:43:39Z

No description provided.

Signed-off-by: Shriram Rajagopalan <[email protected]>

htuch · 2017-10-25T14:47:42Z

XDS_PROTOCOL.md

+* CDS updates (if any) must always be pushed first.
+* EDS updates (if any) must arrive after CDS updates for the respective clusters.
+* LDS updates must arrive after corresponding CDS/EDS updates.
+* RDS updates related to the newly added listeners must arrive in the end.


Should explain the "break" bit afterwards, where you clean up the old clusters (for example).

htuch · 2017-10-25T14:48:38Z

XDS_PROTOCOL.md

+* RDS updates related to the newly added listeners must arrive in the end.
+
+xDS updates can be pushed independently if no new clusters/routes/listeners
+are added.


or if it's acceptable to temporarily drop traffic during updates (this is after all the whole point of the eventual consistency model, some folks don't mind dropping traffic or inconsistent routing across nodes if it makes management simpler).

htuch · 2017-10-25T14:58:55Z

XDS_PROTOCOL.md

+* LDS updates must arrive after corresponding CDS/EDS updates.
+* RDS updates related to the newly added listeners must arrive in the end.
+
+xDS updates can be pushed independently if no new clusters/routes/listeners


This is too strong and not accurate I think. You can add a route that refers to existing clusters, for example, with a completely independent RDS update. Also, if you modify a cluster, it will need to go out and fetch its corresponding EDS update, see https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/envoy-users/jn3VJJBGoPo/Z_EpNSk2BgAJ. We probably need CDS/EDS warming to be implemented to make updates safe from dropping traffic (otherwise, you probably need to do a full RDS/CDS/EDS update sequence with new cluster name to avoid drops).

should i remove it altogether?

Signed-off-by: Shriram Rajagopalan <[email protected]>

mattklein123 · 2017-10-25T18:08:25Z

ci/build_setup.sh

-  exit 1
-fi
+mkdir -p ${BUILD_DIR}
+# if [[ ! -d "${BUILD_DIR}" ]]


Please just delete this vs. comment.

htuch · 2017-10-25T18:15:15Z

XDS_PROTOCOL.md

+* EDS updates (if any) must arrive after CDS updates for the respective clusters.
+* LDS updates must arrive after corresponding CDS/EDS updates.
+* RDS updates related to the newly added listeners must arrive in the end.
+* Stale CDS clusters (ones no longer being referenced) can then be removed.


.. and also related EDS.

Signed-off-by: Shriram Rajagopalan <[email protected]>

mattklein123 · 2017-10-25T20:02:25Z

XDS_PROTOCOL.md

+  referenced) can then be removed.
+
+xDS updates can be pushed independently if no new clusters/routes/listeners
+are added or or if it's acceptable to temporarily drop traffic during updates.


typo "or or"

Signed-off-by: Shriram Rajagopalan <[email protected]>

htuch · 2017-10-26T03:00:44Z

XDS_PROTOCOL.md

-* CDS/EDS resources corresponding to routes in LDS/RDS should be available at
-  update.
+In general, to avoid traffic drop, sequencing of updates should follow a
+`make before break` model, wherein


I wouldn't use verbatim quotes here, but instead MD emphasis. Not worth changing unless you need another revision though.

htuch · 2017-10-26T03:02:03Z

XDS_PROTOCOL.md

+* Stale CDS clusters and related EDS endpoints (ones no longer being
+  referenced) can then be removed.
+
+xDS updates can be pushed independently if no new clusters/routes/listeners


My previous comment on this wasn't reflected in the text here; there are some nuances that should be communicated that were there, in particular around the lack of CDS/EDS warming and the safety of independent updates for new clusters/routes that have no relationship with existing ones.

rshriram · 2017-10-26T03:15:46Z

Is there a rough text that I can use as base?

mattklein123 · 2017-10-26T15:26:08Z

@rshriram I think @htuch is looking for a short description around the fact that a) we do warm listeners (https://www.envoyproxy.io/envoy/configuration/listeners/lds), but we don't currently warm clusters (envoyproxy/envoy#1930). You should be able to craft a short statement from both of those and/or link. It's unclear to me how high priority warming CDS actually is. I think it's actually pretty uncommon use case that people update clusters. Add/remove is much more common.

Signed-off-by: Shriram Rajagopalan <[email protected]>

rshriram · 2017-10-26T16:13:04Z

I added some text. PTAL.

mattklein123

LGTM, thanks.

Shriram Rajagopalan added 30 commits October 13, 2017 16:14

Adding missing filters

566e122

Signed-off-by: Shriram Rajagopalan <[email protected]>

adding tcp fault to bazel

6aed0e5

Signed-off-by: Shriram Rajagopalan <[email protected]>

review feedback

ff6eeea

Signed-off-by: Shriram Rajagopalan <[email protected]>

nit

523a11a

Signed-off-by: Shriram Rajagopalan <[email protected]>

syncing with docs

a00975f

Signed-off-by: Shriram Rajagopalan <[email protected]>

nit

867aac0

Signed-off-by: Shriram Rajagopalan <[email protected]>

cleanups

a729bc1

Signed-off-by: Shriram Rajagopalan <[email protected]>

trying out a longer import

9456f27

Signed-off-by: Shriram Rajagopalan <[email protected]>

Merge remote-tracking branch 'upstream/master'

6b7220f

Signed-off-by: Shriram Rajagopalan <[email protected]>

new filters

5e0d5c2

Signed-off-by: Shriram Rajagopalan <[email protected]>

rate limit and health check filters

3819e57

Signed-off-by: Shriram Rajagopalan <[email protected]>

shuffling protos

1c0144a

Signed-off-by: Shriram Rajagopalan <[email protected]>

fix BUILD

e0dd931

Signed-off-by: Shriram Rajagopalan <[email protected]>

fix BUILD 2

b867cc5

Signed-off-by: Shriram Rajagopalan <[email protected]>

feedback

e6f8369

Signed-off-by: Shriram Rajagopalan <[email protected]>

remove optional percent

f331fa8

Signed-off-by: Shriram Rajagopalan <[email protected]>

use wrappers

889628a

Signed-off-by: Shriram Rajagopalan <[email protected]>

BUILD

8638cbb

Signed-off-by: Shriram Rajagopalan <[email protected]>

fix

8cb7a69

Signed-off-by: Shriram Rajagopalan <[email protected]>

feedback

7502b0c

Signed-off-by: Shriram Rajagopalan <[email protected]>

moving fault to http

81186d7

Signed-off-by: Shriram Rajagopalan <[email protected]>

stray build

95286bb

Signed-off-by: Shriram Rajagopalan <[email protected]>

beautify bazel build

7ba311c

Signed-off-by: Shriram Rajagopalan <[email protected]>

REQUIRED fields

3412880

Signed-off-by: Shriram Rajagopalan <[email protected]>

Merge remote-tracking branch 'upstream/master'

3d0bfda

Signed-off-by: Shriram Rajagopalan <[email protected]>

bad commit

f01ef4f

Signed-off-by: Shriram Rajagopalan <[email protected]>

fix

24da3eb

Signed-off-by: Shriram Rajagopalan <[email protected]>

fix

3abb345

Signed-off-by: Shriram Rajagopalan <[email protected]>

bad commit

c7784fb

Signed-off-by: Shriram Rajagopalan <[email protected]>

fix

77ce4e8

Signed-off-by: Shriram Rajagopalan <[email protected]>

Shriram Rajagopalan added 6 commits October 24, 2017 18:28

new option to route

c3665d4

Signed-off-by: Shriram Rajagopalan <[email protected]>

fix cross package ref

74d96b6

Signed-off-by: Shriram Rajagopalan <[email protected]>

nits

c50c5e9

Signed-off-by: Shriram Rajagopalan <[email protected]>

circle ci

69c9237

Signed-off-by: Shriram Rajagopalan <[email protected]>

Merge remote-tracking branch 'upstream/master'

e86b9e5

Signed-off-by: Shriram Rajagopalan <[email protected]>

updating docs

b5e4eea

Signed-off-by: Shriram Rajagopalan <[email protected]>

htuch reviewed Oct 25, 2017

View reviewed changes

Shriram Rajagopalan added 2 commits October 25, 2017 13:14

Merge remote-tracking branch 'upstream/master'

1f13aeb

Signed-off-by: Shriram Rajagopalan <[email protected]>

fix 1

91bc65f

Signed-off-by: Shriram Rajagopalan <[email protected]>

rshriram closed this Oct 25, 2017

rshriram reopened this Oct 25, 2017

Shriram Rajagopalan added 3 commits October 25, 2017 13:30

revert

c22f482

Signed-off-by: Shriram Rajagopalan <[email protected]>

remove travis

1a391c4

Signed-off-by: Shriram Rajagopalan <[email protected]>

nits

bc04f80

Signed-off-by: Shriram Rajagopalan <[email protected]>

mattklein123 reviewed Oct 25, 2017

View reviewed changes

ci/build_setup.sh Outdated

exit 1

fi

mkdir -p ${BUILD_DIR}

# if [[ ! -d "${BUILD_DIR}" ]]

Copy link

Member

mattklein123 Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please just delete this vs. comment.

htuch reviewed Oct 25, 2017

View reviewed changes

nits

5710cec

Signed-off-by: Shriram Rajagopalan <[email protected]>

mattklein123 reviewed Oct 25, 2017

View reviewed changes

typo

6f44ef7

Signed-off-by: Shriram Rajagopalan <[email protected]>

htuch reviewed Oct 26, 2017

View reviewed changes

clarifications

2c138d9

Signed-off-by: Shriram Rajagopalan <[email protected]>

mattklein123 approved these changes Oct 26, 2017

View reviewed changes

htuch approved these changes Oct 26, 2017

View reviewed changes

htuch merged commit b085af2 into envoyproxy:master Oct 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update xDS docs to clarify ordering #206

Update xDS docs to clarify ordering #206

rshriram commented Oct 25, 2017

htuch Oct 25, 2017

htuch Oct 25, 2017

htuch Oct 25, 2017

rshriram Oct 25, 2017

mattklein123 Oct 25, 2017

htuch Oct 25, 2017

mattklein123 Oct 25, 2017

htuch Oct 26, 2017

htuch Oct 26, 2017

rshriram commented Oct 26, 2017

mattklein123 commented Oct 26, 2017

rshriram commented Oct 26, 2017

mattklein123 left a comment

Update xDS docs to clarify ordering #206

Update xDS docs to clarify ordering #206

Conversation

rshriram commented Oct 25, 2017

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

rshriram commented Oct 26, 2017

mattklein123 commented Oct 26, 2017

rshriram commented Oct 26, 2017

mattklein123 left a comment

Choose a reason for hiding this comment