Introduce a formal policy for maintaining cloudproviders #5198

MaciekPytel · 2022-09-19T12:21:40Z

This is an initial proposal for a policy on cloudprovider maintenance discussed in a recent sig-meeting.

The policy largely codifies what we've already been doing for years (including the requirements we've already imposed on new providers). It also introduces a new 'cloudprovider maintenance request' mechanism, following the general idea previously discussed in the sig-meeting.

Which component this PR applies to?

cluster-autoscaler

What type of PR is this?

/kind documentation

Does this PR introduce a user-facing change?

NONE

This policy is only relevant for CA / cloudprovider developers and not its users.

MaciekPytel · 2022-09-19T12:53:54Z

/hold
Holding to avoid any accidental merge before the discussion is done.

sig-leads for approval: @gjtempleton @mwielgus

cc: @ericrrath @jackfrancis @piepmatz @gajicdev @7fELF @ringtail @jtherin @elmiko @Andrius521 @nilo19 @ellistarn @gjtempleton @davidjumani @birdiesanders @DMajrekar @kevin-wangzefeng @dbonfigli @ddymko @thirdeyenick @displague @andrewsykim @zalmarge @schegi @enxebre @d-mo @alphajc @pawelkuc @RealHarshThakur @towca @jaypipes @drmorr0 @avorima @detiber @Sh4d1 @feiskyer @vishalanarase @PhilippeChepy @Jeffwan @OriHoch @louisportay @hardikdr @jlamillan @hello2mao @pierre-emmanuelJ @x13n @mrajashree @tghartland @remyleone @shysank @RainbowMango @LKaemmerling @ctrox @cprivitere @4ND3R50N @v-pap @ArturasRa @deitch @arunmk @aleksandra-malinowska @MorrisLaw @marwanad

MaciekPytel · 2022-09-19T14:05:10Z

At least the github UI doesn't seem to be rendering some of the logins in the comment above correctly (is there maximum logins per line limit?) and I'm not sure if the notifications went out. Just in case:

cc: @RainbowMango @LKaemmerling @ctrox @cprivitere @4ND3R50N @v-pap @ArturasRa @deitch @arunmk @aleksandra-malinowska @MorrisLaw @marwanad

deitch · 2022-09-19T14:09:26Z

I don't think I got the alert for this, so thanks for @ cc'ing after.

This all looks good, glad to see it somewhat formalized.

The out-of-tree provider appears to be almost an after-thought, or a fallback. Is there any desire to move it entirely to out-of-tree, the same way, e.g. CCM is done?

MaciekPytel · 2022-09-19T15:57:55Z

Thanks for confirming my suspicions on notification not working @deitch, I'll make sure to split it into multiple lines next time.

Re: external provider:

No plans to remove in-tree providers. There are things you can do in-tree that you can't do over grpc such as implementing custom processors to be used alongside your cloudprovider. Also, as far as I know, no one tried to run externalgrpc provider in a cluster with thousands of nodes and I doubt it would work. For comparison, it took us years to get GCE provider to reliably work at 5k node scale. This is purely a guess, but I'd imagine the proto would need to be updated to allow batching requests and the CA side of implementation would likely need much more caching and optimization. Given that all the very large clusters I know of run on existing in-tree providers, I don't know if anyone will be willing to invest that much effort into optimizing externalgrpc.
Based on the above, my recommendation to anyone building a new provider would be to start with externalgrpc if they just want to get it working with minimal effort. However, if you want some advanced, customized functionality and/or thousands of nodes scale and your're ready to invest a few months of fulltime effort, you're probably better off with in-tree.
I think you're right on externalgrpc sounding like a fallback / secondary option in the doc though, which is not what I intended. I'll update the doc to make external the first option. Thanks for the suggestion!

elmiko

overall looks good to me, i have a couple comments but no major changes.

elmiko · 2022-09-19T17:02:29Z

cluster-autoscaler/cloudprovider/POLICY.md

+    by kubernetes/kubernetes repository and the same version of the library is
+    used by CA and Kubernetes. This requirement is mainly driven by
+    the problems with version conflicts in transitive dependencies we've
+    experienced in the past.


is it worth mentioning that cloudproviders are welcome to carry dependencies in their directory if they need?

elmiko · 2022-09-19T17:05:02Z

cluster-autoscaler/cloudprovider/POLICY.md

+## Cloudprovider maintenance requirements
+
+In order to allow code changes to Cluster Autoscaler that would require
+non-trivial changes in cloudproviders this policy introduces _Cloudprovider
+maintenance request_ (CMR) mechanism.
+
+ * CMR will be issued via a github issue tagging all
+   cloudprovider owners and describing the problem being solved and the changes
+   requested.
+ * CMR will clearly state the minor version in which the changes are expected
+   (ex. 1.26).
+ * CMR will need to be discussed on sig-autoscaling meeting and approved by
+   sig leads before being issued. It will also be announced on sig-autoscaling
+   slack channel and highlited in sig-autoscaling meeting notes.
+ * A CMR may be issued no later then [enhancements
+   freeze](https://github.com/kubernetes/sig-release/blob/master/releases/release_phases.md#enhancements-freeze)
+   of a given Kubernetes minor version.
+
+Cloudprovider owners will be required to address CMR or request an exception via
+the CMR github issue. A failure to take any action will result in cloudprovider
+being considered abandoned and marking it as deprecated as described below.
+
+### Empty maintenance request
+
+If no CMRs are issued in a given minor release, core maintainers will issue an
+_empty CMR_. The purpose of an empty CMR is to verify that cloudprovider owners
+are still actively maintaining their integration. The only action required for
+an empty CMR is replying on the github issue. Only one owner from each
+cloudprovider needs to reply on the issue.
+
+Empty CMR follows the same rules as any other CMR. In particular it needs to be
+issued by enhancements freeze.


i think it would be nice to bring this part on a separate review since it proposes a new process.

elmiko · 2022-09-19T17:05:40Z

cluster-autoscaler/cloudprovider/POLICY.md

+### Cloudprovider deprecation and deletion
+
+If cloudprovider owners fail to take actions described above, the particular
+integration will be marked as deprecated in the next CA minor release. A
+deprecated cloudprovider will be completely removed after 1 year as per
+[Kubernetes deprecation
+policy](https://kubernetes.io/docs/reference/using-api/deprecation-policy/#deprecating-a-feature-or-behavior).
+
+A deprecated cloudprovider may become maintained again if the owners become
+active again or new owners step up. In order to regain maintained status any
+outstanding CMRs will need to be addressed.


i might bring this on a separate review as well, but i think it's so useful to have that we should include in the primary pr.

I've left a version of it, but moved part specific to CMR to a separate PR.

jaypipes

Thanks @MaciekPytel for this. I know I haven't been able to attend many SIG meetings this year and that the AWS cloud provider has a number of open issues/PRs on it. Will try to do better at filling these gaps.

Proposal looks reasonable and fair to me.

jaypipes · 2022-10-10T13:19:31Z

cluster-autoscaler/cloudprovider/POLICY.md

+Cloudprovider owners will be required to address CMR or request an exception via
+the CMR github issue. A failure to take any action will result in cloudprovider
+being considered abandoned and marking it as deprecated as described below.


👍 seems fair to me.

k8s-ci-robot · 2022-10-10T13:21:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaypipes, MaciekPytel

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:

~~cluster-autoscaler/OWNERS~~ [MaciekPytel]

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

jlamillan · 2022-10-11T00:12:25Z

cluster-autoscaler/cloudprovider/POLICY.md

+built in.
+
+An external cloudprovider implementation doesn't live in this repository and is
+not a part of CA image. As such it is also not a subject to this policy.


Typo. Should read:

As such it is also not subject to this policy.

not

As such it is also not a subject to this policy.

x13n · 2022-11-02T12:46:07Z

Could we make it a part of the policy to have non-empty approvers AND reviewers sections? I just sent out #5287 to update one of the OWNERS files, but if we require it, it should be here.

The policy largely codifies what we've already been doing for years (including the requirements we've already imposed on new providers).

MaciekPytel · 2022-12-05T14:53:25Z

@x13n, @deitch - addressed your comments. I've also split the CMR part into a separate PR as per @elmiko comment. This one is now essentially writing down what we've been doing for a while anyway.

elmiko

/lgtm

thanks @MaciekPytel

x13n · 2022-12-09T20:08:22Z

Can we cancel the hold on this one now?

gjtempleton · 2022-12-12T15:09:44Z

/lgtm

MaciekPytel · 2022-12-14T11:06:08Z

/hold cancel

This has been discussed multiple times on sig meeting now and it's been lgtm-ed by sig lead, so I think it's fine to let it merge

k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 19, 2022

k8s-ci-robot requested review from feiskyer and x13n September 19, 2022 12:22

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 19, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 19, 2022

MaciekPytel force-pushed the cp_policy branch from 1124070 to 7ac5564 Compare September 19, 2022 12:55

elmiko reviewed Sep 19, 2022

View reviewed changes

jbartosik added the area/cluster-autoscaler label Sep 26, 2022

x13n mentioned this pull request Sep 30, 2022

Clean up stale OWNERS #5228

Merged

jaypipes approved these changes Oct 10, 2022

View reviewed changes

jlamillan reviewed Oct 11, 2022

View reviewed changes

Introduce a formal policy for maintaining cloudproviders

5238cbe

The policy largely codifies what we've already been doing for years (including the requirements we've already imposed on new providers).

MaciekPytel force-pushed the cp_policy branch from 7ac5564 to 5238cbe Compare December 5, 2022 14:50

MaciekPytel mentioned this pull request Dec 5, 2022

Introduce Cloudprovider Maintenance Request policy #5350

Merged

elmiko reviewed Dec 5, 2022

View reviewed changes

k8s-ci-robot assigned elmiko Dec 5, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2022

k8s-ci-robot assigned gjtempleton Dec 12, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2022

k8s-ci-robot merged commit c296a14 into kubernetes:master Dec 14, 2022

vishalanarase mentioned this pull request Feb 23, 2023

Updated the owners of civo cloudprovider #5530

Closed

This was referenced Jun 7, 2023

Updated the owners of civo cloudprovider #5840

Closed

Updated the owners of civo cloudprovider #5841

Merged

dbonfigli mentioned this pull request Oct 9, 2023

Updated the owners of externalgrpc cloudprovider #6183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a formal policy for maintaining cloudproviders #5198

Introduce a formal policy for maintaining cloudproviders #5198

MaciekPytel commented Sep 19, 2022

MaciekPytel commented Sep 19, 2022

MaciekPytel commented Sep 19, 2022

deitch commented Sep 19, 2022

MaciekPytel commented Sep 19, 2022

elmiko left a comment

elmiko Sep 19, 2022

MaciekPytel Dec 5, 2022

elmiko Sep 19, 2022

elmiko Sep 19, 2022

MaciekPytel Dec 5, 2022

jaypipes left a comment

jaypipes Oct 10, 2022

k8s-ci-robot commented Oct 10, 2022

jlamillan Oct 11, 2022

MaciekPytel Dec 5, 2022

x13n commented Nov 2, 2022

MaciekPytel commented Dec 5, 2022

elmiko left a comment

x13n commented Dec 9, 2022

gjtempleton commented Dec 12, 2022

MaciekPytel commented Dec 14, 2022

Introduce a formal policy for maintaining cloudproviders #5198

Introduce a formal policy for maintaining cloudproviders #5198

Conversation

MaciekPytel commented Sep 19, 2022

Which component this PR applies to?

What type of PR is this?

Does this PR introduce a user-facing change?

MaciekPytel commented Sep 19, 2022

MaciekPytel commented Sep 19, 2022

deitch commented Sep 19, 2022

MaciekPytel commented Sep 19, 2022

elmiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaypipes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x13n commented Nov 2, 2022

MaciekPytel commented Dec 5, 2022

elmiko left a comment

Choose a reason for hiding this comment

x13n commented Dec 9, 2022

gjtempleton commented Dec 12, 2022

MaciekPytel commented Dec 14, 2022