Fix and cleanup container metrics #293
Conversation
|
@abhinandanpb |
| cntr, err := c.containerStore.Get(containerID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to find container %q: %v", containerID, err) | ||
| return nil, fmt.Errorf("failed to find container: %v", err) |
There was a problem hiding this comment.
Why remove containerID? We will have not sufficient info to debug when user input a not exist containerID.
There was a problem hiding this comment.
The assumption is that the caller always know what id they are using. We only include information which caller doesn't know in the error message.
pkg/server/container_stats.go
Outdated
| @@ -32,19 +32,22 @@ func (c *criContainerdService) ContainerStats(ctx context.Context, in *runtime.C | |||
| return nil, fmt.Errorf("invalid container stats request") | |||
There was a problem hiding this comment.
We can remove the below code . The "c.containerStore.Get" can catch it.
if in.GetContainerId() == "" {
return nil, fmt.Errorf("invalid container stats request")
}
pkg/server/container_stats.go
Outdated
| return nil, fmt.Errorf("failed to find container %q: %v", containerID, err) | ||
| return nil, fmt.Errorf("failed to find container: %v", err) | ||
| } | ||
| request := &tasks.MetricsRequest{Filters: []string{"id==" + containerID}} |
There was a problem hiding this comment.
We should use cntr.Metadata.ID not "containerID".
| resp, err := c.taskService.Metrics(ctx, request) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to fetch metrics for tasks: %v", err) | ||
| return nil, fmt.Errorf("failed to fetch metrics for task: %v", err) |
| return nil, fmt.Errorf("failed to fetch metrics for task: %v", err) | ||
| } | ||
| if len(resp.Metrics) != 1 { | ||
| return nil, fmt.Errorf("unexpected metrics response: %+v", resp.Metrics) |
| in *runtime.ListContainerStatsRequest, | ||
| ) (*runtime.ListContainerStatsResponse, error) { | ||
| request, candidateContainers, err := c.buildTaskMetricsRequest(in) | ||
| request, containers, err := c.buildTaskMetricsRequest(in) |
There was a problem hiding this comment.
Should we do truncindex here? There is conainerID in filter.
There was a problem hiding this comment.
Haven't looked at your PR yet. Will do after the v1.0.0-alpha.0 release, and we could discuss there. :)
pkg/server/container_stats_list.go
Outdated
| if r.Filter.GetId() != "" && c.ID != r.Filter.GetId() { | ||
| var containers []containerstore.Container | ||
| for _, cntr := range c.containerStore.List() { | ||
| if r.Filter.GetId() != "" && cntr.ID != r.Filter.GetId() { |
There was a problem hiding this comment.
I'd prefer to use GetFilter() as above.
abhi
left a comment
abhi
left a comment
There was a problem hiding this comment.
Lot cleaner. Thanks. LGTM. minor nit
pkg/server/container_stats.go
Outdated
| return nil, fmt.Errorf("failed to decode container metrics: %v", err) | ||
| cs, err := c.getContainerMetrics(cntr.Metadata, resp.Metrics[0]) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to generate container metrics: %v", err) |
There was a problem hiding this comment.
I would stick with decode. We are not technically generating :)
| return nil, fmt.Errorf("failed to fetch metrics for task: %v", err) | ||
| } | ||
| if len(resp.Metrics) != 1 { | ||
| return nil, fmt.Errorf("unexpected metrics response: %+v", resp.Metrics) |
| var cs runtime.ContainerStats | ||
| if err := c.getContainerMetrics(containerID, resp.Metrics[0], &cs); err != nil { | ||
| return nil, fmt.Errorf("failed to decode container metrics: %v", err) | ||
| cs, err := c.getContainerMetrics(cntr.Metadata, resp.Metrics[0]) |
Signed-off-by: Lantao Liu <[email protected]>
Random-Liu
force-pushed
the
cleanup-container-metrics
branch
from
622e799 to
97b6e82
Compare
|
LGTM |
4 participants
This PR:
ContainerStats;r.GetFilter == nil,GetFilteris a function;Signed-off-by: Lantao Liu [email protected]