Skip to content
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.

Sign up for GitHub

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

Jump to bottom

docs: clarify xds docs with warming information #6236

Merged
merged 4 commits into from
Mar 12, 2019
Merged

docs: clarify xds docs with warming information #6236

merged 4 commits into from
Mar 12, 2019

Conversation

ramaraochavali
Copy link
Contributor

Signed-off-by: Rama Chavali rama.rao@salesforce.com
Description: As a follow-up to #6151 , this PR updates xds docs.
Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

@ramaraochavali
update xds docs with warming
0624a09 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Contributor Author

@mattklein123 @htuch PTAL

@ramaraochavali
format and minor correction
2e71cde 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
venilnoronha
venilnoronha reviewed Mar 10, 2019
View reviewed changes
api/XDS_PROTOCOL.md Outdated
go through `warming` before they can serve requests. This process happens both during
[`Envoy initialization`](https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/init.html#initialization)
and when the `Cluster` or `Listener` is updated. Warming of `Cluster` is completed only when a
`ClusterLoadAssignment` response is supplied by management server. Similarly warming of `Listener`
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/Similarly/Similarly,

api/XDS_PROTOCOL.md Outdated
[`Envoy initialization`](https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/init.html#initialization)
and when the `Cluster` or `Listener` is updated. Warming of `Cluster` is completed only when a
`ClusterLoadAssignment` response is supplied by management server. Similarly warming of `Listener`
is completed only when a `RouteConfiguration` is suppled by management server if the listener
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove extra space after `RouteConfiguration`

api/XDS_PROTOCOL.md Outdated
and when the `Cluster` or `Listener` is updated. Warming of `Cluster` is completed only when a
`ClusterLoadAssignment` response is supplied by management server. Similarly warming of `Listener`
is completed only when a `RouteConfiguration` is suppled by management server if the listener
refers to an RDS configuration. Management Server is expected to provide the EDS/RDS updates during
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/Server/server

api/XDS_PROTOCOL.md Outdated
is completed only when a `RouteConfiguration` is suppled by management server if the listener
refers to an RDS configuration. Management Server is expected to provide the EDS/RDS updates during
warming. If management server does not provide EDS/RDS information, Envoy will not initialize
it self during initialization phase and the updates sent via CDS/LDS will not take effect till
Copy link
Member

Choose a reason for hiding this comment

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

s/it self/itself

@ramaraochavali
review comments
cd8ce60 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6236 (comment) was created by @ramaraochavali.

see: more, trace.

@ramaraochavali
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6236 (comment) was created by @ramaraochavali.

see: more, trace.

venilnoronha
venilnoronha previously approved these changes Mar 10, 2019
View reviewed changes
Copy link
Member

@venilnoronha venilnoronha left a comment

Choose a reason for hiding this comment

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

LGTM modulo nit. Thanks!

api/XDS_PROTOCOL.md Outdated
is completed only when a `RouteConfiguration` is suppled by management server if the listener
refers to an RDS configuration. Management server is expected to provide the EDS/RDS updates during
warming. If management server does not provide EDS/RDS responses, Envoy will not initialize
itself during initialization phase and the updates sent via CDS/LDS will not take effect till
Copy link
Member

Choose a reason for hiding this comment

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

Drop "during initialization phase"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was in two minds when I wrote this - but decided to add this for more clarity. LMK if you strong feel about removing, I can remove.

Copy link
Member

Choose a reason for hiding this comment

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

Up to you. I don't feel too strongly.

mattklein123
mattklein123 reviewed Mar 11, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, will defer to @htuch on the full review.

As an aside, I think we should migrate this MD file into the sphinx docs where they will be more discoverable, we can ref link, etc. If people agree can we open an issue to track that?

htuch
htuch reviewed Mar 11, 2019
View reviewed changes
api/XDS_PROTOCOL.md Outdated
[`Envoy initialization`](https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/init.html#initialization)
and when the `Cluster` or `Listener` is updated. Warming of `Cluster` is completed only when a
`ClusterLoadAssignment` response is supplied by management server. Similarly, warming of `Listener`
is completed only when a `RouteConfiguration` is suppled by management server if the listener
Copy link
Member

Choose a reason for hiding this comment

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

Nit: s/suppled/supplied/

api/XDS_PROTOCOL.md Outdated
is completed only when a `RouteConfiguration` is suppled by management server if the listener
refers to an RDS configuration. Management server is expected to provide the EDS/RDS updates during
warming. If management server does not provide EDS/RDS responses, Envoy will not initialize
itself during initialization phase and the updates sent via CDS/LDS will not take effect till
Copy link
Member

Choose a reason for hiding this comment

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

Nit: "the initialization phase", s/till/untill/

@htuch htuch added the waiting label Mar 11, 2019
@ramaraochavali
review comments
a5fc137 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Contributor Author

@htuch addressed review comments. PTAL

htuch
htuch approved these changes Mar 12, 2019
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

@htuch htuch merged commit 06c47cc into envoyproxy:master Mar 12, 2019
@ramaraochavali ramaraochavali deleted the fix/xds_docs branch March 12, 2019 04:38
spenceral added a commit to spenceral/envoy that referenced this pull request Mar 20, 2019
@spenceral
Merge branch 'master' into addl-cb-stats
d29d7b1 
* master: (59 commits)
  http fault: add response rate limit injection (envoyproxy#6267)
  xds: introduce initial_fetch_timeout option to limit initialization time (envoyproxy#6048)
  test: fix cpuset-threads tests (envoyproxy#6278)
  server: add an API for registering for notifications for server instance life… (envoyproxy#6254)
  remove remains of TestBase (envoyproxy#6286)
  dubbo_proxy: Implement the routing of Dubbo requests (envoyproxy#5973)
  Revert "stats: add new BoolIndicator stat type (envoyproxy#5813)" (envoyproxy#6280)
  runtime: codifying runtime guarded features (envoyproxy#6134)
  mysql_filter: fix integration test flakes (envoyproxy#6272)
  tls: update BoringSSL to debed9a4 (3683). (envoyproxy#6273)
  rewrite buffer implementation to eliminate evbuffer dependency (envoyproxy#5441)
  Remove the dependency from TimeSystem to libevent by using the Event::Scheduler abstraction as a delegate. (envoyproxy#6240)
  fuzz: fix use of literal in default initialization. (envoyproxy#6268)
  http: add HCM functionality required for rate limiting (envoyproxy#6242)
  Disable mysql_integration_test until it is deflaked. (envoyproxy#6250)
  test: use ipv6_only IPv6 addresses in custom cluster integration tests. (envoyproxy#6260)
  tracing: If parent span is propagated with empty string, it causes th… (envoyproxy#6263)
  upstream: fix oss-fuzz issue envoyproxy#11095. (envoyproxy#6220)
  Wire up panic mode subset to receive updates (envoyproxy#6221)
  docs: clarify xds docs with warming information (envoyproxy#6236)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 left review comments

@venilnoronha venilnoronha venilnoronha left review comments

@htuch htuch htuch approved these changes

Assignees

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ramaraochavali @venilnoronha @mattklein123 @htuch