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.

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

operator: Add support for configuring HTTP server timeouts #9405

Merged
merged 35 commits into from
May 22, 2023

Conversation

periklis
Copy link
Collaborator

@periklis periklis commented May 5, 2023

What this PR does / why we need it:
The server-side timeout (read & write) have been so far constant not-aligned values, i.e. the lokistack-gateway used 12m and loki 1m. This mismatch yields to server timeouts after one minute when doing long-range queries. This PR adds support to configure these values via the LokiStack custom resource. In addition it aligns the gateway's timeouts with those of Loki by reusing the latter and adding some wiggle room.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@periklis periklis requested a review from xperimental May 5, 2023 12:17
@periklis periklis requested a review from a team as a code owner May 5, 2023 12:17
@periklis periklis self-assigned this May 5, 2023
operator/CHANGELOG.md Outdated Show resolved Hide resolved
operator/apis/loki/v1/lokistack_types.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@xperimental xperimental left a comment

Choose a reason for hiding this comment

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

Left a few comments in the PR and made a branch with the goal of making the code simpler and avoiding a few indirections. Let me know what you think.

operator/internal/manifests/var.go Outdated Show resolved Hide resolved
operator/internal/manifests/options_test.go Outdated Show resolved Hide resolved
operator/internal/manifests/options_test.go Outdated Show resolved Hide resolved
operator/internal/manifests/options.go Outdated Show resolved Hide resolved
operator/internal/manifests/options.go Outdated Show resolved Hide resolved
operator/internal/manifests/internal/config/options.go Outdated Show resolved Hide resolved
@xperimental
Copy link
Collaborator

As I mentioned during the daily, I noticed that our defaults for queryTimeout differ between documentation (1m) and code (3m). I have updated the linked diff with additional changes. All existing comments should be covered.

Copy link
Collaborator

@xperimental xperimental left a comment

Choose a reason for hiding this comment

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

Timeout customization seems to work fine in test-cluster. Did not test an actual long-running query yet, though.

@periklis periklis merged commit 1b87aea into grafana:main May 22, 2023
periklis added a commit to periklis/loki that referenced this pull request May 23, 2023
)

Co-authored-by: Robert Jacob <rojacob@redhat.com>
periklis added a commit to periklis/loki that referenced this pull request May 23, 2023
)

Co-authored-by: Robert Jacob <rojacob@redhat.com>
openshift-merge-robot added a commit to openshift/loki that referenced this pull request May 23, 2023
openshift-merge-robot added a commit to openshift/loki that referenced this pull request May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants