FlushFrequency config type

[pohly]

What type of PR is this?

/kind bug

What this PR does / why we need it:

The intention for Kubernetes 1.23 was to support flushFrequency: 60 and treat
that as seconds. But time.Duration counts nanoseconds. Because the struct is
embedded inside the beta KubeletConfiguration, we cannot switch to a type that
serializes differently (like metav1.Duration) and therefore (for now) just
update the documentation to match the implementation from Kubernetes 1.23.

Special notes for your reviewer:

This was found in #105797 while working on a JSON unmarshaling unit test for LoggingConfiguration.

Does this PR introduce a user-facing change?

kubelet: The `flushFrequency` field in the kubelet configuration was meant to accept an integer counting seconds, but the actual implementation used nanonseconds. Configuration files that use, for example, `flushFrequency: 60` for 60s must be updated to `flushFrequency: 60000000000`.

[pohly]

/sig instrumentation

[k8s-triage-robot]

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.

[pohly]

This could also be solved with metav1.Duration, albeit without support for flushFrequency: 60. Would it be okay to do that?

It would be a much simpler solution.

[fedebongio]

/cc @jpbetz
/triage accepted

[pohly]

This could also be solved with metav1.Duration, albeit without support for flushFrequency: 60.

I switched to that and updated the PR description.

The number of users of flushFrequency: 60 should be so small, not breaking those does not justify adding an entire new type that doesn't offer that much actual additional functionality. metav1.Duration.UnmarshalJSON could be extended to handle a number as input, but even that looks like overkill.

The original code with a new resource.Duration is in https://github.com/pohly/kubernetes/commits/log-flush-frequency-config-type-resource.

[liggitt]

echoing from slack:

I'm a pretty strong -1 on breaking decoding of fields exposed in beta+ config files.

the main option I see is to add a parallel field named something like flushFrequencyDuration that accepts duration, mark flushFrequency as deprecated, and translate values in that field into seconds in flushFrequencyDuration

changing the existing field to a string would mean anything using 1.23 structs to generate or read/modify config would break on a serialized config containing a string value for that field

[ehashman]

Because the field is used in a beta API, there are beta stability expectations for this field.

[ehashman]

If we change the type, we will break things that expect the API to work as released in 1.23. We should consider instead adding a new field with the desired type and deprecate the old one, e.g. FlushFrequencyDuration as @liggitt suggested.

[pohly]

This will be a future improvement to make the configuration more user-friendly. I want to keep this bug fix PR simple and not spend more time on it than necessary (mine and of reviewers).

So let's make the documentation match the implementation from 1.23. I'll update the PR.

[pohly]

@ehashman: update pushed, please take another look.

[ehashman]

/hold cancel
/kind documentation
/lgtm

/assign @liggitt

[liggitt]

therefore (for now) just update the documentation to match the implementation from Kubernetes 1.23.

There will never be a more convenient or less impactful time to resolve this than now. Given the other discussion about settling on a single stable serializable version of this struct, do we really want to leave this as-is? Marking this field as deprecated, setting it to omitempty so it doesn't get serialized when unset, printing a warning when it is used, and using a peer flushFrequencyDuration field that parses durations as originally proposed seems better long term

[liggitt]

(I don't object to fixing the documentation, but I couldn't tell if that was all that was planned here)

[pohly]

There will never be a more convenient or less impactful time to resolve this than now.

I don't think that this really affects many users in practice. I understand that we must not break serialization because that might break tools even when this feature is not actively set (serialize defaults, restore later), but I doubt that there are many users who need the nicer serialization with a string instead of nanoseconds. Therefore implementing that improvement had a lower priority for me.

Given the other discussion about settling on a single stable serializable version of this struct, do we really want to leave this as-is?

My intention was to tackle this in this order:

• fix the documentation to match the implementation (this PR)
• convert to a single struct (the other PR)
• change the serialization as you proposed

From a compatibility perspective it doesn't matter because we still need to be compatible with the same JSON fromat as in 1.23, but it would be less code that needs to be changed (only one struct).

[pohly]

@liggitt: I've implemented your proposal in https://github.com/pohly/kubernetes/commits/log-flush-frequency-new-field, with commits in the order that I proposed above (first this PR, ready for backporting to 1.23, then the open single-struct API PR for 1.24, and finally the flush frequency enhancement, also for 1.24).

[liggitt]

/lgtm
/approve

for doc fix as first in the chain of updates

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, pohly

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:

• staging/src/k8s.io/component-base/config/OWNERS [liggitt]

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

[pohly]

/retest