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

Remove proxymap entry after closing connection #3190

Merged
merged 3 commits into from
Mar 19, 2018
Merged

Remove proxymap entry after closing connection #3190

merged 3 commits into from
Mar 19, 2018

Conversation

tgraf
Copy link
Member

@tgraf tgraf commented Mar 17, 2018

No description provided.

@tgraf tgraf added kind/bug This is a bug in the Cilium logic. wip area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Mar 17, 2018
@tgraf tgraf requested a review from a team March 17, 2018 12:46
@tgraf tgraf requested a review from a team as a code owner March 17, 2018 12:46
houndci-bot
houndci-bot reviewed Mar 17, 2018
View reviewed changes
pkg/maps/proxymap/proxymap.go
// ProxyMapKey is the generic type for Proxy6Key or Proxy4Key
type ProxyMapKey interface{}

type ProxyMapValue interface {
Copy link

Choose a reason for hiding this comment

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

exported type ProxyMapValue should have comment or be unexported
type name will be used as proxymap.ProxyMapValue by other packages, and that stutters; consider calling this Value

pkg/maps/proxymap/proxymap.go Outdated
)

// ProxyMapKey is the generic type for Proxy6Key or Proxy4Key
type ProxyMapKey interface{}
Copy link

Choose a reason for hiding this comment

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

type name will be used as proxymap.ProxyMapKey by other packages, and that stutters; consider calling this Key

pkg/maps/proxymap/ipv6.go Outdated
@@ -47,6 +47,10 @@ type Proxy6Value struct {
Lifetime uint32
}

func (v *Proxy6Value) GetSourceIdentity() uint32 {
Copy link

Choose a reason for hiding this comment

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

exported method Proxy6Value.GetSourceIdentity should have comment or be unexported

pkg/maps/proxymap/ipv4.go Outdated
@@ -50,6 +50,10 @@ type Proxy4Value struct {
Lifetime uint32
}

func (v *Proxy4Value) GetSourceIdentity() uint32 {
Copy link

Choose a reason for hiding this comment

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

exported method Proxy4Value.GetSourceIdentity should have comment or be unexported

houndci-bot
houndci-bot reviewed Mar 17, 2018
View reviewed changes
pkg/maps/proxymap/proxymap.go
// ProxyMapKey is the generic type for Proxy6Key or Proxy4Key
type ProxyMapKey interface{}

type ProxyMapValue interface {
Copy link

Choose a reason for hiding this comment

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

exported type ProxyMapValue should have comment or be unexported
type name will be used as proxymap.ProxyMapValue by other packages, and that stutters; consider calling this Value

pkg/maps/proxymap/proxymap.go Outdated
)

// ProxyMapKey is the generic type for Proxy6Key or Proxy4Key
type ProxyMapKey interface{}
Copy link

Choose a reason for hiding this comment

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

type name will be used as proxymap.ProxyMapKey by other packages, and that stutters; consider calling this Key

@tgraf tgraf changed the title Proxymap remove on close Remove proxymap entry after closing connection Mar 17, 2018
@tgraf tgraf mentioned this pull request Mar 17, 2018
Closed
@cilium cilium deleted a comment from houndci-bot Mar 18, 2018
@cilium cilium deleted a comment from houndci-bot Mar 18, 2018
@tgraf
Copy link
Member Author

tgraf commented Mar 18, 2018

test-me-please

@tgraf tgraf mentioned this pull request Mar 18, 2018
Merged
@tgraf tgraf added pending-review and removed wip labels Mar 18, 2018
@tgraf tgraf requested a review from joestringer March 18, 2018 10:27
@tgraf tgraf requested a review from a team as a code owner March 18, 2018 17:23
houndci-bot
houndci-bot reviewed Mar 18, 2018
View reviewed changes
pkg/maps/proxymap/proxymap.go
)

// ProxyMapKey is the generic type for Proxy6Key or Proxy4Key
type ProxyMapKey interface{}
Copy link

Choose a reason for hiding this comment

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

type name will be used as proxymap.ProxyMapKey by other packages, and that stutters; consider calling this Key

@tgraf
Copy link
Member Author

tgraf commented Mar 18, 2018

test-me-please

rlenglet
rlenglet suggested changes Mar 18, 2018
View reviewed changes
pkg/proxy/kafka.go Outdated
// The proxymap contains an entry with metadata for the receive side of the
// connection, remove it after the connection has been closed.
if pair.Rx != nil {
if err := k.redirect.removeProxyMapEntryOnClose(pair.Rx.conn); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

It can take up to 1 minute for the rx connection to get actually closed, because that’s the delay we set for SO_LINGER. Therefore, we need to wait for at least 1 minute before removing the proxy map entry. Otherwise, the connection may still hang for the client.

Copy link
Member Author

Choose a reason for hiding this comment

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

If 1 minute is too long, we can reduce the SO_LINGER delay. But we should always wait for at least that delay before removing the map entry.

Good point. I'm not sure lingering is actually better though. 1 minute is definitely too long just to send out some existing data. I agree that we should wait though.

@rlenglet
Copy link
Contributor

If 1 minute is too long, we can reduce the SO_LINGER delay. But we should always wait for at least that delay before removing the map entry.

@tgraf
Copy link
Member Author

tgraf commented Mar 19, 2018

test-me-please

rlenglet
rlenglet approved these changes Mar 19, 2018
View reviewed changes
Copy link
Contributor

@rlenglet rlenglet left a comment

Choose a reason for hiding this comment

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

LGTM. I would just add a small buffer to the sleep.

pkg/proxy/kafka.go Outdated
// We are running in our own go routine here so we can just
// block this go routine until after the connection is
// guaranteed to have been closed
time.Sleep(proxyConnectionCloseTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a small buffer, to let an eventual RST packet get through. In the worst case, the IP stack will decide to send an RST just around that time. We need to give it a chance to get through. How about adding like 1 second?

time.Sleep(proxyConnectionCloseTimeout + 1 * time.Second)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, can't hurt. It's probably unlikely though that we can transmit an RST if we can't flush out the data and close the connection within the timeout. It's likely a connectivity problem anyway.

@tgraf
proxymap: Add generic interface to delete, lookup and clean entries
7ff50bd 
Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
kafka: Remove proxymap entries on closing of receive connection
4e0087c 
When the receive side of the proxied connection is closed, after the SO_LINGER
timeout has passed, remove the proxymap entry as we no longer need access to
the metadata. This relaxes the garbage collector and pressure on the map
capacity.

Fixes: #3172

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
proxy: Reduce SO_LINGER timeout to 10 seconds
3ee259d 
The existing value of 1 minute can put a lot of stress on the proxymap table in
an environment with many concurrent, short lived connections.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
Copy link
Member Author

tgraf commented Mar 19, 2018

test-me-please

@tgraf tgraf merged commit bb11ad1 into master Mar 19, 2018
@tgraf tgraf deleted the proxymap-remove-on-close branch March 19, 2018 12:07
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

@rlenglet rlenglet rlenglet approved these changes

@ianvernon ianvernon ianvernon approved these changes

@joestringer joestringer Awaiting requested review from joestringer

Assignees
No one assigned
Labels
area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tgraf @rlenglet @ianvernon @houndci-bot