Skip to content

Conversation

mdbooth
Copy link
Contributor

@mdbooth mdbooth commented Apr 21, 2023

Which issue(s) this PR fixes:
Fixes #1326

Special notes for your reviewer:
This PR relies on behaviour in clusterctl which updates storage versions on upgrade.

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 21, 2023
@netlify
Copy link

netlify bot commented Apr 21, 2023

Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
🔨 Latest commit b1bb18b
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/645275745703e200081b8833
😎 Deploy Preview https://deploy-preview-1527--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 21, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

any chance to remove this too:

# Can be drop once kubernetes-sigs/cluster-api-provider-openstack#1326 is done.
- linters:
- staticcheck
text: "SA1019"
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added as a second commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, CI lint is passing. 👌

Copy link
Contributor

@seanschneeweiss seanschneeweiss left a comment

Choose a reason for hiding this comment

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

Keeping compatibility for that many revisions of the API of course is a pain and should be minimized. So thanks beforehand for working on this.

Two thoughts on this:

  • Looking at CAPI, where they did almost the same for their Alpha versions. CAPI deprecated the API before removing it. kubernetes-sigs/cluster-api#8071. Should we deprecate first and then remove - regardless the fact, that this is an alpha API version? From src:

    Alpha API versions may be removed in any release without prior deprecation notice

  • Should we adapt kubernetes-sigs/cluster-api#8038?
    Especially this part
  • We will set served to false for the apiVersions in all our CRDs.
  • Then users can’t read or write with the old apiVersions anymore.
  • The API server will still be able to read and convert the old apiVersions.
  • If necessary, users can easily enable the apiVersions again by reverting served back to true on the CRD.

Copy link
Contributor

@seanschneeweiss seanschneeweiss left a comment

Choose a reason for hiding this comment

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

This might need modification too:

commonLabels:
cluster.x-k8s.io/v1alpha3: v1alpha3
cluster.x-k8s.io/v1beta1: v1alpha4_v1alpha5_v1alpha6_v1alpha7

@mdbooth
Copy link
Contributor Author

mdbooth commented Apr 24, 2023

Keeping compatibility for that many revisions of the API of course is a pain and should be minimized. So thanks beforehand for working on this.

Two thoughts on this:

  • We will set served to false for the apiVersions in all our CRDs.
  • Then users can’t read or write with the old apiVersions anymore.
  • The API server will still be able to read and convert the old apiVersions.
  • If necessary, users can easily enable the apiVersions again by reverting served back to true on the CRD.

I think this is the important discussion, and I'm ambivalent.

I am inclined to say lets get these 2 specifically gone and do better in the future. The reasons are:

  • We haven't effectively tested these versions in a long time. I have little confidence they still work.
  • Anybody who upgraded to v0.7 is no longer running them anyway.
  • Anybody who skips directly to v0.8 can recover by downgrading to v0.7, waiting for the upgrade to complete, then upgrading to v0.8 again. We need to ensure this is in the 0.8 release notes, though.
  • We're making the continued modification of the current API more burdensome by keeping them.

However, I appreciate this is a bullish view and I could easily be persuaded otherwise.

In the spirit of 'do better in the future', though, I wonder if we should immediately deprecate v1alpha5 and v1alpha6 with a target of setting 'unserved' in v0.9 and removal in 'v0.10'?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2023
@jichenjc
Copy link
Contributor

In the spirit of 'do better in the future', though, I wonder if we should immediately deprecate v1alpha5 and v1alpha6 with a target of setting 'unserved' in v0.9 and removal in 'v0.10'?

is this CAPI or other provider should consider already? we may not be identical to them but reference should be helpful

anyway, I think your statement is reasonable and we want folks to keep upgrade to save effort with lower API version maintainance

@mdbooth
Copy link
Contributor Author

mdbooth commented May 2, 2023

I've added this to the agenda of this week's office hours. Lets make a decision on this by the end of this week at the latest.

@tormath1
Copy link
Contributor

tormath1 commented May 3, 2023

@mdbooth the documentation/readme is not updated at the moment:

## Compatibility with Cluster API and Kubernetes Versions

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2023
@mdbooth mdbooth force-pushed the remove-old-apis branch from c40801b to a07ae30 Compare May 3, 2023 14:43
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2023
@mdbooth mdbooth force-pushed the remove-old-apis branch from a07ae30 to b1bb18b Compare May 3, 2023 14:53
@mdbooth
Copy link
Contributor Author

mdbooth commented May 3, 2023

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth, mnaser, seanschneeweiss

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:
  • OWNERS [mdbooth,seanschneeweiss]

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

@jichenjc
Copy link
Contributor

jichenjc commented May 4, 2023

/lgtm

thanks for the update

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2023
@k8s-ci-robot k8s-ci-robot merged commit 9233490 into kubernetes-sigs:main May 4, 2023
@mdbooth mdbooth deleted the remove-old-apis branch May 4, 2023 06:51
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove v1alpha3/4 ?
6 participants