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

add deprecation warnings for all cloud providers #69171

Merged

Conversation

andrewsykim
Copy link
Member

What this PR does / why we need it:
Adds a deprecation warning for all cloud providers. Please note that this is only a deprecation warning, there are no expected code changes from doing this. Once all providers have external cloud controllers that are stable and widely used we will fully disable provider functionality, but we don't anticipate that for maybe another year.

Release note:

add deprecation warning for all cloud providers

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 27, 2018
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 27, 2018
@andrewsykim
Copy link
Member Author

cc @kubernetes/sig-cloud-provider @cheftako @dims @d-nishi @frapposelli @hogepodge @justaugustus @khenidak @mcrute (sorry if I missed anyone else).

@dims
Copy link
Member

dims commented Sep 27, 2018

/lgtm
/hold

(please feel free to cancel the hold when folks have given their thumbs up Andrew)

@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. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 27, 2018
@justaugustus
Copy link
Member

@andrewsykim -- I have a preference towards ...will be removed in a future release over ...will be removed in the near future.

@hogepodge
Copy link

I recommend the following actions:

  • reaching out to all providers to make sure their deprecation messages are accurate
  • back-porting messages to next stable 1.12 release to capture most accurate information (especially in light of some of the negative feedback we got to 1.12 deprecation warnings).

@neolit123
Copy link
Member

/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 27, 2018
@@ -40,10 +40,14 @@ var (
external bool
detail string
}{
{"openstack", true, "https://github.com/kubernetes/cloud-provider-openstack"},
{"photon", false, "The Photon Controller project is no longer maintained."},
{"aws", false, "The AWS provider is deprecated and will be removed in the near future"},
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be worth either adding the KEP link or the sig cloud-provider link for background/details.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we should tie KEP locations to code.

Choose a reason for hiding this comment

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

Thinking about how the deprecation works, if the goal is to deprecate every provider, we can enforce that in code more strongly. As it stands we opt-in to deprecation, provide a flag that indicates if an external provider is available, then send either the same message (aside from name) or point to a new repository.

Taking the DRY principle, for 1.13 I'm thinking the loader should automatically signal a deprecation with the named provider, and offer an external location if it is configured. In this way we guarantee a minimum deprecation warning, with the option to specify a migration path.

@yastij
Copy link
Member

yastij commented Sep 27, 2018

Mostly lgtm, we might want to point users to the kep or something that contains our timeline for moving out-of-tree

@hogepodge
Copy link

Thinking about the message, as a sig are we willing to put a date on earliest removal? "This code is deprecated and may be removed at the 1. release or later" where is sufficiently far enough in the future to give us time to do it properly.

@yastij
Copy link
Member

yastij commented Sep 28, 2018

If we do have a timeline, we do have to warn users about it.

@frapposelli
Copy link
Member

Should we consider the Kubernetes deprecation policy for this?

Technically, cloud controller manager is still a beta feature in 1.12, if it graduates to GA in 1.13, following the deprecation policy, we should wait 3 releases or 12 months (whichever is longer) before removing the cloud provider code from k/k.

In the context of this specific PR, I echo @justaugustus comment, we should reword the deprecation note with ...will be removed in a future release, and maybe change the message with the explicit version once the deprecation policy kicks in.

@d-nishi
Copy link

d-nishi commented Sep 28, 2018

a) completely on board with adding the deprecation warning;
b) agree with @frapposelli on adopting deprecation period/policy from kubernetes aka 3 releases (or) 12 months before removing the code from k/k;
c) this might require us to initiate the deprecation warning/period as a governance requirement as soon as the alpha of the out-of-tree ccm is available for majority of the providers.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2018
@andrewsykim
Copy link
Member Author

Reworded to ...removed in a future release. I agree that it would be nice to put a concrete release where we will remove providers, but we're learning that removing providers from in-tree is really complicated/difficult and don't want to commit to anything here yet until we have a better understanding of the scope. Having said that, if we're all in agreeance to add a release to make a stronger case here (maybe 1.16, 3 releases from 1.13), and adjust as we go, I'm cool with that too. Thoughts?

@d-nishi
Copy link

d-nishi commented Sep 28, 2018

assuming alpha is 1.13 for most, 1.16 for final deprecation makes sense with the possibility to extend to 1.17.

@andrewsykim
Copy link
Member Author

1.16 for final deprecation makes sense with the possibility to extend to 1.17.

Works for me! Any objections?

@justaugustus
Copy link
Member

Not sure I agree. Are we saying that the "deprecation clock" starts with a out-of-tree CP in alpha? I'd imagine that we'd instead start the clock closer to beta or GA phase. The timeline also makes an assumption that it will only take one release cycle to move through to the next phase, which is not strictly true.

That said, I don't think we need to nail down specific timelines until there's a wider discussion with SIG Arch. The language as it exists in this PR is ambiguous enough to suit the current case.

/lgtm
(SIG Azure)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, dims, justaugustus

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:

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

@d-nishi
Copy link

d-nishi commented Sep 28, 2018

@justaugustus -- good point and I agree. we can start the clock post beta in 1.14 or 1.15 and close after 3 releases aka 1.17 or 1.18

@cheftako
Copy link
Member

Worth remembering that the deprecation clock is how long we have to wait to be allowed to do the delete, not when we are forced to do the delete. If we start the clock now, we would be allowed to delete in 1.16. However we could still do the delete later than that. So deprecating early is just giving more warning notice to everyone.

@justaugustus
Copy link
Member

justaugustus commented Sep 28, 2018

Agreed that having a wider period is preferable, but I still feel that that clock shouldn't start until beta phase.

@andrewsykim
Copy link
Member Author

andrewsykim commented Sep 28, 2018

Okay, but seems like we all agree that having some sort of warning in place sooner is better than later. I will follow up to add that information once we have a better idea. Also important to note, the CCM is in beta currently, we just need more providers adopting in production environments.

Thanks all!

/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 Sep 28, 2018
@k8s-ci-robot k8s-ci-robot merged commit 4b12761 into kubernetes:master Sep 29, 2018
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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants