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
9 changes: 7 additions & 2 deletions api/envoy/service/status/v2/csds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,20 @@ enum ConfigStatus {
// Management server has sent the config to client and received ACK.
SYNCED = 1;

// Config is not sent.
// Config is not sent from the management server; or the client haven't
// received the requested config.
NOT_SENT = 2;

// Management server has sent the config to client but hasn’t received
// ACK/NACK.
STALE = 3;

// Management server has sent the config to client but received NACK.
// Management server has sent the config to client but received NACK; or the
// client has received the config but replied with NACK.
ERROR = 4;

// The client has received the config and repied with ACK.
ACKED = 5;
}

// Request for client status of clients identified by a list of NodeMatchers.
Expand Down
9 changes: 7 additions & 2 deletions api/envoy/service/status/v3/csds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,20 @@ enum ConfigStatus {
// Management server has sent the config to client and received ACK.
SYNCED = 1;

// Config is not sent.
// Config is not sent from the management server; or the client haven't
// received the requested config.
NOT_SENT = 2;

// Management server has sent the config to client but hasn’t received
// ACK/NACK.
STALE = 3;

// Management server has sent the config to client but received NACK.
// Management server has sent the config to client but received NACK; or the
// client has received the config but replied with NACK.
ERROR = 4;

// The client has received the config and repied with ACK.
Copy link
Member

Choose a reason for hiding this comment

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

I'm almost thinking we want separate status enum messages or very explicit field naming to help make clear which one (client vs. server status) we're talking about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, seeing these comments, I guess I do think that having a separate set of enums would be clearer. We can call them something like CLIENT_REQUESTED, CLIENT_ACKED, and CLIENT_NACKED.

We can either add these as a new values in this existing enum, or we can add a whole separate enum field for use in 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.

Adding a new proto enum struct named ClientSideStatusConfig. It's a bit verbose, but I think it needs to have a very clear naming.

ACKED = 5;
}

// Request for client status of clients identified by a list of NodeMatchers.
Expand Down
9 changes: 7 additions & 2 deletions 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.

9 changes: 7 additions & 2 deletions generated_api_shadow/envoy/service/status/v2/csds.proto
View file
Open in desktop

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

9 changes: 7 additions & 2 deletions 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.

9 changes: 7 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.