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

Fixed docker diskio bug due to reseting of map. #5582

Merged
merged 5 commits into from
Nov 21, 2017
Merged

Fixed docker diskio bug due to reseting of map. #5582

merged 5 commits into from
Nov 21, 2017

Conversation

kwojcicki
Copy link
Contributor

Issue explaining this bug #5568 general idea is if 2+ containers the map is overwritten every time and no delta metrics are pushed

@elasticmachine
Copy link
Collaborator

Can one of the admins verify this patch?

houndci-bot
houndci-bot reviewed Nov 14, 2017
View reviewed changes
metricbeat/module/docker/diskio/diskio_test.go Outdated
apiContainer.Stats.Read = time.Now()
apiContainer.Container = containers
apiContainer.Stats.BlkioStats.IOServicedRecursive = append(apiContainer.Stats.BlkioStats.IOServicedRecursive, metrics)
dockerStats := make([]docker.Stat, 0)
Copy link

Choose a reason for hiding this comment

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

can probably use "var dockerStats []docker.Stat" instead

metricbeat/module/docker/diskio/diskio_test.go Outdated
apiContainer2.Stats.Read = time.Now()
apiContainer2.Container = containers[1]
apiContainer2.Stats.BlkioStats.IOServicedRecursive = append(apiContainer2.Stats.BlkioStats.IOServicedRecursive, metrics)
dockerStats := make([]docker.Stat, 0)
Copy link

Choose a reason for hiding this comment

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

can probably use "var dockerStats []docker.Stat" instead

houndci-bot
houndci-bot reviewed Nov 14, 2017
View reviewed changes
metricbeat/module/docker/diskio/diskio_test.go Outdated
apiContainer.Stats.Read = time.Now()
apiContainer.Container = containers
apiContainer.Stats.BlkioStats.IOServicedRecursive = append(apiContainer.Stats.BlkioStats.IOServicedRecursive, metrics)
dockerStats := make([]docker.Stat, 0)
Copy link

Choose a reason for hiding this comment

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

can probably use "var dockerStats []docker.Stat" instead

metricbeat/module/docker/diskio/diskio_test.go Outdated
apiContainer2.Stats.Read = time.Now()
apiContainer2.Container = containers[1]
apiContainer2.Stats.BlkioStats.IOServicedRecursive = append(apiContainer2.Stats.BlkioStats.IOServicedRecursive, metrics)
dockerStats := make([]docker.Stat, 0)
Copy link

Choose a reason for hiding this comment

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

can probably use "var dockerStats []docker.Stat" instead

@andrewkroh andrewkroh self-requested a review November 14, 2017 18:54
@andrewkroh andrewkroh requested a review from exekias November 14, 2017 18:54
@kwojcicki
Copy link
Contributor Author

Not sure whats causing the failed test in travis-ci?

command [go test -tags integration -cover -coverprofile /tmp/gotestcover-533043541 github.com/elastic/beats/libbeat/outputs/redis]: exit status 1
--- FAIL: TestPublishListTLS (0.00s)
	redis_integration_test.go:270: Failed to connect to redis host: dial tcp 172.18.0.6:6380: getsockopt: connection refused
--- FAIL: TestPublishChannelTLS (0.02s)
	redis_integration_test.go:270: Failed to connect to redis host: dial tcp 172.18.0.6:6380: getsockopt: connection refused
FAIL

Might have just been an issue on the pipeline side having no networking temporarily?

@ruflin
Copy link
Contributor

ruflin commented Nov 15, 2017

@kwojcicki The above issue was caused by a change in alpine:edge and is not related to your changes. If you rebase on master this should disappear.

ruflin
ruflin reviewed Nov 15, 2017
View reviewed changes
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Cloud you add a Changelog entry?

Also have a look at the houndci bot comments.

metricbeat/module/docker/diskio/diskio_test.go
)

var blkioService BLkioService
var oldBlkioRaw = make([]BlkioRaw, 3)
var newBlkioRaw = make([]BlkioRaw, 3)

func TestDeltaMultipleContainers(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding these tests

andrewkroh
andrewkroh reviewed Nov 15, 2017
View reviewed changes
metricbeat/module/docker/diskio/diskio_test.go Outdated
dockerStats = append(dockerStats, apiContainer1)
dockerStats = append(dockerStats, apiContainer2)
stats := blkioService.getBlkioStatsList(dockerStats)
totals := make([]float64, 2, 2)
Copy link
Member

Choose a reason for hiding this comment

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

No need to specify a capacity because the capacity of the slice is equal to its length if unspecified.

@kwojcicki
Copy link
Contributor Author

@ruflin added changelog entry, addressed houndci comments and I attempted to rebase not sure I did it correctly? (sorry not super great with git)

ruflin
ruflin approved these changes Nov 16, 2017
View reviewed changes
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM. @andrewkroh Could you also have a look again?

@andrewkroh
Copy link
Member

jenkins, test it

@andrewkroh andrewkroh merged commit 6ba7fdc into elastic:master Nov 21, 2017
@andrewkroh andrewkroh added the needs_backport PR is waiting to be backported to other branches. label Nov 21, 2017
@kwojcicki
Copy link
Contributor Author

I was thinking of back porting this to 6.0 and maybe 5.6 any objections to that?

@andrewkroh
Copy link
Member

SGTM

BTW we have a script at ./dev-tools/cherrpick_pr that most of us use to automate the cherrypick and PR creation.

kwojcicki added a commit to kwojcicki/beats that referenced this pull request Nov 23, 2017
@kwojcicki
Fixed docker diskio bug due to reseting of map. (elastic#5582)
582b0b7 
* Fixed docker diskio bug due to reseting of map.

Fixes elastic#5568
@kwojcicki kwojcicki mentioned this pull request Nov 23, 2017
Merged
kwojcicki added a commit to kwojcicki/beats that referenced this pull request Nov 23, 2017
@kwojcicki
Fixed docker diskio bug due to reseting of map. (elastic#5582)
25b6dda 
* Fixed docker diskio bug due to reseting of map.

Fixes elastic#5568
@kwojcicki kwojcicki mentioned this pull request Nov 24, 2017
Merged
ruflin pushed a commit that referenced this pull request Nov 27, 2017
@kwojcicki @ruflin
Fixed docker diskio bug due to reseting of map. (#5582) (#5698)
ebdec71 
* Fixed docker diskio bug due to reseting of map.

Fixes #5568
ruflin pushed a commit that referenced this pull request Nov 27, 2017
@kwojcicki @ruflin
Fixed docker diskio bug due to reseting of map. (#5582) (#5705)
b1807df 
* Fixed docker diskio bug due to reseting of map.

Fixes #5568
@ruflin ruflin removed the needs_backport PR is waiting to be backported to other branches. label Nov 28, 2017
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
@kwojcicki @ruflin
Fixed docker diskio bug due to reseting of map. (elastic#5582) (elast…
809118b 
…ic#5698)

* Fixed docker diskio bug due to reseting of map.

Fixes elastic#5568
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@andrewkroh andrewkroh andrewkroh left review comments

@ruflin ruflin ruflin approved these changes

@exekias exekias Awaiting requested review from exekias

Assignees
No one assigned
Labels
bug Metricbeat Metricbeat
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kwojcicki @elasticmachine @ruflin @andrewkroh @houndci-bot