@kubernetes/sig-network-pr-reviews

Do we have a way of denoting a field as beta when it's not an annotation? By putting this in the v1.Service definition aren't we implying that it's a GA feature?

Not saying we shouldn't, just curious if we can actually claim that this is beta.

@caseydavenport I'm using beta based on these criteria. If no one objects to the field's naming, and given the length of time we've been carrying the annotation with no modifications to the semantics, I'd like to suggest we move it to a field. It is probably stable in reality, but not reserving the right to change the semantics in a backward compatible way over the next release doesn't really buy anything. Also, as users have been using the field in production for some time now, I don't think its endanger of breaking anyone's cluster.

@kow3ns I wasn't claiming it was anything but stable :)

Moving to a field sounds good to me, I was more just curious about this point from the doc you linked:

Object Versioning: API version name contains beta (e.g. v2beta3)

Which isn't true for Services. Does that mean we just go straight to GA?

@caseydavenport There is precedence for moving beta features to fields in v1 objects if we have a high confidence in the API (e.g. PodAffinity/PodAntiAffinity). I don't think the presence of a field automatically promotes it GA in all perspectives, but it means that we will support the field in a backward compatible way and it meets the beta criteria of stability. I will of course deffer to SIG API machinery and the API reviewers on this.

/test pull-kubernetes-federation-e2e-gce

@erictune @janetkuo @enisoc @foxish

/test pull-kubernetes-federation-e2e-gce

@kubernetes/sig-api-machinery-misc @lavalamp field promotion

@kow3ns wrote:

The annotation is considered to be deprecated but will remain available for a full deprecation cycle.

The documented deprecation cycle for an alpha annotation is 0 releases. Since we intend to support it longer than that, we should explicitly say when that is. Is the plan to remove it in 1.9?

The annotation is considered to be deprecated but will remain available for a full deprecation cycle.

The documented deprecation cycle for an alpha annotation is 0 releases. Since we intend to support it longer than that, we should explicitly say when that is. Is the plan to remove it in 1.9?

What do you mean by "remain available"? Trying to represent the same data as both an annotation and as a field is extremely problematic. This has been tried before and wreaks havoc with edit, patch, apply, etc.

@enisoc @liggitt We told users that they could use it even though it's alpha, and we enabled it by default. We have to at least treat it as beta (Beta: 3 months or 1 release (whichever is longer)), and we can't reasonably deprecate it until we release a stable version. We can't just remove it and break them. We need to support it for at least one release cycle and give them time to update their running services, stable helm charts, and examples.

I understand the pain we have had in the past with this, and this PR doesn't attempt to keep the field consistent. Its an either or. If you use the annotation, the field is ignored. If you remove annotation, the value of the field will be used. In 1.8, we will give users a full release cycle to update the field on their running services, update the stable helm charts, and modify their manifests, tutorials, and examples.

@enisoc @liggitt We told users that they could use it even though it's alpha, and we enabled it by default. We have to at least treat it as beta (Beta: 3 months or 1 release (whichever is longer)), and we can't reasonably deprecate it until we release a stable version.

That seems like an untenable approach... agree on what alpha means, ship something under that definition, enable it in one deployment, then force the open source project to maintain beta-level support for it?

If you use the annotation, the field is ignored.

That also seems backwards... an alpha-level annotation should not win over a supported field.

@liggitt by we I mean we the kubernetes developers, told the open source community (e.g. helm maintainers) and other users that it was usable.
When I say enabled it by default, I mean there does not appear to have ever been a configurable feature gate to disable this annotation in any environment.

It's the community, in particular all of the people who are using it that I'm not aware of, that I'm worried about breaking.

@liggitt by we I mean we the kubernetes developers, told the open source community (e.g. helm maintainers) and other users that it was usable.

It was usable as an alpha feature, with alpha guarantees around compatibility. It is a mistake to act like alpha features require support in future versions. That sets a bad precedent, makes people expect and depend on alpha features being supported, and destroys the point of having an alpha level feature in the first place.

If the field is being added, I think we should add release-note-action-required, and note the alpha annotation is no longer honored.

That also seems backwards... an alpha-level annotation should not win over a supported field.

@liggitt my thought about the annotation being present and overriding the field is that during an upgrade from 1.7 to 1.8 the following would occur.

1. The field value defaults to false and the annotation, if present and set to true, overrides the default value allowing the user to maintain the desired behavior across upgrade and rollback scenarios.
2. When users are confident in the 1.8 cluster, they can modify their services to remove the annotation and set the field to true. They may optionally leave the annotation in place in case they should decide to roll back at some later point.
3. During this time helm charts, examples, tutorials will work with either the annotation or the field.
4. During an upgrade to 1.9, we do as you suggest and add the release-note-action-required and a final deprecation notice.

I remember the confusion around storage-class annotations, init-container annotations, and pod-affinity/pod-anti-affinity annotations and have been bitten by it as well. That is why I'm suggesting the above. While it may not set the best precedent, I think it will keep our users from being surprised during an upgrade.

So I still want to move forward with the API change but will remove the annotation based on feedback from @liggitt, @lavalamp, and @bgrant0607. The argument makes sense, and if SIG API concurs that we should stick to our guns wrt to our policy, I'm good with it. However, wrt to #49239. I'd like to fix this in a separate PR as it will require coordination with SIG Network. We need to decide on, and deliver, a compatible DNS implementation prior modifying the endpoints controllers behavior wrt to unreadiness. I propose,

1. promote the annotation in 1.8 concurrently with updating kube-dns to the desired behavior.
2. in 1.9 modify the endpoint controller to the correct behavior, and default to the updated version of kube-dns.

@kow3ns Changing the Endpoints controller behavior in step (2) would a significant change in behavior. I don't think we can do that separately from breaking the alpha API.

@bgrant0607 as per #49239

/retest

/approve

/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bgrant0607, crimsonfaith91, kow3ns

Associated issue: 47880

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

/retest

Automatic merge from submit-queue (batch tested with PRs 50418, 49830, 49206, 49061, 49912)