Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.

Handle pod metadata with missing containerStatuses. #49

Merged
igorpeshansky merged 4 commits into from
Jan 12, 2018
Merged

Handle pod metadata with missing containerStatuses. #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e661cde
Use container spec to find the status, since the spec is always present.
igorpeshansky Jan 12, 2018
744963c
Get rid of the unused container_id.
igorpeshansky Jan 12, 2018
a1f0b09
Make container_status optional.
igorpeshansky Jan 12, 2018
cbb874e
Log container status as well.
igorpeshansky Jan 12, 2018
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
74 changes: 35 additions & 39 deletions src/kubernetes.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,6 @@ constexpr const char kNodeSelectorPrefix[] = "?fieldSelector=spec.nodeName%3D";

constexpr const char kWatchParam[] = "watch=true";

constexpr const char kDockerIdPrefix[] = "docker://";

constexpr const char kServiceAccountDirectory[] =
"/var/run/secrets/kubernetes.io/serviceaccount";

Expand Down Expand Up @@ -249,8 +247,8 @@ MetadataUpdater::ResourceMetadata KubernetesReader::GetPodMetadata(
}

MetadataUpdater::ResourceMetadata KubernetesReader::GetContainerMetadata(
const json::Object* pod, const json::Object* container_status,
const json::Object* container_spec, json::value associations,
const json::Object* pod, const json::Object* container_spec,
const json::Object* container_status, json::value associations,
Timestamp collected_at) const throw(json::Exception) {
const std::string zone = environment_.InstanceZone();
const std::string cluster_name = environment_.KubernetesClusterName();
Expand All @@ -273,17 +271,6 @@ MetadataUpdater::ResourceMetadata KubernetesReader::GetContainerMetadata(
const std::string node_name = spec->Get<json::String>("nodeName");

const std::string container_name = container_spec->Get<json::String>("name");
std::size_t docker_prefix_end = sizeof(kDockerIdPrefix) - 1;
const std::string docker_id =
container_status->Get<json::String>("containerID");
if (docker_id.compare(0, docker_prefix_end, kDockerIdPrefix) != 0) {
LOG(ERROR) << "ContainerID "
<< docker_id
<< " does not start with " << kDockerIdPrefix
<< " (" << docker_prefix_end << " chars)";
docker_prefix_end = 0;
}
const std::string container_id = docker_id.substr(docker_prefix_end);
// TODO: find is_deleted.
//const json::Object* state = container_status->Get<json::Object>("state");
bool is_deleted = false;
Expand All @@ -303,11 +290,16 @@ MetadataUpdater::ResourceMetadata KubernetesReader::GetContainerMetadata(
{"version", json::string(kKubernetesApiVersion)},
{"raw", container_spec->Clone()},
})},
{"status", json::object({
{"version", json::string(kKubernetesApiVersion)},
{"raw", container_status->Clone()},
})},
}));
if (container_status) {
blobs->emplace(std::make_pair(
"status",
json::object({
{"version", json::string(kKubernetesApiVersion)},
{"raw", container_status->Clone()},
})
));
}
if (labels) {
blobs->emplace(std::make_pair(
"labels",
Expand All @@ -324,17 +316,14 @@ MetadataUpdater::ResourceMetadata KubernetesReader::GetContainerMetadata(
LOG(INFO) << "Raw container metadata: " << *container_raw_metadata;
}

const std::string k8s_container_id = boost::algorithm::join(
std::vector<std::string>{kK8sContainerResourcePrefix, container_id},
kResourceTypeSeparator);
const std::string k8s_container_pod = boost::algorithm::join(
std::vector<std::string>{kK8sContainerResourcePrefix, pod_id, container_name},
kResourceTypeSeparator);
const std::string k8s_container_name = boost::algorithm::join(
std::vector<std::string>{kK8sContainerNameResourcePrefix, namespace_name, pod_name, container_name},
kResourceTypeSeparator);
return MetadataUpdater::ResourceMetadata(
std::vector<std::string>{k8s_container_id, k8s_container_pod, k8s_container_name},
std::vector<std::string>{k8s_container_pod, k8s_container_name},
k8s_container,
#ifdef ENABLE_KUBERNETES_METADATA
MetadataAgent::Metadata(kKubernetesApiVersion,
Expand Down Expand Up @@ -400,29 +389,36 @@ KubernetesReader::GetPodAndContainerMetadata(
const json::Array* container_statuses =
status->Get<json::Array>("containerStatuses");

// Move the container specs into a map from name to spec.
std::map<std::string, const json::Object*> container_spec_by_name;
for (const json::value& c_spec : *container_specs) {
const json::Object* container_spec = c_spec->As<json::Object>();
const std::string name = container_spec->Get<json::String>("name");
container_spec_by_name.emplace(name, container_spec);
// Move the container statuses into a map from name to status.
std::map<std::string, const json::Object*> container_status_by_name;
for (const json::value& c_status : *container_statuses) {
const json::Object* container_status = c_status->As<json::Object>();
const std::string name = container_status->Get<json::String>("name");
container_status_by_name.emplace(name, container_status);
}

for (const json::value& c_status : *container_statuses) {
for (const json::value& c_spec : *container_specs) {
if (config_.VerboseLogging()) {
LOG(INFO) << "Container: " << *c_status;
LOG(INFO) << "Container: " << *c_spec;
Copy link
Contributor

@dhrupadb dhrupadb Jan 12, 2018

Choose a reason for hiding this comment

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

Assuming the spec has a lot more in it than just the status -- might this make it too chatty?

Copy link
Contributor

@bmoyles0117 bmoyles0117 Jan 12, 2018

Choose a reason for hiding this comment

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

I agree with the comment, however, this is only being displayed if verbose logging is enabled, so I'm ok with keeping it.

Copy link
Contributor Author

@igorpeshansky igorpeshansky Jan 12, 2018

Choose a reason for hiding this comment

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

Heh, the status object isn't that small either. At some point the code logged both, so I figured I was just changing the order, but I guess the logging of the spec got lost.
Just pushed a commit that also logs the status. The config option says "verbose logging", so some chattiness should be expected, and it's helpful in debugging the processing of the incoming JSON.

}
const json::Object* container_status = c_status->As<json::Object>();
const std::string name = container_status->Get<json::String>("name");
auto spec_it = container_spec_by_name.find(name);
if (spec_it == container_spec_by_name.end()) {
LOG(ERROR) << "Internal error; spec not found for container " << name;
continue;
const json::Object* container_spec = c_spec->As<json::Object>();
const std::string name = container_spec->Get<json::String>("name");
auto status_it = container_status_by_name.find(name);
Copy link
Contributor

@bmoyles0117 bmoyles0117 Jan 12, 2018

Choose a reason for hiding this comment

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

What does "_it" mean here? "is there"?

Copy link
Contributor Author

@igorpeshansky igorpeshansky Jan 12, 2018

Choose a reason for hiding this comment

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

Short for "iterator". Fairly standard C++ convention.

Copy link
Contributor

@bmoyles0117 bmoyles0117 Jan 12, 2018

Choose a reason for hiding this comment

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

SG

const json::Object* container_status;
if (status_it == container_status_by_name.end()) {
container_status = nullptr;
if (config_.VerboseLogging()) {
LOG(INFO) << "No status for container " << name;
}
} else {
container_status = status_it->second;
if (config_.VerboseLogging()) {
LOG(INFO) << "Container status: " << *container_status;
}
}
const json::Object* container_spec = spec_it->second;
result.emplace_back(GetLegacyResource(pod, name));
result.emplace_back(
GetContainerMetadata(pod, container_status, container_spec,
GetContainerMetadata(pod, container_spec, container_status,
Copy link
Contributor

@bmoyles0117 bmoyles0117 Jan 12, 2018

Choose a reason for hiding this comment

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

Nit: generally not sure why you reversed the args here, but whatever the order ends up being, it'd be nice if it lined up with the definitions above. That way the code reads consistently throughout. Either reverse the order of the params, or the order of the definitions.

Copy link
Contributor Author

@igorpeshansky igorpeshansky Jan 12, 2018

Choose a reason for hiding this comment

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

It does, though -- container_spec is now declared in the loop header, and container_status is retrieved by name later. What am I missing?

Copy link
Contributor

@bmoyles0117 bmoyles0117 Jan 12, 2018

Choose a reason for hiding this comment

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

I didn't zoom out and realize this is being invoked inside of the loop. This looks good now that I better understand.

associations->Clone(), collected_at));
}

Expand Down
4 changes: 2 additions & 2 deletions src/kubernetes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ class KubernetesReader {
const throw(json::Exception);
// Given a pod object and container info, return the container metadata.
MetadataUpdater::ResourceMetadata GetContainerMetadata(
const json::Object* pod, const json::Object* container_status,
const json::Object* container_spec, json::value associations,
const json::Object* pod, const json::Object* container_spec,
const json::Object* container_status, json::value associations,
Timestamp collected_at) const throw(json::Exception);
// Given a pod object and container name, return the legacy resource.
// The returned "metadata" field will be Metadata::IGNORED.
Expand Down