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

CSDS: add new config status ACKED #13121

Merged
merged 10 commits into from
Sep 18, 2020
Merged

CSDS: add new config status ACKED #13121

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e142072
Add new config status for CSDS: ACKED
lidizheng Sep 15, 2020
78c118c
Split the client-side config status into another enum
lidizheng Sep 16, 2020
ef87599
Enrich the documentation to describe expected config dump behavior
lidizheng Sep 16, 2020
8f2fb64
Simplify the new enum's naming and improve documentation
lidizheng Sep 16, 2020
15a4491
Update the service description to include getting configs from clients
lidizheng Sep 16, 2020
abd2351
Make formatter happy
lidizheng Sep 16, 2020
5fb88f0
Add UDPA oneof annotation
lidizheng Sep 16, 2020
87b889f
Import migrate.proto to use UDPA oneof
lidizheng Sep 16, 2020
0ba9254
Use oneof_promotion in v3 and real oneof in v4alpha
lidizheng Sep 17, 2020
f7f38af
Notes down the expected behavior for management server saw NACK
lidizheng Sep 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion api/envoy/service/status/v3/csds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,9 @@ enum ClientSideConfigStatus {
// Client received the config and replied with ACK.
CLIENT_ACKED = 2;

// Client received the config and replied with NACK.
// Client received the config and replied with NACK. Notably, the attached
// config dump is not the NACK version, but the most recent accepted one. If
// no config is accepted yet, the attached config dump should remain empty.
CLIENT_NACKED = 3;
}

Expand All @@ -88,7 +90,13 @@ message PerXdsConfig {
option (udpa.annotations.versioning).previous_message_type =
"envoy.service.status.v2.PerXdsConfig";

// Config status is generated by management servers.
ConfigStatus status = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably worth adding a comment here that status will be populated by xDS servers but client_side_config_status will be populated by xDS clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments added.

Copy link
Contributor

Choose a reason for hiding this comment

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

(It's too bad that we can't use a oneof here, but that would be a breaking change for the go proto API.)

Copy link
Member

Choose a reason for hiding this comment

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

Suggest adding (udpa.annotations.field_migrate).oneof_promotion annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Annotation added, TIL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@htuch Can you advice what should we use in v4alpha? The format suggests that the migration annotation is expected to be expanded into real oneof in v4alpha. But that will break backward compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine -- v4alpha will be a new major version (if it ever actually happens), so breaking changes are okay. This is actually the whole point of the annotation, to make sure this change happens when the next major version bump occurs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, but it looks like this didn't actually work for some reason. The annotation was carried forward to v4alpha instead of being turned into a oneof. Maybe the annotation has the wrong syntax or something?


// Client side config status is populated by xDS clients. Only one of the
// config status should present in one PerXdsConfig message. No matter what
// the client side config status is, xDS clients should always dump the most
// recent accepted xDS config.
ClientSideConfigStatus client_side_config_status = 7;

oneof per_xds_config {
Expand Down
10 changes: 9 additions & 1 deletion api/envoy/service/status/v4alpha/csds.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion generated_api_shadow/envoy/service/status/v3/csds.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 10 additions & 2 deletions generated_api_shadow/envoy/service/status/v4alpha/csds.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.