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

CLI doesn't complain when given unknown deployments, pods, or paths #49

Closed
wmorgan opened this issue Dec 14, 2017 · 2 comments
Closed
Assignees
Labels
area/cli priority/P2 Nice-to-have for Release
Milestone

Comments

@wmorgan
Copy link
Member

wmorgan commented Dec 14, 2017

For tap and stat, and maybe other commands, specifying a non-existing deployment or pod doesn't result in any kind of error message.

E.g. stat:

william:~/devel/conduit$ conduit stat deploy default/hello
NAME            REQUEST_RATE   SUCCESS_RATE   P50_LATENCY   P99_LATENCY
default/hello         2.5rps         96.00%         352ms         875ms
william:~/devel/conduit$ conduit stat deploy default/hello2
NAME   REQUEST_RATE   SUCCESS_RATE   P50_LATENCY   P99_LATENCY

and tap:

william:~/devel/conduit$ conduit tap deploy default/hello
[172.17.0.3:53280 -> 172.17.0.11:80]
HTTP Request
Stream ID: (0, 263)
HTTP POST world.default.svc.cluster.local/helloworld.World/Greeting
...
^C
william:~/devel/conduit$ conduit tap deploy hello
^C

This isn't too bad on its surface, but a) it means that e.g. forgetting to put the namespace for tap deploy means that the command just hangs forever (this is an easy mistake, and we had a user confused about this in #conduit today), and b) it doesn't match how kubectl works, e.g.

william:~/devel/conduit$ kubectl get -n conduit deploy/prometheus
NAME         DESIRED   CURRENT   UP-TO-DATE   AVAILABLE   AGE
prometheus   1         1         1            1           7d
william:~/devel/conduit$ kubectl get -n conduit deploy/prometheus2
Error from server (NotFound): deployments.extensions "prometheus2" not found

I think these commands should return an error if the specifying pod, deployment, or path (maybe?) is not found.

@pcalcado pcalcado self-assigned this Dec 15, 2017
pcalcado added a commit that referenced this issue Jan 25, 2018
Previously, running `$conduit tap` would return a `Unexpected EOF` error when the server wasn't available. This was due to a few problems with the way we were handling errors all the way down the tap server. This change fixes that and cleans some of the protobuf-over-HTTP code.

- first step towards #49
- closes #106
@olix0r olix0r added the priority/P2 Nice-to-have for Release label Mar 13, 2018
@olix0r olix0r added this to the 0.4.0 milestone Mar 13, 2018
@siggy
Copy link
Member

siggy commented Apr 16, 2018

This issue is two-thirds resolved.

tap

will be fixed in #827

conduit tap continues to exhibit this behavior.

$ conduit tap deploy foo
...

stat

stat was fixed in #760:

$ conduit stat deploy emoji
Error: error calling stat with request: deployment.apps "emoji" not found
Usage:
  conduit stat [flags] RESOURCETYPE [RESOURCENAME]

get

conduit get no longer supports resources as input:

$ conduit get emojivoto/web-b8f8786b8-49d9j
Error: invalid resource type emojivoto/web-b8f8786b8-49d9j, only pods are allowed as resource types
Usage:
  conduit get [flags] pods

@olix0r olix0r modified the milestones: 0.4.0, 0.4.1 Apr 16, 2018
@siggy siggy self-assigned this Apr 19, 2018
siggy added a commit that referenced this issue Apr 20, 2018
The TapByResource endpoint was previously a stub.

Implement end-to-end tapByResource functionality, with support for
specifying any kubernetes resource(s) as target and destination.

Fixes #803, #49

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit that referenced this issue Apr 22, 2018
The TapByResource endpoint was previously a stub.

Implement end-to-end tapByResource functionality, with support for
specifying any kubernetes resource(s) as target and destination.

Fixes #803, #49

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit that referenced this issue Apr 23, 2018
The TapByResource endpoint was previously a stub.

Implement end-to-end tapByResource functionality, with support for
specifying any kubernetes resource(s) as target and destination.

Fixes #803, #49

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit that referenced this issue Apr 23, 2018
The TapByResource endpoint was previously a stub.

Implement end-to-end tapByResource functionality, with support for
specifying any kubernetes resource(s) as target and destination.

Fixes #803, #49

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
@siggy
Copy link
Member

siggy commented Apr 23, 2018

Resolved in #827.

@siggy siggy closed this as completed Apr 23, 2018
khappucino pushed a commit to Nordstrom/linkerd2 that referenced this issue Mar 5, 2019
Metric eviction was introduced to protect against the situation where,
over time, the proxy addresses an effectively unbounded number of
endpoints. Metrics with endpoint-specific dimensions must be removed
over time to prevent a form of memory leak.

Eviction is implemented for transport stats. However, transport stats do
not yet contain per-endpoint dimensions. The eviction logic is
superfluous in this case, since transport metrics are bounded and small.

In preparation of changes to transport telemetry, this disables tracking
of transport metrics for eviction. This logic will be restored when we
support per-endpoint transport metrics.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/cli priority/P2 Nice-to-have for Release
Projects
None yet
Development

No branches or pull requests

4 participants