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 2 commits
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
19 changes: 18 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 @@ -34,7 +34,7 @@ service ClientStatusDiscoveryService {
}
}

// Status of a config.
// Status of a config from a management server view.
enum ConfigStatus {
// Status info is not available/unknown.
UNKNOWN = 0;
Expand All @@ -53,6 +53,22 @@ enum ConfigStatus {
ERROR = 4;
}

// Config status from a client-side view.
enum ClientSideConfigStatus {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest calling this ClientConfigStatus.

Copy link
Contributor Author

@lidizheng lidizheng Sep 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Changed to ClientConfigStatus.

In this proto definition, there is the ClientConfig message. It might be a bit confusing to readers that why ClientConfigStatus is not the status for the ClientConfig message, but the client-side view of config status. On the other hand, the ClientSideConfigStatus is not much better.

Better naming is still welcomed. Picking ClientConfigStatus since it is simpler.

// Config status is not available/unknown.
CLIENT_UNKNOWN = 0;

// Client requested the config but hasn't received any config from management
// server yet.
CLIENT_REQUESTED = 1;

// Client received the config and replied with ACK.
CLIENT_ACKED = 2;

// Client received the config and replied with NACK.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's explicitly document that the resource content that is returned in this case will be the most recent version accepted, if any, not the version that was NACKed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documented the expected config dump behavior here and in the PerXdsConfig message.

CLIENT_NACKED = 3;
}

// Request for client status of clients identified by a list of NodeMatchers.
message ClientStatusRequest {
option (udpa.annotations.versioning).previous_message_type =
Expand All @@ -73,6 +89,7 @@ message PerXdsConfig {
"envoy.service.status.v2.PerXdsConfig";

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?

ClientSideConfigStatus client_side_config_status = 7;

oneof per_xds_config {
admin.v3.ListenersConfigDump listener_config = 2;
Expand Down
19 changes: 18 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.

19 changes: 18 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.

19 changes: 18 additions & 1 deletion 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.