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

Honor connection pooling while tracing #7979

Merged
merged 1 commit into from
Jul 31, 2019

Conversation

harshavardhana
Copy link
Member

Description

Honor connection pooling while tracing

Motivation and Context

This PR fixes relying on r.Context().Done()
by setting

Connection: "close"

HTTP Header, this has detrimental issues for
client-side connection pooling. Since this
header explicitly tells clients to turn-off
connection pooling. This causing pro-active
connections to be closed leaving many conn's
in TIME_WAIT state. This can be observed with
mc admin trace -a when running distributed
setup.

This PR also fixes tracing filtering issue
when bucket names have minio as prefixes,
trace was erroneously ignoring them.

How to test this PR?

In a distributed setup mc admin trace -a to observe TIME_WAIT build up when you are performing lot's of I/O such as mc cp -r on another terminal.

Another bug fix related to filtering can be tested with the master branch when running mint tests, mc admin trace doesn't print anything. This PR fixes this issue.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Fixes a regression - This is a regression in a way because we should avoid forcibly closing connections between servers, this came in as part of the Tracing feature PR.
  • Documentation needed
  • Unit tests needed
  • Functional tests needed (If yes, add mint PR # here: )

@codecov-io
Copy link

codecov-io commented Jul 26, 2019

Codecov Report

Merging #7979 into master will increase coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7979      +/-   ##
==========================================
+ Coverage   45.01%   45.04%   +0.02%     
==========================================
  Files         315      315              
  Lines       50778    50746      -32     
==========================================
- Hits        22859    22856       -3     
+ Misses      25791    25761      -30     
- Partials     2128     2129       +1
Impacted Files Coverage Δ
cmd/handler-utils.go 75.43% <ø> (+4.29%) ⬆️
cmd/peer-rest-server.go 1.19% <0%> (ø) ⬆️
cmd/admin-handlers.go 10.45% <0%> (-0.07%) ⬇️
cmd/peer-rest-client.go 1.51% <0%> (-0.01%) ⬇️
cmd/fs-v1.go 61.06% <0%> (-0.38%) ⬇️
cmd/iam.go 14.97% <0%> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c71895f...5a954a4. Read the comment docs.

cmd/admin-handlers.go Outdated Show resolved Hide resolved
cmd/peer-rest-server.go Outdated Show resolved Hide resolved
cmd/admin-handlers.go Outdated Show resolved Hide resolved
@harshavardhana harshavardhana force-pushed the fix-trace branch 2 times, most recently from 60bbffa to fbf26eb Compare July 30, 2019 20:39
@harshavardhana
Copy link
Member Author

PTAL @krisis @poornas fixed the comments.

cmd/admin-handlers.go Outdated Show resolved Hide resolved
@poornas
Copy link
Contributor

poornas commented Jul 30, 2019

build failures on this @harshavardhana

@harshavardhana harshavardhana force-pushed the fix-trace branch 2 times, most recently from e99cb03 to bc3bc2c Compare July 30, 2019 21:43
Copy link
Contributor

@poornas poornas left a comment

Choose a reason for hiding this comment

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

your change introduces a regression, the mc client receives trace only on the node it made a connection to, not all the nodes in the cluster

@harshavardhana
Copy link
Member Author

your change introduces a regression, the mc client receives trace only on the node it made a connection to, not all the nodes in the cluster

which part of the code that would be causing regression @poornas ?

@harshavardhana harshavardhana force-pushed the fix-trace branch 3 times, most recently from 5df7afb to ea36b8a Compare July 30, 2019 23:48
This PR fixes relying on r.Context().Done()
by setting

```
Connection: "close"
```

HTTP Header, this has detrimental issues for
client side connection pooling. Since this
header explicitly tells clients to turn-off
connection pooling. This causing pro-active
connections to be closed leaving many conn's
in TIME_WAIT state. This can be observed with
`mc admin trace -a` when running distributed
setup.

This PR also fixes tracing filtering issue
when bucket names have `minio` as prefixes,
trace was erroneously ignoring them.
@minio-ops
Copy link

Mint Automation

Test Result
mint-compression-xl.sh ✔️
mint-xl.sh ✔️
mint-large-bucket.sh ✔️
mint-compression-dist-xl.sh ✔️
mint-compression-fs.sh ✔️
mint-worm.sh ✔️
mint-fs.sh ✔️
mint-dist-xl.sh ✔️
mint-gateway-nas.sh more...

7979-5a954a4/mint-gateway-nas.sh.log:

Running with
SERVER_ENDPOINT:      72.28.97.57:32675
ACCESS_KEY:           minio
SECRET_KEY:           ***REDACTED***
ENABLE_HTTPS:         0
SERVER_REGION:        us-east-1
MINT_DATA_DIR:        /mint/data
MINT_MODE:            full
ENABLE_VIRTUAL_STYLE: 0

To get logs, run 'docker cp b5ab07884333:/mint/log /tmp/mint-logs'

(1/14) Running aws-sdk-go tests ... FAILED in 13 seconds
{
  "alert": "",
  "args": {
    "bucketName": "aws-sdk-go-test-z3wllrgrgtwuwd",
    "expiry": 60000000000,
    "objectName": "presignedTest"
  },
  "duration": 12599,
  "error": "RequestError: send request failed\ncaused by: Put http://72.28.97.57:32675/aws-sdk-go-test-z3wllrgrgtwuwd: dial tcp 72.28.97.57:32675: connect: no route to host",
  "function": "PresignedPut",
  "message": "AWS SDK Go CreateBucket Failed",
  "name": "aws-sdk-go",
  "status": "FAIL"
}

Executed 0 out of 14 tests successfully.

@kannappanr kannappanr merged commit 123ccca into minio:master Jul 31, 2019
@harshavardhana harshavardhana deleted the fix-trace branch July 31, 2019 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants