RFC: component-base config to beta #105448
Conversation
k8s-ci-robot
added
release-note-none
|
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. |
|
I don't know how much https://github.com/kubernetes/community/blob/master/sig-architecture/api-review-process.md#mechanics applies here. Here are some potentially relevant KEPs:
The second one is relevant because the goal is to add some more fields to The new fields will be alpha. There's precedence for having alpha fields in beta APIs, so this should be okay as long as the new fields are explicitly called out as "alpha" or "experimental" (similar to the alpha log sanitization field). |
|
/retest |
k8s-ci-robot
added
the
needs-rebase
pohly
force-pushed
the
log-configuration-beta
branch
from
14b1806 to
cac38e6
Compare
k8s-ci-robot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pohly The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pohly
force-pushed
the
log-configuration-beta
branch
from
cac38e6 to
7f0b261
Compare
k8s-ci-robot
added
size/L
|
/retest |
|
/remove-sig api-machinery |
k8s-ci-robot
removed
the
sig/api-machinery
|
@fedebongio I saw you removing api-machinery earlier. Sorry for the noise. I brought up these useless labels in ecb3813#r57456677 which then moved to #105414 (comment). |
| @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and | |||
| limitations under the License. | |||
| */ | |||
|
|
|||
| package v1alpha1 | |||
| package v1beta1 | |||
|
|
|||
| import ( | |||
| "time" | |||
There was a problem hiding this comment.
the TODO(#80289) below seems relevant to this promotion... we don't actually want to propagate a recommended default of using endpoints objects for leader election to new consumers
I would recommend dropping the default for the ResourceLock field to force callers to explicitly select the resource type to use (the same way they have to set ResourceNamespace/ResourceName today)
There was a problem hiding this comment.
This is code that I am not familiar with. Same with ResourceLock below. I can try to address these comments, but I am really starting to wonder whether combining all of these unrelated structs in a single config package is the right approach. I find it cleaner to split the package into individual pieces that are aligned with ownership.
Shall I try that as part of this PR or first split it while keeping everything as alpha?
There was a problem hiding this comment.
I would recommend dropping the default for the ResourceLock field to force callers to explicitly select the resource type to use (the same way they have to set ResourceNamespace/ResourceName today)
@liggitt Why?
I think we should default to Lease - I don't see any advantage of choosing anything else. [And if someone really wants to change, they can always do that.]
There was a problem hiding this comment.
The consequences of selecting the wrong one between two versions are really bad (totally breaks leader election). We should make the consumer select the resource type to use and document the recommended one if they're starting fresh.
There was a problem hiding this comment.
OK - if you're worrying about people upgrading the current usage to the new API without looking into it - that makes sense.
In that case leaving it empty and documenting that Lease is the desired lock type and mentioning the migration risk and migration path to it sounds like desired path.
| @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and | |||
| limitations under the License. | |||
| */ | |||
|
|
|||
| package v1alpha1 | |||
| package v1beta1 | |||
|
|
|||
| import ( | |||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |||
There was a problem hiding this comment.
the ResourceLock field needs to document
- the allowed values (
leases,endpointsleases,endpoints,configmapsleases, andconfigmaps) - the recommended value if backwards compatibility with an older config is not required (
leases) - the recommended value if backwards compatibility with an older config using
endpointsorconfigmapsis required (endpointsleasesorconfigmapsleases, respectively)
| @@ -81,8 +81,7 @@ type ClientConnectionConfiguration struct { | |||
| Burst int32 `json:"burst"` | |||
| } | |||
|
|
|||
| // LoggingConfiguration contains logging options | |||
| // Refer [Logs Options](https://github.com/kubernetes/component-base/blob/master/logs/options.go) for more information. | |||
| // LoggingConfiguration contains logging options. | |||
| type LoggingConfiguration struct { | |||
| // Format Flag specifies the structure of log messages. | |||
| // default value of format is `text` | |||
There was a problem hiding this comment.
I'm not sure what to do with the Sanitization field in the config while the feature is still experimental... it doesn't look like the feature is gated, and the config field doesn't indicate it is experimental like the command-line flag does, so it would be possible for someone to have this enabled in a config and not realize they are using an unstable feature (this is true today, I'm just wanting to use this promotion as a check of existing fields)
possibilities:
- drop the field from the beta type
- feature gate the functionality (failing if you try to set to set this to true without the feature enabled)
- rename this to an explicitly experimental field
what do you think?
There was a problem hiding this comment.
it doesn't look like the feature is gated
Based on the comment in https://groups.google.com/g/kubernetes-sig-architecture/c/Nxsc7pfe5rw/m/Sw6FAj57BAAJ a specific feature gate for log sanitization wouldn't have been needed, but hiding it behind a common "logging feature gate" gate would have had value. Neither was done, but it's not too late to add one.
the config field doesn't indicate it is experimental like the command-line flag does
There is an [Experimental] tag in
The field name itself could have included "experimental" to make it even clearer.
possibilities:
- drop the field from the beta type
This is the same as removing the feature because the KubeletConfiguration and thus command line only supports one version of the struct. If that version then doesn't have the field, it cannot be enabled.
We would have to keep v1alpha1 with the field and add v1beta1 without it, then in kubelet stick with v1alpha1. In practice, we want the alpha features in all commands, so nothing would use v1beta1 and we can delay the promotion until all fields are beta quality and v1alpha1 can be removed.
- feature gate the functionality (failing if you try to set to set this to true without the feature enabled)
I prefer that. For CPU manager, CPUManagerPolicyOptions was added, but gates for alpha and beta were also discussed. In this case, we could add AlphaLoggingConfiguration as feature gate with alpha as permanent state and put several things behind it (for now):
- sanitization
- JSON output
- the upcoming JSON options for buffering
Then when graduating some of them to beta, we add BetaLoggingConfiguration and check for that for those features.
When GA, they are always enabled.
However, when we do this, why bother with versioning the LoggingConfiguration API at all? What does the version indicate, and to whom? End-users are not seeing it and cannot use it to choose a specific API version. Marking it as alpha could have warned our fellow developers that using it in a beta struct might not be a good idea, but that hasn't really worked either...
So if we add such a feature gate, we should keep v1alpha1 and do the graduation only when all fields are beta. Hmm, but what if we then add a new alpha field? We would have to re-introduce v1alpha1. That's a lot of back-and-forth for little gain.
I'm leaning towards removing the entire versioning unless someone can explain to me what it is good for and why that justifies the extra work.
Perhaps if the LoggingConfiguration was used with its own TypeMeta then it would make sense, but that's not what these structs are meant for, right?
- rename this to an explicitly experimental field
I prefer the feature gate approach because it causes less churn for users (compared to having to rename fields in a config file: AlphaSanitization -> BetaSanitization -> Sanitization).
There was a problem hiding this comment.
I just wanted to mention that i started advocating against encoding the maturity of a feature in the names of annotations, flags, fields, etc. It is due to the breaking changes on graduations.
Users who use an experimental feature should know about the maturity from documentation - flag descriptions, go docs. Just using a feature without reading its documentation is not advisable, if copied from examples online.
There was a problem hiding this comment.
I guess v1alpha1.LoggingConfiguration and a separate v1beta1.LoggingConfiguration with less fields (just those which are beta) would have made some sense if the former had been used in v1alpha1.KubeletConfiguration and the latter in v1beta1.KubeletConfiguration. But there is no v1alpha1.KubeletConfiguration...
There was a problem hiding this comment.
One problem with feature gates is when the configuration enables a feature, either by default or because of user selection, and the feature gate disables it. Should that trigger a warning?
As an example, suppose that log sanitization was beta and enabled by default. Now kubelet gets run with
---logging-sanitization=true --feature-gates BetaLoggingConfiguration=false
The default value is set before parsing feature gates, so LoggingConfiguration.Sanitization will be true. The code which validates it will run after parsing feature gates, but it cannot complain about its own default value, right? So validation cannot emit an error. Later the code which uses the field can ignore it, but should that then trigger a warning?
That the user explicitly enabled the feature is not tracked, so it has to be treated like the default case.
There was a problem hiding this comment.
I'm leaning towards removing the entire versioning unless someone can explain to me what it is good for and why that justifies the extra work.
I prototyped that here to get a better feeling for what that would look like:
Introducing feature gates is not part of that yet, but that would be the goal before merging.
I find that new code structure a lot cleaner, but of course I am biased 😁
-
everything related to logging is under component-base/logs;
previously this was scattered across different packages and
different files under "logs" (why some code was in logs/config.go
vs. logs/options.go vs. logs/logs.go never became clear to me):-
long-term config and command line API are clearly separated
into the "api" package underneath that -
logs/logs.go itself only deals with legacy global flags and
logging configuration
-
-
removal of separate Go APIs like logs.BindLoggingFlags and
logs.Options
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
@pohly: 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-ci-robot
added
the
needs-rebase
pohly
changed the title
|
/priority backlog |
k8s-ci-robot
added
priority/backlog
|
Is this PR still needed, please rebase if so (or we can close it?) |
|
It's on hold, waiting for a decision about #105797. Some parts of it are still relevant even when that PR is merged. |
|
closing in favor of #105797 /close |
|
@liggitt: Closed this PR. In response to this:
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. |
What type of PR is this?
/kind cleanup
/kind api-change
What this PR does / why we need it:
In practice, these structs already became beta when they were embedded in other
structs that themselves are presented as beta to users (kube-scheduler v1beta2,
kubelet v1beta1) because to them it is not visible that some parts of the
overall config is still alpha.
Special notes for your reviewer:
This could have been addressed by calling out entire subsets (like kubelet "logging") as experimental, but wasn't done:
While at it, docs get improved bit.
Does this PR introduce a user-facing change?