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

[Metricbeat][Kubernetes Volume] Add pvc reference to distinguish ephemeral from persistent volumes #38839

Merged
merged 19 commits into from May 16, 2024

Conversation

herrBez
Copy link
Contributor

@herrBez herrBez commented Apr 10, 2024

Proposed commit message

[Metricbeat][Kubernetes Volume] Add pvc reference to distinguish ephemeral from persistent volumes

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • [ ]

How to test this PR locally

Create a PVC and mount the PVC as a volume in a pod, e.g. (on GKE):

---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: pvc-demo
spec:
  accessModes:
    - ReadWriteOnce
  resources:
    requests:
      storage: 1Gi
  storageClassName: standard-rwo
---
kind: Pod
apiVersion: v1
metadata:
  name: pod-demo
spec:
  volumes:
    - name: pvc-demo-vol
      persistentVolumeClaim:
       claimName: pvc-demo
  containers:
    - name: pod-demo
      image: nginx
      resources:
        limits:
          cpu: 10m
          memory: 80Mi
        requests:
          cpu: 10m
          memory: 80Mi
      ports:
        - containerPort: 80
          name: "http-server"
      volumeMounts:
        - mountPath: "/usr/share/nginx/html"
          name: pvc-demo-vol

Deploy metricbeat and check that there exist documents in kubernetes.volume with the field kubernetes.persistentvolumeclaim.name.

Alternatively execute the test.

Related issues

Use cases

The use-case is to being able to monitor persistent volumes and ignoring ephemeral volumes for threshold alerts.

As of now there is no way to distinguish persistent from ephemeral volumes. However, the endpoint called by the module $NODE_URL/summary/stats does actually report a pvcReference that allows to bind the pod's volume with a corresponding pvc.

Example Response:

...
 "volume": [
        {
          "time": "2024-04-09T17:34:17Z",
          "availableBytes": 31509590016,
          "capacityBytes": 31526391808,
          "usedBytes": 24576,
          "inodesFree": 1966069,
          "inodes": 1966080,
          "inodesUsed": 11,
          "name": "pvc-demo-vol",
          "pvcRef": {
            "name": "pvc-demo",
            "namespace": "default"
          }
        },
        {
          "time": "2024-04-09T17:34:17Z",
          "availableBytes": 83873792,
          "capacityBytes": 83886080,
          "usedBytes": 12288,
          "inodesFree": 502853,
          "inodes": 502862,
          "inodesUsed": 9,
          "name": "kube-api-access-h2cll"
        }
     ]
...

Screenshots

Logs

@herrBez herrBez requested a review from a team as a code owner April 10, 2024 15:45
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 10, 2024
Copy link
Contributor

mergify bot commented Apr 10, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @herrBez? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@herrBez herrBez added the Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team label Apr 10, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Apr 10, 2024
@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 10, 2024

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 101 min 10 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@herrBez
Copy link
Contributor Author

herrBez commented Apr 17, 2024

cc @elastic/obs-ds-hosted-services

@tetianakravchenko tetianakravchenko removed the request for review from gsantoro May 2, 2024 15:17
Copy link
Contributor

@tetianakravchenko tetianakravchenko left a comment

Choose a reason for hiding this comment

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

could you please also add a changelog recorf?

@@ -121,7 +121,11 @@
"inodesFree": 473551,
"inodes": 473560,
"inodesUsed": 9,
"name": "default-token-sg8x5"
"name": "default-token-sg8x5",
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please add a separate sample with the pvcRef to keep in the test file both examples - one for persistentvolumeclaim and one for the ephemeral volume?

@tetianakravchenko tetianakravchenko requested review from a team, gizas and MichaelKatsoulis and removed request for a team May 2, 2024 15:25
@tetianakravchenko
Copy link
Contributor

a new field also should be added in the fields file: https://github.com/elastic/beats/blob/main/metricbeat/module/kubernetes/volume/_meta/fields.yml

@herrBez
Copy link
Contributor Author

herrBez commented May 2, 2024

Hi,

I tried to address the raised point about the tests^^. I also tried specifying the field. What I am not sure about is that in the end I want to "reuse" kubernetes.persistentvolumeclaim.name which is part of the kubernetes.state_persistentvolumeclaims dataset. My doubt comes from the fact that we did not specify kubernetes.pod.name. WDYT?

@herrBez
Copy link
Contributor Author

herrBez commented May 3, 2024

Apparently, adding the field broke the pipeline with the following error:

[2024-05-02T16:39:21.927Z] E Exception: export command returned with an error: Error generating Index Pattern: field <kubernetes.persistentvolumeclaim.name> is duplicated, remove it or set 'overwrite: true', {Name:name Type:keyword Description:PVC name. Format: Fields:[] MultiFields:[] Enabled: Analyzer:{Name: Definition:} SearchAnalyzer:{Name: Definition:} Norms:false Dynamic:{Value:} Index: DocValues: CopyTo: IgnoreAbove:0 AliasPath: MigrationAlias:false Dimension: DynamicTemplate:false Unit: MetricType: ObjectType: ObjectTypeMappingType: ScalingFactor:0 ObjectTypeParams:[] Analyzed: Count:0 Searchable: Aggregatable: Script: Pattern: InputFormat: OutputFormat: OutputPrecision: LabelTemplate: UrlTemplate:[] OpenLinkInCurrentTab: Overwrite:false DefaultField: Path:kubernetes.persistentvolumeclaim.name}, {"aggregatable":true,"analyzed":false,"count":0,"doc_values":true,"indexed":true,"name":"kubernetes.persistentvolumeclaim.name","scripted":false,"searchable":true,"type":"string"}.

@MichaelKatsoulis
Copy link
Contributor

Apparently, adding the field broke the pipeline with the following error:

@herrBez in beats, the different kubernetes metricsets share the same index and mappings, so if the field is already declared, you don't need to declare it again. There is a duplicate key.
In integrations repo, when this field will be also added, then you need to add it kubernetes.volume data stream as well. The reason is that in agent each data stream has its own index and hence mappings.

Copy link
Contributor

mergify bot commented May 8, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b kubernetes-pvc-volume upstream/kubernetes-pvc-volume
git merge upstream/main
git push upstream kubernetes-pvc-volume

@herrBez
Copy link
Contributor Author

herrBez commented May 8, 2024

Hi Michael, I addressed the required changes and the build works fine

@MichaelKatsoulis
Copy link
Contributor

@herrBez Can you add a screenshot showing the new field being populated to Elasticsearch? You can use a view from discovery filtering by the kubernetes.volume metricset and showing the new field.

@@ -75,6 +75,9 @@ func eventMapping(content []byte, logger *logp.Logger) ([]mapstr.M, error) {
if volume.Inodes > 0 {
kubernetes2.ShouldPut(volumeEvent, "fs.inodes.pct", float64(volume.InodesUsed)/float64(volume.Inodes), logger)
}
if volume.PvcRef.Name != "" {
kubernetes2.ShouldPut(volumeEvent, "persistentvolumeclaim.name", volume.PvcRef.Name, logger)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the feeling that this way the field that will be generated will be kubernetes.volume.persistentvolumeclaim.name . Is that what you want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, great static analysis :D.. Indeed, its creating the field you mention. I actually want to reuse the field kubernetes.persistentvolumeclaim.name to allow correlation, but I am honestly a little bit lost.

After some thinking I tried with:

kubernetes2.ShouldPut(volumeEvent, mb.ModuleDataKey+".persistentvolumeclaim.name", volume.PvcRef.Name, logger)

But it does not seem to work 🤔

Copy link
Contributor Author

@herrBez herrBez May 10, 2024

Choose a reason for hiding this comment

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

Ok, the line I wrote in the comment is correct. For some reasons I were still using the old binary 🤦

Copy link
Contributor

Choose a reason for hiding this comment

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

I had the feeling 😂

@herrBez
Copy link
Contributor Author

herrBez commented May 10, 2024

Here is the screenshot with the new version:

image

@herrBez
Copy link
Contributor Author

herrBez commented May 14, 2024

Can I go ahead and merge it? Should I open a PR in the integration repository to add the field to the kubernetes.volume datastream?

@MichaelKatsoulis
Copy link
Contributor

Can I go ahead and merge it? Should I open a PR in the integration repository to add the field to the kubernetes.volume datastream?

I will merge it. About the field in the integration, not yet. We do it after the release of beats. I will add this in our list cause we have some other fields to declare as well.

@MichaelKatsoulis MichaelKatsoulis merged commit 9b338cd into elastic:main May 16, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add pvc name and namespace to kubernetes volume metricset
5 participants