Bug 1908707: fluentd fails to deliver message with Server returned nothing #73

syedriko · 2021-01-30T06:06:47Z

Description

Fix for https://bugzilla.redhat.com/show_bug.cgi?id=1908707.
Make net/http transport timeouts and limits configurable. Increase the default HTTP server read/write/idle timeouts from 5 seconds to 1 minute.
/cc @jcantrill
/assign @ewolinetz

…thing

openshift-ci-robot · 2021-01-30T06:06:54Z

@syedriko: This pull request references Bugzilla bug 1908707, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

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 ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1908707: fluentd fails to deliver message with Server returned nothing

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.

jcantrill · 2021-01-30T13:20:12Z

/approve

syedriko · 2021-01-30T15:24:34Z

/test cluster-logging-operator-e2e

…thing Corrected syntax of flags declarations, added unit tests for the new flags.

syedriko · 2021-01-31T15:58:39Z

/test cluster-logging-operator-e2e

jcantrill

Few nits that I am not firm on the necessity to resolve in the PR. I leave it to you , but otherwise lgtm

jcantrill · 2021-02-01T15:13:48Z

pkg/config/options.go

+		AuthBackEndRoles:          map[string]BackendRoleConfig{},
+		AuthWhiteListedNames:      []string{},
+		AuthAdminRole:             "",
+		HTTPReadTimeout:           time.Duration(1) * time.Minute,


For consistency, should we choose the same unit and apply it to all config?

I think this dovetails with the next comment. For consistency we'd need to go with the smallest unit we're likely to encounter, which is millisecond, and express a minute as time.Duration(60000) * time.Millisecond. This is arguably more difficult to read than time.Duration(1) * time.Minute. I'm leaning towards the easiest to read - the smallest value with the right units factor.

Per my other comment, as long as the CLI allows us to specify a duration I am satisfied leaving these as-is

jcantrill · 2021-02-01T15:14:35Z

pkg/config/flags.go

@@ -37,5 +37,18 @@ func newFlagSet() *flag.FlagSet {
 	flagSet.String("auth-admin-role", "", "The name of the only role that will be passed on the request if it is found in the list of roles")
 	flagSet.String("auth-default-role", "", "The role given to every request unless it has the auth-admin-role")

+	//net/http.Server timeouts for the server side of the proxy
+	flagSet.Duration("http-read-timeout", time.Duration(1)*time.Minute, "The maximum duration for reading the entire HTTP request. Zero means no timeout.")


Should we expand the flags to identify the unit? e.g http-read-timeout-sec

That was my first impulse, too, but I don't think it's the right thing. A flag of type time.Duration accepts many different time units, from nanoseconds to hours. See https://golang.org/pkg/flag/#Duration. Under the covers, https://golang.org/pkg/time/#ParseDuration does the parsing. I had fun with it at https://github.com/syedriko/elasticsearch-proxy/blob/f16618abbf400e2f3536c257df9bcc2cc52e86cf/pkg/config/options_test.go#L320

Can i define a proper duration in configuration on the CLI? It will let me use say 1s or 10m? If so, I'm good as-is

It will even let you define --http-expect-continue-timeout=1h2m3s4ms5us6ns if you so desire.

pkg/config/options.go

…thing Code review feedback

syedriko · 2021-02-02T01:59:39Z

/test cluster-logging-operator-e2e

jcantrill

/lgtm
/retest

openshift-ci-robot · 2021-02-02T13:49:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcantrill, syedriko

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:

~~OWNERS~~ [jcantrill]

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

jcantrill · 2021-02-02T13:49:46Z

/cherrypick release-4.6
/cherrypick release-4.5

openshift-cherrypick-robot · 2021-02-02T13:49:46Z

@jcantrill: 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.

In response to this:

/cherrypick release-4.6
/cherrypick release-4.5

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.

openshift-ci-robot · 2021-02-02T15:56:21Z

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

openshift/elasticsearch-proxy#73

Bugzilla bug 1908707 has been moved to the MODIFIED state.

In response to this:

Bug 1908707: fluentd fails to deliver message with Server returned nothing

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.

openshift-cherrypick-robot · 2021-02-02T15:56:33Z

@jcantrill: #73 failed to apply on top of branch "release-4.6":

Applying: Bug 1908707: fluentd fails to deliver message with Server returned nothing
Using index info to reconstruct a base tree...
M	pkg/config/options.go
M	pkg/proxy/server.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/proxy/server.go
Auto-merging pkg/config/options.go
CONFLICT (content): Merge conflict in pkg/config/options.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Bug 1908707: fluentd fails to deliver message with Server returned nothing
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-4.6
/cherrypick release-4.5

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.

Bug 1924258: fluentd fails to deliver message with Server returned nothing

Bug 1924329: fluentd fails to deliver message with Server returned nothing

Bug 1908707: fluentd fails to deliver message with Server returned no…

022ff2b

…thing

openshift-ci-robot assigned ewolinetz Jan 30, 2021

openshift-ci-robot requested a review from jcantrill January 30, 2021 06:06

openshift-ci-robot added bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent 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 Jan 30, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2021

syedriko changed the title ~~Bug 1908707: fluentd fails to deliver message with Server returned nothing~~ WIP Bug 1908707: fluentd fails to deliver message with Server returned nothing Jan 31, 2021

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 31, 2021

Bug 1908707: fluentd fails to deliver message with Server returned no…

f16618a

…thing Corrected syntax of flags declarations, added unit tests for the new flags.

syedriko changed the title ~~WIP Bug 1908707: fluentd fails to deliver message with Server returned nothing~~ Bug 1908707: fluentd fails to deliver message with Server returned nothing Jan 31, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 31, 2021

jcantrill reviewed Feb 1, 2021

View reviewed changes

Bug 1908707: fluentd fails to deliver message with Server returned no…

80ecd72

…thing Code review feedback

jcantrill reviewed Feb 2, 2021

View reviewed changes

openshift-ci-robot assigned jcantrill Feb 2, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2021

openshift-merge-robot merged commit c29db5d into openshift:master Feb 2, 2021

syedriko mentioned this pull request Feb 2, 2021

Bug 1924258: fluentd fails to deliver message with Server returned nothing #74

Merged

syedriko added a commit to syedriko/elasticsearch-proxy that referenced this pull request Feb 2, 2021

Manual cherry-pick of openshift#73

88da762

Bug 1924258: fluentd fails to deliver message with Server returned nothing

syedriko added a commit to syedriko/elasticsearch-proxy that referenced this pull request Feb 2, 2021

Manual cherry-pick of openshift#73

553bf90

Bug 1924258: fluentd fails to deliver message with Server returned nothing

syedriko added a commit to syedriko/elasticsearch-proxy that referenced this pull request Feb 2, 2021

Manual cherry-pick of openshift#73

1887937

Bug 1924329: fluentd fails to deliver message with Server returned nothing

syedriko mentioned this pull request Feb 2, 2021

Bug 1924329: fluentd fails to deliver message with Server returned nothing #75

Merged

jcantrill mentioned this pull request Feb 8, 2021

Writetimeout is too short for index-pattern creation in Kibana #69

Closed

syedriko deleted the bz_1908707 branch April 2, 2021 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1908707: fluentd fails to deliver message with Server returned nothing #73

Bug 1908707: fluentd fails to deliver message with Server returned nothing #73

syedriko commented Jan 30, 2021

openshift-ci-robot commented Jan 30, 2021

jcantrill commented Jan 30, 2021

syedriko commented Jan 30, 2021

syedriko commented Jan 31, 2021

jcantrill left a comment

jcantrill Feb 1, 2021

syedriko Feb 1, 2021 •

edited

Loading

jcantrill Feb 2, 2021

jcantrill Feb 1, 2021

syedriko Feb 1, 2021 •

edited

Loading

jcantrill Feb 2, 2021

syedriko Feb 2, 2021

syedriko commented Feb 2, 2021

jcantrill left a comment

openshift-ci-robot commented Feb 2, 2021

jcantrill commented Feb 2, 2021

openshift-cherrypick-robot commented Feb 2, 2021

openshift-ci-robot commented Feb 2, 2021

openshift-cherrypick-robot commented Feb 2, 2021

Bug 1908707: fluentd fails to deliver message with Server returned nothing #73

Bug 1908707: fluentd fails to deliver message with Server returned nothing #73

Conversation

syedriko commented Jan 30, 2021

Description

openshift-ci-robot commented Jan 30, 2021

jcantrill commented Jan 30, 2021

syedriko commented Jan 30, 2021

syedriko commented Jan 31, 2021

jcantrill left a comment

Choose a reason for hiding this comment

jcantrill Feb 1, 2021

Choose a reason for hiding this comment

syedriko Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

jcantrill Feb 2, 2021

Choose a reason for hiding this comment

jcantrill Feb 1, 2021

Choose a reason for hiding this comment

syedriko Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

jcantrill Feb 2, 2021

Choose a reason for hiding this comment

syedriko Feb 2, 2021

Choose a reason for hiding this comment

syedriko commented Feb 2, 2021

jcantrill left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 2, 2021

jcantrill commented Feb 2, 2021

openshift-cherrypick-robot commented Feb 2, 2021

openshift-ci-robot commented Feb 2, 2021

openshift-cherrypick-robot commented Feb 2, 2021

syedriko Feb 1, 2021 •

edited

Loading

syedriko Feb 1, 2021 •

edited

Loading