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

[receiver/awscontainerinsightsreceiver] Pod Detection changes for K8's in containerd runtime #11666

Merged
merged 14 commits into from
Aug 3, 2022

Conversation

vasireddy99
Copy link
Contributor

@vasireddy99 vasireddy99 commented Jun 27, 2022

Description:
dockershim will be deprecated in the latest kubernetes release. Due to this following changes in PR are required when using containerd runtime

  • k8s cluster using containerd runtime will not be able to get the pod metrics as containerd socket is not mounted
    pod detection logic relies on pause container with name 'POD`, which is only the case for docker
  • Define a new constant TypeInfraContainer and remove most usage on POD container name.
    update all the manifest to mount containerd

EKS Announcement: Amazon EKS is ending support for dockershim

Link to tracking Issue:

Documentation:

  • Read me changes to example deployment template is modified
  • Logged in CHANGELOG.md

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -303,6 +306,9 @@ spec:
- name: varlibdocker
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to retain this even if the path doesn't exist on the host?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is safe to retain the path, reference from the container-d supported version of yaml.
and in viceversa tested this from local on EKS 1.22 version with this yaml file
image

@@ -117,7 +117,9 @@ func processContainer(info *cInfo.ContainerInfo, mInfo extractors.CPUMemInfoProv
namespace := info.Spec.Labels[namespaceLabel]
podName := info.Spec.Labels[podNameLabel]
podID := info.Spec.Labels[podIDLabel]
if containerName == "" || namespace == "" || podName == "" {
// NOTE: containerName can be empty for pause container on containerd
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to validate that this is not a pause container before entering this branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like there actually isn't a way to do this validation. 😕

@vasireddy99 vasireddy99 changed the title [awscontainerinsightsreceiver] Pod Detection changes for K8's in containerd runtime [receiver/awscontainerinsightsreceiver] Pod Detection changes for K8's in containerd runtime Jul 7, 2022
@@ -0,0 +1,16 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: breaking
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a breaking change, is it? It doesn't change exported APIs at all. It seems more like an enhancement to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, thanks for the suggestion

@Aneurysm9 Aneurysm9 added the ready to merge Code review completed; ready to merge by maintainers label Aug 3, 2022
@dmitryax dmitryax merged commit ba0517b into open-telemetry:main Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants