Skip to content

improve DefaultClientDialContext#944

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
p0lyn0mial:improve-setting-tcp-usr-timeout
Nov 23, 2020
Merged

improve DefaultClientDialContext#944
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
p0lyn0mial:improve-setting-tcp-usr-timeout

Conversation

@p0lyn0mial
Copy link
Contributor

wrapping the DialContext method and setting the socket options on a copy of the file descriptor is not deterministic.
as per documentation attempting to change properties of the original using this duplicate
may or may not have the desired effect.

instead we should set the options on the original file descriptor.

@p0lyn0mial
Copy link
Contributor Author

I performed the manual tests described #926 (comment) and #926 (comment) and haven't found any issues.

I got the same results.

@p0lyn0mial
Copy link
Contributor Author

/assign @sttts

@p0lyn0mial
Copy link
Contributor Author

/cherry-pick release-4.6

@openshift-cherrypick-robot

@p0lyn0mial: once the present PR merges, I will cherry-pick it on top of release-4.6 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-4.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sttts
Copy link
Contributor

sttts commented Nov 23, 2020

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 23, 2020
Control: func(network, address string, con syscall.RawConn) error {
var err error
err = con.Control(func(fd uintptr) {
err = setDefaultSocketOptions(int(fd))
Copy link
Contributor

Choose a reason for hiding this comment

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

which err will survive?

Copy link
Contributor Author

@p0lyn0mial p0lyn0mial Nov 23, 2020

Choose a reason for hiding this comment

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

/hold

good catch, thx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/hold cancel

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 23, 2020
@p0lyn0mial p0lyn0mial force-pushed the improve-setting-tcp-usr-timeout branch 2 times, most recently from e5bbe18 to eb0dcb8 Compare November 23, 2020 12:01
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 23, 2020
@sttts
Copy link
Contributor

sttts commented Nov 23, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2020
@p0lyn0mial p0lyn0mial force-pushed the improve-setting-tcp-usr-timeout branch from eb0dcb8 to c6a33e2 Compare November 23, 2020 12:11
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2020
@p0lyn0mial p0lyn0mial force-pushed the improve-setting-tcp-usr-timeout branch from c6a33e2 to 0fc42f4 Compare November 23, 2020 12:25
wrapping the DialContext method and setting the socket options on a copy of the file descriptor is not deterministic.
as per documentation attempting to change properties of the original using this duplicate
may or may not have the desired effect.

instead we should set the options on the original file descriptor.
@p0lyn0mial p0lyn0mial force-pushed the improve-setting-tcp-usr-timeout branch from 0fc42f4 to a8786c0 Compare November 23, 2020 12:26
@sttts
Copy link
Contributor

sttts commented Nov 23, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: p0lyn0mial, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-cherrypick-robot

@p0lyn0mial: new pull request created: #945

Details

In response to this:

/cherry-pick release-4.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants