Skip to content

Conversation

@mxpv
Copy link
Member

@mxpv mxpv commented Aug 5, 2022

Signed-off-by: Maksym Pavlenko [email protected]

mxpv added 3 commits August 4, 2022 18:05
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
@mxpv mxpv force-pushed the go branch 4 times, most recently from e80e4a1 to 05b0693 Compare August 5, 2022 19:45
@containerd containerd deleted a comment from k8s-ci-robot Aug 5, 2022
@mxpv mxpv force-pushed the go branch 4 times, most recently from 85f5043 to 55e402f Compare August 5, 2022 23:55
@containerd containerd deleted a comment from k8s-ci-robot Aug 5, 2022
@mxpv
Copy link
Member Author

mxpv commented Aug 6, 2022

/test pull-containerd-sandboxed-node-e2e

@mxpv mxpv requested a review from fuweid August 6, 2022 03:04
Signed-off-by: Maksym Pavlenko <[email protected]>
Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@dmcgowan dmcgowan added this to the 1.7 milestone Aug 9, 2022
@dmcgowan
Copy link
Member

dmcgowan commented Aug 9, 2022

I marked this as cherry-pick for 1.6 as well since we will be supporting the 1.6 branch longer than go 1.18 will be supported. We don't necessarily need to cherry-pick right away though or use the same commits.

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

Addr: s.config.Addr,
Handler: s.handler,
TLSConfig: s.config.TLSConfig,
ReadHeaderTimeout: 3 * time.Second, // Fix linter G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server
Copy link
Member

Choose a reason for hiding this comment

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

No sure 3 second is good enough. Just in case, the 10 Minutes maybe better than 3 second because there is no limit at the beginning.

It's not block issue. But if we change it to low tolerance value, we should mark it in change log.

Cc @mikebrow

Copy link
Member

Choose a reason for hiding this comment

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

yeah this one is very likely to cause node failures..

Copy link
Member

Choose a reason for hiding this comment

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

I agree maybe 10 or 15min.. maybe 30?

@mxpv mxpv merged commit 23f66ec into containerd:main Aug 10, 2022
@mxpv mxpv deleted the go branch August 10, 2022 19:12
fuweid added a commit to fuweid/containerd that referenced this pull request Aug 13, 2022
It is follow-up of containerd#7254. This commit will increase ReadHeaderTimeout
from 3s to 30m, which prevent from unexpected timeout when the node is
running with high-load. 30 Minutes is longer enough to get close to
before what containerd#7254 changes.

And ideally, we should allow user to configure the streaming server if
the users want this feature.

Signed-off-by: Wei Fu <[email protected]>
fuweid added a commit to fuweid/containerd that referenced this pull request Aug 17, 2022
It is follow-up of containerd#7254. This commit will increase ReadHeaderTimeout
from 3s to 30m, which prevent from unexpected timeout when the node is
running with high-load. 30 Minutes is longer enough to get close to
before what containerd#7254 changes.

And ideally, we should allow user to configure the streaming server if
the users want this feature.

Signed-off-by: Wei Fu <[email protected]>
fuweid added a commit to fuweid/containerd that referenced this pull request Aug 17, 2022
It is follow-up of containerd#7254. This commit will increase ReadHeaderTimeout
from 3s to 30m, which prevent from unexpected timeout when the node is
running with high-load. 30 Minutes is longer enough to get close to
before what containerd#7254 changes.

And ideally, we should allow user to configure the streaming server if
the users want this feature.

Signed-off-by: Wei Fu <[email protected]>
estesp added a commit that referenced this pull request Aug 18, 2022
@fuweid fuweid mentioned this pull request Oct 10, 2022
vvejell1 pushed a commit to vvejell1/containerd that referenced this pull request Nov 4, 2022
It is follow-up of containerd#7254. This commit will increase ReadHeaderTimeout
from 3s to 30m, which prevent from unexpected timeout when the node is
running with high-load. 30 Minutes is longer enough to get close to
before what containerd#7254 changes.

And ideally, we should allow user to configure the streaming server if
the users want this feature.

Signed-off-by: Wei Fu <[email protected]>
thaJeztah pushed a commit to thaJeztah/containerd that referenced this pull request Mar 7, 2023
It is follow-up of containerd#7254. This commit will increase ReadHeaderTimeout
from 3s to 30m, which prevent from unexpected timeout when the node is
running with high-load. 30 Minutes is longer enough to get close to
before what containerd#7254 changes.

And ideally, we should allow user to configure the streaming server if
the users want this feature.

Signed-off-by: Wei Fu <[email protected]>
(cherry picked from commit 460b053)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah added cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch and removed cherry-pick/1.6.x labels Jul 14, 2023
jsturtevant pushed a commit to jsturtevant/containerd that referenced this pull request Sep 21, 2023
It is follow-up of containerd#7254. This commit will increase ReadHeaderTimeout
from 3s to 30m, which prevent from unexpected timeout when the node is
running with high-load. 30 Minutes is longer enough to get close to
before what containerd#7254 changes.

And ideally, we should allow user to configure the streaming server if
the users want this feature.

Signed-off-by: Wei Fu <[email protected]>
kiashok pushed a commit to kiashok/containerd that referenced this pull request Oct 23, 2024
It is follow-up of containerd#7254. This commit will increase ReadHeaderTimeout
from 3s to 30m, which prevent from unexpected timeout when the node is
running with high-load. 30 Minutes is longer enough to get close to
before what containerd#7254 changes.

And ideally, we should allow user to configure the streaming server if
the users want this feature.

Signed-off-by: Wei Fu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants