Skip to content

GEP: Add Timeouts GEP first cut #1744

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
Merged
merged 1 commit into from
Mar 8, 2023
Merged

GEP: Add Timeouts GEP first cut #1744

merged 1 commit into from
Mar 8, 2023

Conversation

youngnick
Copy link
Contributor

What type of PR is this?
/kind gep

What this PR does / why we need it:
This PR adds a first attempt at systematizing the discussion around timeouts in Gateway API.

It also adds support for Markdown tables and MermaidJS diagrams to our mkdocs installation.

Which issue(s) this PR fixes:
Updates #1742

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added kind/gep PRs related to Gateway Enhancement Proposal(GEP) cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 20, 2023
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 20, 2023
@youngnick
Copy link
Contributor Author

I'd recommend checking the preview at https://deploy-preview-1744--kubernetes-sigs-gateway-api.netlify.app/geps/gep-1742/ to see the diagrams properly.

dprotaso
dprotaso reviewed Feb 21, 2023
View reviewed changes
site-src/geps/gep-1742.md
Comment on lines +15 to +16
- Create some method to configure some timeouts
- Timeout config must be applicable to most if not all Gateway API implementations.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to capture what types of timeouts are relevant to a broad set of users even if implementations currently don't support it.

Then we have enough data to say to implementors - 'if you add support for X,Y timeouts you'll handle 80% of use cases'

@youngnick youngnick mentioned this pull request Feb 22, 2023
Closed
keithmattix
keithmattix approved these changes Feb 22, 2023
View reviewed changes
site-src/geps/gep-1742.md

## Non-Goals

- TBD
Copy link
Contributor

Choose a reason for hiding this comment

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

Support for all possible timouts seems like a good non-goal

site-src/geps/gep-1742.md
Create some sort of design so that Gateway API objects can be used to configure
timeouts for different types of connection.

## Goals
Copy link
Contributor

Choose a reason for hiding this comment

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

This is where we have to decide whether or not we want to be opinionated about what a timeout is

Xunzhuo
Xunzhuo reviewed Feb 27, 2023
View reviewed changes
site-src/geps/gep-1742.md
to make this process less difficult, then to find common timeouts that we can
build into Gateway API.

For this initial round, we'll focus on Layer 7 HTTP traffic, while acknowledging
Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should focus on L7 firstly.

Xunzhuo
Xunzhuo reviewed Feb 27, 2023
View reviewed changes
Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

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

Thanks for ping me to take a review @shaneutt. Happy to see this move on @youngnick.

Initial content looks good to me.

mlavacca
mlavacca approved these changes Feb 27, 2023
View reviewed changes
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

Thanks for asking me for the review. This document seems totally reasonable as a first iteration and a good starting point 👍

shaneutt
shaneutt approved these changes Feb 27, 2023
View reviewed changes
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

I like that for now we're just stating what we'd like to do and providing some groundwork information about the relevant proxies at play so we can continue to build from here (the mermaid diagrams are nice too). I think as provisional, this is a good place to start the conversation and I have no blockers. 👍

site-src/geps/gep-1742.md
Comment on lines +30 to +31
For this initial round, we'll focus on Layer 7 HTTP traffic, while acknowledging
that Layer 4 connections have their own interesting timeouts as well.
Copy link
Member

Choose a reason for hiding this comment

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

I'm tempted to say, put L4 in non-goals to keep the scope here focused on HTTPRoute. But I don't consider this a blocker or anything.

Xunzhuo
Xunzhuo approved these changes Mar 8, 2023
View reviewed changes
Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

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

looks good for me, and there is no blocker from me, maybe need another review from @robscott before getting merged this.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: keithmattix, mlavacca, shaneutt, Xunzhuo, youngnick

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

@youngnick
Copy link
Contributor Author

This GEP definitely needs some more information to be viable, but could be merged as-is because it's provisional anyway.

@youngnick youngnick mentioned this pull request Mar 8, 2023
Open
Xunzhuo
Xunzhuo reviewed Mar 8, 2023
View reviewed changes
Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks @youngnick !

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2023
@youngnick
Copy link
Contributor Author

/hold for multiple review

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2023
@youngnick
Copy link
Contributor Author

oh, actually, we already have another maintainer review, sorry @Xunzhuo.

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2023
@k8s-ci-robot k8s-ci-robot merged commit 0a9e2ac into kubernetes-sigs:main Mar 8, 2023
shaneutt added a commit to shaneutt/gateway-api that referenced this pull request Mar 8, 2023
@shaneutt
Revert "Merge pull request kubernetes-sigs#1744 from youngnick/gep-17…
67d32aa 
…42-timeouts"

This reverts commit 0a9e2ac, reversing
changes made to f844222.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dprotaso dprotaso dprotaso left review comments

@Xunzhuo Xunzhuo Xunzhuo approved these changes

@keithmattix keithmattix keithmattix approved these changes

@shaneutt shaneutt shaneutt approved these changes

@mlavacca mlavacca mlavacca approved these changes

@howardjohn howardjohn Awaiting requested review from howardjohn

@robscott robscott Awaiting requested review from robscott

@skriss skriss Awaiting requested review from skriss

Assignees

@Xunzhuo Xunzhuo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/gep PRs related to Gateway Enhancement Proposal(GEP) lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@youngnick @k8s-ci-robot @dprotaso @keithmattix @shaneutt @mlavacca @Xunzhuo