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

Bug 1897918: UPSTREAM: 93861: apiserver: add --permit-address-sharing flag to listen with SO_REUSEADDR #309

Merged
merged 1 commit into from
Feb 13, 2021
Merged

Bug 1897918: UPSTREAM: 93861: apiserver: add --permit-address-sharing flag to listen with SO_REUSEADDR #309

merged 1 commit into from
Feb 13, 2021

Conversation

sttts
Copy link

@sttts sttts commented Aug 10, 2020 •
edited
Loading

Compare https://stackoverflow.com/a/14388707. This allows to bind before a closed socket leave TIME_WAIT state.

Note: this does not allow another process to bind to the same addr/port in non TIME_WAIT state.

Credits to @bparees for having the insights into the SO_REUSEADDR semantics.

Upstream: kubernetes#93861

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2020
@sttts sttts force-pushed the sttts-so-reuseaddr branch from fa366a9 to 624d596 Compare August 10, 2020 13:54
@sttts sttts force-pushed the sttts-so-reuseaddr branch from 624d596 to d8a7b3b Compare August 10, 2020 13:58
@sttts sttts changed the title UPSTREAM: 00000: apiserver: add --permit-port-sharing flag to listen with SO_REUSEADDR UPSTREAM: 00000: apiserver: add --permit-address-sharing flag to listen with SO_REUSEADDR Aug 10, 2020
@sttts sttts force-pushed the sttts-so-reuseaddr branch 3 times, most recently from ebdffa5 to e9de99b Compare August 10, 2020 15:01
@sttts sttts changed the title UPSTREAM: 00000: apiserver: add --permit-address-sharing flag to listen with SO_REUSEADDR UPSTREAM: 93861: apiserver: add --permit-address-sharing flag to listen with SO_REUSEADDR Aug 10, 2020
@sttts
Copy link
Author

sttts commented Aug 11, 2020

/retest

1 similar comment
@sttts
Copy link
Author

sttts commented Aug 21, 2020

/retest

@openshift-ci-robot
Copy link

@sttts: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-fips e9de99b419db5df53ab74eb8637f9d003db8eb16 link /test e2e-aws-fips
ci/prow/verify e9de99b419db5df53ab74eb8637f9d003db8eb16 link /test verify
ci/prow/k8s-e2e-gcp e9de99b419db5df53ab74eb8637f9d003db8eb16 link /test k8s-e2e-gcp

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@sttts sttts mentioned this pull request Aug 29, 2020
Merged
@sttts
Copy link
Author

sttts commented Oct 27, 2020

/retest

@openshift-merge-robot
Copy link

openshift-merge-robot commented Oct 27, 2020 •
edited
Loading

@sttts: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/verify e9de99b419db5df53ab74eb8637f9d003db8eb16 link /test verify
ci/prow/k8s-e2e-gcp e9de99b419db5df53ab74eb8637f9d003db8eb16 link /test k8s-e2e-gcp
ci/prow/e2e-aws-serial e9de99b419db5df53ab74eb8637f9d003db8eb16 link /test e2e-aws-serial

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@sttts sttts changed the title UPSTREAM: 93861: apiserver: add --permit-address-sharing flag to listen with SO_REUSEADDR Bug 1897918: UPSTREAM: 93861: apiserver: add --permit-address-sharing flag to listen with SO_REUSEADDR Nov 16, 2020
@openshift-ci-robot openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Nov 16, 2020
@openshift-ci-robot
Copy link

@sttts: This pull request references Bugzilla bug 1897918, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.7.0) matches configured target release for branch (4.7.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1897918: UPSTREAM: 93861: apiserver: add --permit-address-sharing flag to listen with SO_REUSEADDR

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 sttts force-pushed the sttts-so-reuseaddr branch from e9de99b to d793b14 Compare February 12, 2021 10:36
@openshift-ci-robot openshift-ci-robot added the backports/unvalidated-commits Indicates that not all commits come to merged upstream PRs. label Feb 12, 2021
@openshift-ci-robot
Copy link

@sttts: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@openshift-ci-robot openshift-ci-robot added the vendor-update Touching vendor dir or related files label Feb 12, 2021
@sttts sttts force-pushed the sttts-so-reuseaddr branch from d793b14 to c961747 Compare February 12, 2021 10:49
@openshift-ci-robot
Copy link

@sttts: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

p0lyn0mial
p0lyn0mial reviewed Feb 12, 2021
View reviewed changes
staging/src/k8s.io/apiserver/pkg/server/options/serving_unix.go Outdated

func permitAddressReuse(network, addr string, conn syscall.RawConn) error {
return conn.Control(func(fd uintptr) {
syscall.SetsockoptInt(int(fd), syscall.SOL_SOCKET, unix.SO_REUSEADDR, 1)
Copy link

Choose a reason for hiding this comment

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

we don't want to track potential errors?

https://golang.org/pkg/syscall/#SetsockoptInt

func SetsockoptInt(fd, level, opt int, value int) (err error)

Copy link
Author

Choose a reason for hiding this comment

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

yeah maybe

Copy link
Author

Choose a reason for hiding this comment

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

done

@sttts
Copy link
Author

sttts commented Feb 12, 2021

/retest

Flake.

@sttts sttts force-pushed the sttts-so-reuseaddr branch from c961747 to fa8a12c Compare February 12, 2021 13:45
@openshift-ci-robot
Copy link

@sttts: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@p0lyn0mial
Copy link

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2021
p0lyn0mial
p0lyn0mial reviewed Feb 12, 2021
View reviewed changes
staging/src/k8s.io/apiserver/pkg/server/options/serving.go Outdated
@@ -203,6 +207,11 @@ func (s *SecureServingOptions) AddFlags(fs *pflag.FlagSet) {
fs.BoolVar(&s.PermitPortSharing, "permit-port-sharing", s.PermitPortSharing,
"If true, SO_REUSEPORT will be used when binding the port, which allows "+
"more than one instance to bind on the same address and port. [default=false]")

fs.BoolVar(&s.PermitAddressSharing, "permit-address-sharing", s.PermitPortSharing,
Copy link

Choose a reason for hiding this comment

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

/hold

we also need to fix it to PermitAddressSharing

Copy link

Choose a reason for hiding this comment

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

the flag would be properly setting s.PermitAddressSharing just in the future the default value could be incorrect

Copy link
Author

Choose a reason for hiding this comment

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

Good catch!

Copy link
Author

Choose a reason for hiding this comment

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

fixed

/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 Feb 12, 2021
@sttts sttts force-pushed the sttts-so-reuseaddr branch from fa8a12c to 6dbab6b Compare February 12, 2021 15:12
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2021
@openshift-ci-robot
Copy link

@sttts: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@sttts sttts force-pushed the sttts-so-reuseaddr branch from 6dbab6b to 59f8ace Compare February 12, 2021 15:13
@openshift-ci-robot
Copy link

@sttts: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@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 Feb 12, 2021
@p0lyn0mial
Copy link

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2021
@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

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-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

8 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 4d85fcb into openshift:master Feb 13, 2021
@openshift-ci-robot
Copy link

@sttts: All pull requests linked via external trackers have merged:

Bugzilla bug 1897918 has been moved to the MODIFIED state.

In response to this:

Bug 1897918: UPSTREAM: 93861: apiserver: add --permit-address-sharing flag to listen with SO_REUSEADDR

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.

@hexfusion hexfusion mentioned this pull request Mar 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bparees bparees bparees left review comments

@p0lyn0mial p0lyn0mial p0lyn0mial left review comments

@deads2k deads2k Awaiting requested review from deads2k

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@p0lyn0mial p0lyn0mial

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. backports/unvalidated-commits Indicates that not all commits come to merged upstream PRs. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. vendor-update Touching vendor dir or related files
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sttts @openshift-ci-robot @openshift-merge-robot @p0lyn0mial @openshift-bot @bparees