Remove `StreamingConnectionIdleTimeout` kubelet option #122104

saschagrunert · 2023-11-29T10:39:17Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The option is not used any more and the timeout is handled by the container runtime.

Removing the CLI because the kubelet already states:

Flag --streaming-connection-idle-timeout has been deprecated, This parameter should be set via the config file specified by the Kubelet's --config flag. See https://kubernetes.io/docs/tasks/administer-cluster/kubelet-config-file/ for more information.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

cc @kwilczynski

Does this PR introduce a user-facing change?

Removed `StreamingConnectionIdleTimeout`/`--streaming-connection-idle-timeout` kubelet option because it is not used any more.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

k8s-ci-robot · 2023-11-29T10:39:22Z

Please note that we're already in Test Freeze for the release-1.29 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.29.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Wed Nov 29 10:04:55 UTC 2023.

kwilczynski · 2023-11-29T15:38:10Z

/approve

k8s-ci-robot · 2023-11-29T15:38:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kwilczynski, saschagrunert
Once this PR has been reviewed and has the lgtm label, please assign smarterclayton for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

cmd/kubelet/OWNERS
pkg/generated/openapi/OWNERS
pkg/kubelet/OWNERS
pkg/kubelet/apis/config/OWNERS
staging/src/k8s.io/kubelet/config/OWNERS

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

aojea · 2023-11-29T19:37:58Z

if you remove the flags, systems that are deployed today setting the flag, are going to fail to run once they upgrade

@liggitt you have always more experience with all these deprecations, any suggestions?

aojea · 2023-11-30T11:39:05Z

Rule #1: API elements may only be removed by incrementing the version of the API group.

https://kubernetes.io/docs/reference/using-api/deprecation-policy/

Nothing is wrong with removing things that have been deprecated for a while

that is not true, do you have more examples, we have a strong commitment with not breaking users,

cmd/kubelet/app/options/options.go

aojea · 2023-11-30T14:30:19Z

pkg/kubelet/apis/config/scheme/testdata/KubeletConfiguration/after/v1beta1.yaml

@@ -80,7 +80,6 @@ seccompDefault: false
 serializeImagePulls: true
 shutdownGracePeriod: 0s
 shutdownGracePeriodCriticalPods: 0s
-streamingConnectionIdleTimeout: 4h0m0s


IIUIC this is a noop for a long time so this is correct

aojea · 2023-11-30T14:33:00Z

Nothing is wrong with removing things that have been deprecated for a while

[...]

Nothing is wrong with removing things that have been deprecated for a while

that is not true, do you have more examples, we have a strong commitment with not breaking users,

How does this apply to a functionality that is currently broken and non-functional? This command-line option does not do anything at the moment, nor does the kubelet configuration option.

My suggestion here is about something that does not do anything any more. There is no valid way to set an idle timeout for streaming connections using this command-line switch (also true for kubelet configuration option).

We had users opening support cases against OpenShift who set this option and were surprised it does not work.

Should this be a genuine piece of functionality we are removing, then I wouldn't say anything. However, this is not the case here. This is more like a leftover from the dockershim removal time that was never removed since following deprecation a long time ago.

That said, given that the policy is clear about the removal of deprecated functionality and that you argue against removing it, then it's best we follow the guidelines here - yours and the policy. 👍

@kwilczynski let me clarify , I think that removing it is ok, I'm just arguing about how to do it, IIUIC this functionality does not work for the long time so the API change is a noop.

However, if you remove the flag without previous announcement, if you have clusters with that flag set at the upgrade they will break, so I think that at least we should warn about this and add a release note about it so people prepare in advance.

aojea · 2023-11-30T14:33:57Z

/lgtm

one comment about the flag , I think is better if we agree on a release for the flag to be removed so we can inform end users in advance and prepare release notes and so on ...

Thanks

k8s-ci-robot · 2023-11-30T14:34:06Z

LGTM label has been added.

Git tree hash: d0d8a377ebbb9e48c772ab9c2bf91d989afb563d

aojea · 2023-11-30T14:37:11Z

an example of potential end users that will have their cluster broken because we remove a flag without notice https://grep.app/search?q=--streaming-connection-idle-timeout

The option is not used any more and the timeout is handled by the container runtime. Signed-off-by: Sascha Grunert <[email protected]>

k8s-ci-robot · 2023-11-30T15:03:10Z

New changes are detected. LGTM label has been removed.

saschagrunert · 2023-11-30T15:58:50Z

/test pull-kubernetes-conformance-kind-ga-only-parallel

jiahuif · 2023-11-30T17:33:32Z

/triage accepted

k8s-triage-robot · 2024-01-18T23:50:47Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

k8s-ci-robot · 2024-02-15T12:35:52Z

PR needs rebase.

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.

k8s-triage-robot · 2024-05-18T11:44:22Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2024-06-17T12:03:49Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2024-06-17T12:03:54Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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-sigs/prow repository.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/code-generation area/kubelet do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 29, 2023

k8s-ci-robot requested review from liggitt and sjenning November 29, 2023 10:39

saschagrunert force-pushed the stream-idle-timeout branch 2 times, most recently from d3ed164 to 6a1c840 Compare November 29, 2023 11:15

saschagrunert changed the title ~~WIP: Remove StreamingConnectionIdleTimeout kubelet option~~ Remove StreamingConnectionIdleTimeout kubelet option Nov 29, 2023

k8s-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 Nov 29, 2023

saschagrunert changed the title ~~Remove StreamingConnectionIdleTimeout kubelet option~~ WIP: Remove StreamingConnectionIdleTimeout kubelet option Nov 29, 2023

k8s-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 Nov 29, 2023

saschagrunert mentioned this pull request Nov 29, 2023

Exec timeout does not kick users out on kubectl client 1.21 #102569

Open

saschagrunert changed the title ~~WIP: Remove StreamingConnectionIdleTimeout kubelet option~~ Remove StreamingConnectionIdleTimeout kubelet option Nov 29, 2023

k8s-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 Nov 29, 2023

aojea reviewed Nov 30, 2023

View reviewed changes

cmd/kubelet/app/options/options.go Outdated Show resolved Hide resolved

aojea reviewed Nov 30, 2023

View reviewed changes

k8s-ci-robot assigned aojea Nov 30, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2023

Remove StreamingConnectionIdleTimeout kubelet option

f8785fc

The option is not used any more and the timeout is handled by the container runtime. Signed-off-by: Sascha Grunert <[email protected]>

saschagrunert force-pushed the stream-idle-timeout branch from 725e6e6 to f8785fc Compare November 30, 2023 15:03

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2023

k8s-ci-robot requested a review from aojea November 30, 2023 15:03

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 30, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2024

dims added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 18, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 18, 2024

k8s-ci-robot closed this Jun 17, 2024

saschagrunert deleted the stream-idle-timeout branch June 17, 2024 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `StreamingConnectionIdleTimeout` kubelet option #122104

Remove `StreamingConnectionIdleTimeout` kubelet option #122104

saschagrunert commented Nov 29, 2023 •

edited

Loading

k8s-ci-robot commented Nov 29, 2023

kwilczynski commented Nov 29, 2023

k8s-ci-robot commented Nov 29, 2023

aojea commented Nov 29, 2023 •

edited

Loading

aojea commented Nov 30, 2023 •

edited

Loading

aojea Nov 30, 2023

aojea commented Nov 30, 2023

aojea commented Nov 30, 2023

k8s-ci-robot commented Nov 30, 2023

aojea commented Nov 30, 2023

k8s-ci-robot commented Nov 30, 2023

saschagrunert commented Nov 30, 2023

jiahuif commented Nov 30, 2023

k8s-triage-robot commented Jan 18, 2024

k8s-ci-robot commented Feb 15, 2024

k8s-triage-robot commented May 18, 2024

k8s-triage-robot commented Jun 17, 2024

k8s-ci-robot commented Jun 17, 2024

Remove StreamingConnectionIdleTimeout kubelet option #122104

Remove StreamingConnectionIdleTimeout kubelet option #122104

Conversation

saschagrunert commented Nov 29, 2023 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Nov 29, 2023

kwilczynski commented Nov 29, 2023

k8s-ci-robot commented Nov 29, 2023

aojea commented Nov 29, 2023 • edited Loading

aojea commented Nov 30, 2023 • edited Loading

aojea Nov 30, 2023

Choose a reason for hiding this comment

aojea commented Nov 30, 2023

aojea commented Nov 30, 2023

k8s-ci-robot commented Nov 30, 2023

aojea commented Nov 30, 2023

k8s-ci-robot commented Nov 30, 2023

saschagrunert commented Nov 30, 2023

jiahuif commented Nov 30, 2023

k8s-triage-robot commented Jan 18, 2024

k8s-ci-robot commented Feb 15, 2024

k8s-triage-robot commented May 18, 2024

k8s-triage-robot commented Jun 17, 2024

k8s-ci-robot commented Jun 17, 2024

Remove `StreamingConnectionIdleTimeout` kubelet option #122104

Remove `StreamingConnectionIdleTimeout` kubelet option #122104

saschagrunert commented Nov 29, 2023 •

edited

Loading

aojea commented Nov 29, 2023 •

edited

Loading

aojea commented Nov 30, 2023 •

edited

Loading