WIP Introduce v1beta1 API #9178
Conversation
|
@johngmyers: You must be a member of the kubernetes/kops-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Kops Maintainers and have them propose you as an additional delegate for this responsibility. 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. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
Great idea! I wonder if its worth having an umbrella issue to track and discuss desired changes from a high level? before the work is done to implement the changes into a PR. I know I have a few in the back of my head too. |
|
I guess this new API version will be implemented over a number of PRs based on the various changes we'd like to put into them. I suppose some will be more global and requires broader agreement, while some are more local (such as removing all the unused fields in Cilium). Starting with an issue on what we'd like to achieve with this API version would be good. How large are the changes we see here? Is beta the next step or will we change some parts of the spec so much that v1alpha3 makes more sense? Regardless, I certainly welcome a new API version and I think 1.19 is a good time to add this. |
|
Heres some from my list, feel free to paste into an issue. Most are around consistency and some could be done without bumping api versions but I never got around to it. [x] I believe there was discussion around cloud-specific fields and if we could colocate them under a cloud-specific type, too. |
How easy would it be to change the structs? Some of those we have today is overly flat and use prefixes, where their helm counterpart is fairly nested. Nesting could make things more user-friendly and easier to reason about. |
|
When this lands we should decide on a formal policy for making changes to the older api versions - we should either require it (if applicable) or not allow it. the later of which i believe we could enforce with prow. |
|
It is an apimachinery requirement that new fields be added to the old apiversions. This is so they round-trip. I have not found apimachinery rules on removing unsupported fields. I believe fields that are checked by api validation should remain in the unversioned API with a The v1alpha2 API has been around so long I believe we should treat it as requiring at least the beta deprecation period of 3 releases/9 months. We might even treat it as stable and give it 12 months before removing. I don't think the new API should have less than a beta deprecation period either. |
|
So if I understand this correctly, we can either offer a v1alpha3 where we can break whatever, but create script/subcommand that converts the compatible parts from v1alpha2 to v1alpha3, or we can introduce v1beta1, but then have to keep v1alpha2 in sync. Most of the stuff in @rifelpet's first post can be done because it is just renaming the json keys, while the structs can remain as they are today. |
|
@olemarkus I don't think you understand correctly. The only difference between alpha and beta is the minimum length of the deprecation period before we remove the API. For beta it is 3 releases or 9 months, whichever is longer. For alpha it is 0 releases. I don't think either the old or new APIs should be removed immediately. For APIs that still exist we need to maintain round-trip semantics, so new fields have to be added to old api versions and moved fields have to have conversion code for old apis. This is independent on whether they are alpha or beta. |
|
hm, didn't we determine that v1alpha1 was missing some fields before we removed it? was that because the prow jobs weren't enforcing round-trip semantics? |
|
@johngmyers I am not sure we disagree here. I am suggesting some changes that breaks round-trip. But if we can avoid this, we definitely should. I don't want to be in a situation where those fields are with us forever though. But if we remove e.g the deprecated cilium fields from v1beta1, can we have conversion logic that sets the unversioned api values to blank? Or is it possible to remove the deprecated fields from the unversioned API and leave them only in v1alpha1, then have conversion logic that sets these values to blank in v1alpha1? |
|
I'm not entirely sure what the exact rules are for removing fields: the apimachinery documentation is extremely hard to find and the few times I did find it I didn't have time left to read it. As I said before, I think we can simply remove unused, unvalidated fields from the unversioned API. That would allow them to be specified, but they'd be immediately dropped. |
|
Doesn't seem too hard. When one make incompatible changes one have to implement manual conversion, as expected. apimachinery makes some boilerplate functions to make life easier (everything that is still mapped 1:1). See johngmyers/kops@v1beta1...olemarkus:api-v1beta1-cilium for my attempt at doing various changes to the API. |
@rifelpet think this happen a while ago before we had |
|
One wrinkle is that kops writes the unversioned API to "cluster.spec" in the state store. We won't be able to make any incompatible changes to the unversioned API. I'm working on a PR to start migrating to a versioned form of the full cluster spec, also writing the unversioned API to "cluster.spec" but not reading it. Perhaps we do that migration in 1.19 and introduce v1beta1 in 1.20. |
|
I don't know if we should do truly incompatible changes. The ones I did will mean some values will get lost, but only those that were ignored already. So while conversion isn't entirely 1:1, the effect on the cluster is the same. It may be a bit confusing to the users though. Another question: Do we want to change some defaults when we create new api version? E.g flip some of the flags we already tell the user to do because of security. |
|
It would be nice to have roundtrip tests: given a v1alpha2 manifest, convert it to v1beta1 and back and ensure its equal. Also do the same for v1beta to v1alpha2 and back. It seems like this would be a good place for that: https://github.com/kubernetes/kops/tree/master/tests/integration/conversion and we should make sure the manifests include any fields that we're modifying in the new apiVersion. This could be in a followup PR though. |
|
We're going to need a whole kops minor version with the #9229 fix or we're going to be significantly constrained in how we can change the unversioned API. So I think this project will have to wait until kops 1.20. |
|
@johngmyers: You must be a member of the kubernetes/kops-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Kops Maintainers and have them propose you as an additional delegate for this responsibility. 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. |
|
Just out of curiosity, what is an incompatible change? Would any of the changes in my branch be incompatible (the round-trip is not entirely complete for some as the deprecated values will be nulled even if the user set a value. The effect on the cluster is identical though) |
k8s-ci-robot
added
the
lifecycle/rotten
|
/remove-lifecycle rotten |
k8s-ci-robot
removed
the
lifecycle/rotten
k8s-ci-robot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
@johngmyers: 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
|
@johngmyers Can this be closed now that #12406 was merged? |
|
This has lists of things to change in the API, so I'm keeping it open for that. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
@johngmyers: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: 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. |
Introduce the v1beta1 API as a copy of v1alpha2. Subsequent PRs during the kops 1.19 development period are expected to change it.
/milestone v1.19