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.

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

API: Unready Targets Dump #12554

Merged
merged 78 commits into from
Sep 10, 2020
Merged

Conversation

ping-sun
Copy link
Contributor

@ping-sun ping-sun commented Aug 7, 2020

Description: Taking advantage of the new feature introduced in #12035, which introduced quick visibility for init managers to check unready targets, this pull request adds protobuf message for unready targets and enables admin to dump configs of unready targets. An example of config dump for listeners’ unready targets is given in this pull request.

Introduce InitDumpHandler with handlerInitDump method to help dump information of unready targets.
Add dumpUnreadyTargets function for init::manager.

The new proto introduced:

// Dumps of unready targets of envoy init managers. Envoy's admin fills this message with init managers,
// which provides the information of their unready targets.
message UnreadyTargetsDumps {
  // Message of unready targets information of an init manager.
  message UnreadyTargetsDump {
    // Name of the init manager. Example: "init_manager_xxx".
    string name = 1;

    // Names of unready targets of the init manager. Example: "target_xxx".
    repeated string target_names = 2;
  }

  // The dumps of unready targets of all init managers.
  repeated UnreadyTargetsDump unready_targets_dumps = 1;
}

Signed-off-by: ping sun <pingsun@google.com>
Signed-off-by: ping sun <pingsun@google.com>
Signed-off-by: ping sun <pingsun@google.com>
…view; targetHandle->name(); remove target_names_ vector

Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: ping sun <pingsun@google.com>
Signed-off-by: ping sun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
…II/envoy into init-manager-query-unready-targets
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
@@ -381,3 +381,18 @@ message EndpointsConfigDump {
// The dynamically loaded endpoint configs.
repeated DynamicEndpointConfig dynamic_endpoint_configs = 3;
}

// Envoy's admin fills this message with init managers, which provides the information of their unready targets.
message UnreadyTargetsConfigDumpList {
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually configuration? It's definitely implied by configuration when interpreted by Envoy, but I'm wondering if it belongs on a different endpoint. @envoyproxy/api-shepherds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this actually configuration? It's definitely implied by configuration when interpreted by Envoy, but I'm wondering if it belongs on a different endpoint. @envoyproxy/api-shepherds

I suppose it does not belong to an actual endpoint. This is just a protobuf message designed to show the unready targets in configs.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's kind of weird to include as part of config dump, but I'll defer to what other @envoyproxy/api-shepherds reckon. @mattklein123 as well as he did the original config dump.

Copy link
Member

Choose a reason for hiding this comment

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

+1, can we spit this out somewhere else? This is not really config in the normal sense of the word. Maybe a new /init_manager or other endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it. I will move it somewhere else, and create a new init_dump.proto

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I'm late to this thread, I feel this should be part of config_dump, otherwise it will be hard to tell what config made the targets unready, querying two endpoints might end up in inconsistent (racy) dump.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a problem that applies to the admin endpoint in general? If we query /config_dump and /clusters, we might see inconsistent output. Same goes for /listeners. Would it make sense to try solve this through some mechanism for batching, e.g. an API endpoint, rather than trying to atomically group things?

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like /config_dump or the potential is an inspection endpoint.
The configs/targets are stale as soon as these init manager status are on the wire.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rst has been updated.

@ping-sun ping-sun requested a review from htuch September 3, 2020 04:47
Signed-off-by: pingsun <pingsun@google.com>
@ping-sun ping-sun dismissed stale reviews from lizan and lambdai via 8c8c1e4 September 3, 2020 22:40
Signed-off-by: pingsun <pingsun@google.com>
@ping-sun
Copy link
Contributor Author

ping-sun commented Sep 4, 2020

I've removed unready targets from config dumps and introduced a new init_dump to display that information. @mattklein123 @htuch

@lambdai
Copy link
Contributor

lambdai commented Sep 8, 2020

Could you update the commit message reflecting the new init dump endpoint ?
And also in docs/root/version_history/current.rst

@ping-sun
Copy link
Contributor Author

ping-sun commented Sep 8, 2020

Could you update the commit message reflecting the new init dump endpoint ?
And also in docs/root/version_history/current.rst

Sure, Updated

Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
Signed-off-by: pingsun <pingsun@google.com>
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.

/lgtm api

@repokitteh-read-only repokitteh-read-only bot removed the api label Sep 9, 2020
@htuch
Copy link
Member

htuch commented Sep 9, 2020

@lizan @dio will leave the rest of the PR for you folks, API LGTM.

@ping-sun ping-sun requested a review from lizan September 9, 2020 16:14
@ping-sun
Copy link
Contributor Author

ping-sun commented Sep 9, 2020

I noticed that config_dump has been sealed in a unique class ConfigDumpHandler, so I followed this behavior and moved my init_dump in a new lib as well.

Signed-off-by: pingsun <pingsun@google.com>
@ping-sun ping-sun changed the title API: Unready targets config dump API: Unready Targets Dump Sep 9, 2020
Signed-off-by: pingsun <pingsun@google.com>
@lizan lizan merged commit 8aef763 into envoyproxy:master Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants