Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear values for disabled alpha fields #50924

Merged
merged 1 commit into from Sep 3, 2017

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Aug 18, 2017

Fixes #51831

Before persisting new or updated resources, alpha fields that are disabled by feature gate must be removed from the incoming objects.

This adds a helper for clearing these values for pod specs and calls it from the strategies of all in-tree resources containing pod specs.

Addresses kubernetes/community#869

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 18, 2017
@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 18, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 18, 2017
@liggitt
Copy link
Member Author

liggitt commented Aug 18, 2017

cc @kubernetes/sig-api-machinery-pr-reviews @kubernetes/api-reviewers

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 18, 2017
@liggitt liggitt added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Aug 18, 2017
@liggitt liggitt added this to the v1.8 milestone Aug 18, 2017
@liggitt liggitt assigned lavalamp and deads2k and unassigned justinsb and juanvallejo Aug 18, 2017
@ncdc
Copy link
Member

ncdc commented Aug 21, 2017

Ouch... For once, I think back to my Java days with annotations and AOP. It sure would be nice to annotate a field as alpha and have a cross-cutting concern apply logic consistently everywhere. /sigh

@mml
Copy link
Contributor

mml commented Aug 21, 2017

cc @jpbetz

@lavalamp
Copy link
Member

This looks to be a hand-written version of something that would be covered by the field gate proposal: https://docs.google.com/document/d/1wuoSqHkeT51mQQ7dIFhUKrdi3-1wbKrNWeIL4cKb9zU/edit

I've glanced at this and I'm worried it will evolve into something unmaintainable. Actually I think this is already unmaintainable. How important is it for these to be cleared?

@liggitt
Copy link
Member Author

liggitt commented Aug 22, 2017

This looks to be a hand-written version of something that would be covered by the field gate proposal

Yes. In the absence of field gate support, this is what we get for 1.8. I'm happy to remove this as soon as an automated/generated solution is available.

Actually I think this is already unmaintainable

It's certainly ugly and not my first preference, but I don't see field gates landing in the next week.

How important is it for these to be cleared?

If we don't clear these, requests to the server that include these fields will be rejected, which is not what we want to happen (a server that disables the alpha field should drop the field from the incoming request, per kubernetes/community#869 (comment)). That would mean that if these fields graduate to beta and start getting used in a future release, a client submitting a manifest with those fields would get these responses:

  • 1.7 server would drop the fields
  • 1.8 server would reject the request
  • 1.9 server would accept the request

1.7 and 1.8 should behave identically

@bgrant0607
Copy link
Member

If we don't clear the fields, then we don't solve one of the main problems with annotations that we wanted to solve by moving away from annotations. (#30819)

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 23, 2017
@liggitt
Copy link
Member Author

liggitt commented Aug 25, 2017

/retest

@lavalamp
Copy link
Member

I agree with the intent but I still feel like this is adding technical debt rather than removing it. Also, the chance that everyone making such a field remembers to clear it like this seems quite low. Unfortunately I don't have a better suggestion.

@liggitt
Copy link
Member Author

liggitt commented Aug 29, 2017

I agree with the intent but I still feel like this is adding technical debt rather than removing it. Also, the chance that everyone making such a field remembers to clear it like this seems quite low. Unfortunately I don't have a better suggestion.

I don't disagree, but I also couldn't come up with anything better for the 1.8 timeframe. Inaction leaves 1.8 in a state we don't want.


// DropDisabledAlphaFields removes disabled fields from the pod spec.
// This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a pod spec.
func DropDisabledAlphaFields(podSpec *api.PodSpec) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to drop the resource field as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, that's a key/value in an existing field... these are new fields

@liggitt
Copy link
Member Author

liggitt commented Sep 1, 2017

need to make a decision on this or provide an alternative

@smarterclayton
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2017
@smarterclayton
Copy link
Contributor

smarterclayton commented Sep 1, 2017 via email

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 51831

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@liggitt liggitt added the kind/bug Categorizes issue or PR as related to a bug. label Sep 3, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 51682, 51546, 51369, 50924, 51827)

@k8s-github-robot k8s-github-robot merged commit f9a82dd into kubernetes:master Sep 3, 2017
@liggitt liggitt deleted the alpha-fields branch September 4, 2017 14:58
k8s-github-robot pushed a commit that referenced this pull request Sep 5, 2017
Automatic merge from submit-queue (batch tested with PRs 51180, 51893)

Clear alpha MountPropagation fields.

This is leftover from #50924, mount propagation introduced a new field that needs to be cleared.

**Which issue this PR fixes**
fixes #51738

**Release note**:

```release-note
NONE
```


@k8s-mirror-api-machinery-pr-reviews 
/assign @liggitt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet