subHostPath is only used inside the if block.

Or

labeledInfo := getContainerInfoFromLabels(status.Labels)

if utilfeature.DefaultFeatureGate.Enabled(features.TerminationLogFileCleanup) {
    containerPath, ok := status.Annotations[containerTerminationMessagePathLabel]
    if ok {
        subHostPath := filepath.Join(config.DefaultKubeletPodsDirName, string(labeledInfo.PodUID), config.DefaultKubeletContainersDirName, labeledInfo.ContainerName)
        for ...
        ...
    }
}

#121181 (comment)
I am very sorry, this comment was mentioned before, but I did not process it because it has been a while. That's my fault. I will address it right now.

Could there be a race condition here? You’re concurrently appending to a slice from multiple goroutines.

good catch, I will fix it soon

Starting with Go 1.22, the loop variable i in each iteration becomes an independent variable. You no longer need to pass i as a parameter into closures.
ref: https://tip.golang.org/doc/go1.22#:~:text=Previously%2C%20the%20variables,in%20Go%201.21.

+ In Go 1.22, each iteration of the loop creates new variables, to avoid accidental sharing bugs.

I will delete the index variable for closure.

This PR changes the behavior in a backward-incompatible manner, which effectively means we're deprecating an existing behavior (since the old behavior has been modified). You should utilize the deprecated feature gate instead, and since deprecated gates have shorter lifecycles, we can remove it more rapidly.

Ref: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/feature-gates.md#deprecation

The current modification involves minimal changes and effectively prevents the issue of inode resource exhaustion caused by lots of termination files. I believe this feature gate should be removed as soon as possible indeed, so I am inclined to use this feature gate as deprecated. Thanks a lot.

@HirazawaUi PTAL. Thanks a lot :)

Our goal is to remove the version comments from this file. Could you please delete this line?

For deprecated feature gates, we set them to false by default. When the feature gate is disabled (false), we deprecate the previously stable behavior. If this causes issues with users' workloads, they can toggle the gate to true to revert to the original behavior. Therefore, the condition should be adjusted to:
if !utilfeature.DefaultFeatureGate.Enabled(features.TerminationLogFileCleanup)

If you change it like that, then the feature gate name must become the opposite, too. Something like "KeepTerminationLogFile": false (default) means "don't delete it", true means "clean up".

I might be more inclined to support renaming the feature gate, as this aligns better with our definition of "Deprecated" and remains consistent with most other deprecated gates.

Ref: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/feature-gates.md#deprecation

By the way, I think you meant that when KeepTerminationLogFile is false (the default), we clean up the files, and when KeepTerminationLogFile is true (manually modified), we keep the log files(current behavior), right?

Yes, I had it backwards.

This also means we don't need to define this variable here, as the feature gate is enabled by default.

Isn't it disabled by default?

Or am I reading {Version: version.MustParse("1.33"), Default: false, PreRelease: featuregate.Deprecated}, wrong?

Ah, apologies, my brain must have frozen while typing that. What I meant to say is: The deprecated behavior is currently enabled by default, so defining this variable shouldn't be necessary.

Detailed explanation:
As mentioned in #121181 (comment)

We typically set deprecated gate to false, and use code control like if !utilfeature.DefaultFeatureGate.Enabled(features.TerminationLogFileCleanUp) to make the code enabled by default. If users experience workload disruptions due to this change, they can set the feature gate to true to restore previous behavior.

Therefore, defining this variable here appears unnecessary since the deprecated code path is already active by default. WDYT?

With https://github.com/kubernetes/kubernetes/pull/121181/files#r1978448921 it starts to make a bit more sense 😄

But then the name becomes confusing: https://github.com/kubernetes/kubernetes/pull/121181/files#r1979432133

Last but not least, not all clusters use the default settings. That's what this test feature here is for. Where will the "kubelet should delete termination message log files when container is removed" test run in the future once this PR is merged?

Is there a test for the opposite behavior? There should be one.

That means you will need two test features: one for "feature is on", another for "feature is off". Look at the description of some other entries in this file and describe them accordingly ("When this label is added to a test, the cluster must be ....").

Regarding the Deprecated feature gate, it appears we haven't previously provided test cases for both enabling and disabling directions. If we need to add tests for both enabling/disabling states, we would have to create a test job in test-infra specifically for this feature gate.

This means someone would need to maintain it, and ultimately remove the test job from test-infra when the feature gate is eventually removed. I'm uncertain if this effort is justified, especially considering we lack formal documentation outlining the correct approach for handling such scenarios.

Also don't need Feature gate variable.

Also don't need Feature gate variable.

@HirazawaUi Sorry, I've been quite busy with work recently. Let me summarize the discussion above:

1. The current feature gate name needs to be changed to better align with the semantics of deprecated feature gates having default false values, such as renaming it to KeepTerminationLogFile.
2. We should then cover scenarios in e2e test cases where this feature gate is enabled versus disabled (default disabled).
   Is that correct?

Yes, that's correct, this also means you'll need to add a test job for this feature gate in the test-infra project. Otherwise, we won't be able to run tests for this feature gate (disabling is our default behavior, but enabling requires configuring kubelet with this feature gate enabled, which existing jobs don't cover).

However, I'm uncertain whether this is worthwhile, @pohly is the specialist on this topic and can provide more accurate guidance.

We have jobs that enable alpha/beta features. There will be more guidance in 1.34 #131040

Right now this change is causing the test suite to hang by the looks of all the failures, otherwise you'd see test runs for alpha/beta feature gate labeled tests in https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/121181/pull-kubernetes-e2e-kind-alpha-beta-features/1904902893534711808 for example

If your test depends on a feature gate it should pass the featuregate and we have common jobs to enable featuregates (and will be updating them soon), WithFeatureGate is appropriate for any test that is linked to a feature gate.

If your test depends on something else being setup (like installing a LoadBalancer controller, some special config like IPv6DualStack) then you need a e2e test WithFeature "feature".

If your test depends on a feature gate it should pass the featuregate and we have common jobs to enable featuregates (and will be updating them soon), WithFeatureGate is appropriate for any test that is linked to a feature gate.

If your test depends on something else being setup (like installing a LoadBalancer controller, some special config like IPv6DualStack) then you need a e2e test WithFeature "feature".

get it!

Yes, that's correct, this also means you'll need to add a test job for this feature gate in the test-infra project. Otherwise, we won't be able to run tests for this feature gate (disabling is our default behavior, but enabling requires configuring kubelet with this feature gate enabled, which existing jobs don't cover).

However, I'm uncertain whether this is worthwhile, @pohly is the specialist on this topic and can provide more accurate guidance.

gently /ping @pohly
Is it necessary to cover the scenario where the deprecated feature is set to true in e2e tests?

Is it necessary to cover the scenario where the deprecated feature is set to true in e2e tests?

The feature owner is responsible to ensure that there is sufficient test coverage, not SIG Testing.

In general, if there is a feature gate that users can flip, then there should be tests somewhere which cover both configurations. They don't need to be E2E tests. Running kubelet with different configurations is easier in E2E node tests.