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.

Sign up for GitHub

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

Jump to bottom

Fix types for response fields of kubelet api #23335

Merged
merged 4 commits into from
Jan 7, 2021
Merged

Fix types for response fields of kubelet api #23335

merged 4 commits into from
Jan 7, 2021

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Jan 4, 2021

What does this PR do?

This PR adjusts the types of the response fields from Kubelet stats/summary api so as to be consistent with the original types of the fields as defined on api's side: https://github.com/kubernetes/kubelet/blob/088ee84ea259bf9c445109ea75b2938dd39d2074/pkg/apis/stats/v1alpha1/types.go

Why is it important?

To avoid hitting integer overflow since we store as int64 while the api can serve values stored as uint64.
int64: -9223372036854775808 to 9223372036854775807
uint64: 0 to 18446744073709551615

Closes #19475

@ChrsMark
Fix types for response fields of kubelet api
efbd2d3 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark added review [zube]: In Review Team:Platforms Label for the Integrations - Platforms team v7.10.0 needs_reviewer PR needs to be assigned a reviewer v7.11.0 v7.12.0 labels Jan 4, 2021
@ChrsMark ChrsMark self-assigned this Jan 4, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Platforms)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jan 4, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 4, 2021 •
edited by jenkins-beats-ci bot
Loading

💚 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

Expand to view the summary

Build stats

  • Build Cause: Pull request #23335 updated

  • Start Time: 2021-01-05T15:57:11.406+0000

  • Duration: 51 min 25 sec

Test stats 🧪

Test Results
Failed 0
Passed 2288
Skipped 507
Total 2795

Steps errors 2

Expand to view the steps failures

Terraform Apply on x-pack/metricbeat/module/aws
  • Took 0 min 15 sec . View more details on here
  • Description: terraform apply -auto-approve
Terraform Apply on x-pack/metricbeat/module/aws
  • Took 0 min 15 sec . View more details on here
  • Description: terraform apply -auto-approve

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 2288
Skipped 507
Total 2795

@ChrsMark
Fix inconsistent types
7809747 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
jsoriano
jsoriano reviewed Jan 4, 2021
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Looks good! But we also have to check if we can store these uint64 numbers in ES with current mappings.

metricbeat/module/kubernetes/pod/data.go
@@ -40,7 +40,7 @@ func eventMapping(content []byte, perfMetrics *util.PerfMetricsCache) ([]common.
nodeCores := perfMetrics.NodeCoresAllocatable.Get(node.NodeName)
nodeMem := perfMetrics.NodeMemAllocatable.Get(node.NodeName)
for _, pod := range summary.Pods {
var usageNanoCores, usageMem, availMem, rss, workingSet, pageFaults, majorPageFaults int64
var usageNanoCores, usageMem, availMem, rss, workingSet, pageFaults, majorPageFaults uint64
Copy link
Member

@jsoriano jsoriano Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We are currently defining these values as long in fields.yml (with a minimum value of -2^63 and a maximum value of 2^63-1). We probably need to change the mapping to be able to store big numbers. unsigned_long is only supported since 7.10, maybe we can use double for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, thanks for catching this! But does this mean that we can only ship it a new minor release right? Otherwise, with patch releases it might cause mapping conflicts?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's backport this only till 7.11.

@ChrsMark
Update fields
6aa7704 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@jsoriano jsoriano removed the v7.10.0 label Jan 5, 2021
@ChrsMark
Add changelog entry
10a046f 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark merged commit 9b7676f into elastic:master Jan 7, 2021
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Jan 7, 2021
@ChrsMark
Fix types for response fields of kubelet api (elastic#23335)
7bd1179 
(cherry picked from commit 9b7676f)
@ChrsMark ChrsMark mentioned this pull request Jan 7, 2021
Merged
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Jan 7, 2021
@ChrsMark
Fix types for response fields of kubelet api (elastic#23335)
f88843b 
(cherry picked from commit 9b7676f)
@ChrsMark ChrsMark mentioned this pull request Jan 7, 2021
Merged
ChrsMark added a commit that referenced this pull request Jan 7, 2021
@ChrsMark
Cherry-pick #23335 to 7.x: Fix types for response fields of kubelet a…
110333e 
…pi (#23383)
ChrsMark added a commit that referenced this pull request Jan 7, 2021
@ChrsMark
Cherry-pick #23335 to 7.11: Fix types for response fields of kubelet …
1aeedfb 
…api (#23384)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsoriano jsoriano jsoriano approved these changes

Assignees

@ChrsMark ChrsMark

Labels
needs_reviewer PR needs to be assigned a reviewer review Team:Platforms Label for the Integrations - Platforms team v7.11.0 v7.12.0 [zube]: In Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[MetricBeat] Kubernetes module Go struct field .pods.volume.inodesFree of type int64
3 participants
@ChrsMark @elasticmachine @jsoriano