[AEP] docs: add proposal for `kwok` provider #5869

vadasambar · 2023-06-19T06:41:33Z

Signed-off-by: vadasambar surajrbanakar@gmail.com

What type of PR is this?

/kind documentation
/kind feature

What this PR does / why we need it:

This is a proposal for implementing kwok provider. Check the files for more explanation.

Which issue(s) this PR fixes:

It will help fix issues like #5769

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Not sure what should be added ^

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Signed-off-by: vadasambar <surajrbanakar@gmail.com>

k8s-ci-robot · 2023-06-19T06:41:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vadasambar
Once this PR has been reviewed and has the lgtm label, please assign x13n for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

cluster-autoscaler/OWNERS

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

Signed-off-by: vadasambar <surajrbanakar@gmail.com>

vadasambar · 2023-06-20T05:18:10Z

Note that the original plan was to create a first draft and then edit the proposal to match the KEP template but after discussion in sig-autoscaling meeting yesterday, I found out that I don't need to write a proposal to add a cloud provider.

I think having a proposal is still valuable (but it doesn't follow standard KEP template). It helps users understand the motivation for adding the cloud provider and it provides a document everyone can review and add comments/suggestions in. I will keep this PR open for reviews/suggestions from users/contributors before merging it in.

I have asked kwok and sig-scalability for review:

#kwok-dev: https://kubernetes.slack.com/archives/C04QPDWGW0M/p1687237015984919
#sig-scalability: https://kubernetes.slack.com/archives/C09QZTRH7/p1687237346496719

wzshiming · 2023-06-22T02:35:08Z

cluster-autoscaler/proposals/kwok_cloud_provider.md

+* `BuildKwokCloudProvider`: 
+   * Create a lister to watch for nodes with the annotation `kwok.x-k8s.io/node: fake`
+      * The annotation is used to identify nodes created by the `kwok` provider (`kwok` provider creates nodes with this annotation). Limiting the lister `kwok` created nodes ensure we don't touch actual nodes. 
+   * Use `KWOK_TEMPLATES_PATH` env variable to fetch user defined template nodes. This is so that the users can specify their own custom template nodes. This can be the yaml result of `kubectl get nodes` from a different cluster with some changes. Or, it can be nodes defined by the user from scratch in yaml. The yaml has to be mounted in the CA pod at the path specified in `KWOK_TEMPLATES_PATH` env variable   


kubernetes-sigs/kwok#668

I'm also designing this Mock data, if you're interested take look at the API

Thank you for sharing the link. kubernetes-sigs/kwok#668 looks interesting. 👍

wzshiming · 2023-06-22T02:41:49Z

cluster-autoscaler/proposals/kwok_cloud_provider.md

+You can see a PoC implementation of rest of the functions at https://github.com/kubernetes/autoscaler/pull/5820/files#diff-44474ffb56eda61e9a6b16c3ca66461cdf8e02a7e89b659f5a45ca32f5fa8588 (only supports template nodes passed via env variable as of writing this)
+
+### Things to note
+1. Once the user is done with using the `kwok` provider, `--cloudprovider` flag can be reset to any other cloud provider. Deleting the CA pod will cleanup all the fake nodes created by the `kwok` provider and restore the cluster to its original state. This behavior doesn't seem to be supported as of writing this (based on my tests). 


Deleting the CA pod will cleanup all the fake nodes created by the kwok provider and restore the cluster to its original state. This behavior doesn't seem to be supported as of writing this (based on my tests).

Yes, kwok will not delete any nodes, we need to filter the nodes to be deleted based on the label/annotation.

This behavior doesn't seem to be supported as of writing this

Sorry for the confusion. I don't expect kwok to clean up nodes. This here is CA. I need to implement a go interface for this proposal. This interface has a Cleanup() function. My expectation is if I implement this function, CA would call it when the CA pod is terminating. But, this expectation is wrong because Cleanup function is for cleaning go routines when the CA pod is terminating. This is why I said This behavior doesn't seem to be supported as of writing this (based on my tests)..

TODO: change the sentence to make it less confusing

wzshiming · 2023-06-22T02:56:16Z

cluster-autoscaler/proposals/kwok_cloud_provider.md

+* relevant unit tests have been added
+* `kwok` cloud provider is `demo`ed in one of the sig-autoscaling meetings
+
+### FAQ


A question, as I understand it, CA depends on metric-server, right? I'm not sure if kwok nodes can scale up/down properly without /metrics/resource. I'm currently working on supporting metric-server.

kubernetes-sigs/kwok#480

Thank you for the link. Fortunately, CA doesn't depend on kubectl top pods or kubectl top nodes metrics. It is based on kube state metrics (it uses pod CPU and memory requests metrics).

That being said, having metrics server would definitely be nice. 😄

vadasambar · 2023-06-23T17:08:14Z

@wzshiming thank you for taking a look at my proposal 🙏 ❤️

x13n · 2023-10-10T11:37:24Z

/assign

This looks quite interesting, I think some scalability issues could indeed be found with such cloud provider. Definitely not all of them, since some of them are cloud provider specific. I'd be happy to marge this, though maybe it would make sense to discuss on the sig meeting first? Or did that happen already? (I see this PR is quite old by now.)

vadasambar · 2023-10-10T17:50:49Z

/assign

This looks quite interesting, I think some scalability issues could indeed be found with such cloud provider. Definitely not all of them, since some of them are cloud provider specific. I'd be happy to marge this, though maybe it would make sense to discuss on the sig meeting first? Or did that happen already? (I see this PR is quite old by now.)

Thank you for showing interest ❤️

I already talked about this proposal in the sig meeting. Based on the discussion, I don't need an AEP to add a new cloudprovider. I have kept this PR so that people can find it easily.

I am working on getting the first release out. I plan to include this proposal as a doc in the PR.

PR: #5820

I think the code can improve (would love your review :) ). I want to get the first release out so that users can try it and work on the feedback.

x13n · 2023-10-13T09:41:28Z

Gotcha. I'd close this PR then since you're already working on the other one. Anyone reading this will know where to look next. Does that make sense?

Also, #5820 is currently marked as "DON'T REVIEW". Is it already in reviewable state?

vadasambar · 2023-10-13T17:51:59Z

Gotcha. I'd close this PR then since you're already working on the other one. Anyone reading this will know where to look next. Does that make sense?

Agreed. I will close this PR.

Also, #5820 is currently marked as "DON'T REVIEW". Is it already in reviewable state?

I am writing tests right now. I would like to get a review on the PR in its current state but I will refrain (don't want to waste your time). Once I am done with the tests, I will mention you in the PR to let you know it is ready.

docs: add wip proposal for kwok provider

f9bf657

Signed-off-by: vadasambar <surajrbanakar@gmail.com>

k8s-ci-robot requested review from BigDarkClown and feiskyer June 19, 2023 06:41

k8s-ci-robot added the area/cluster-autoscaler label Jun 19, 2023

vadasambar changed the title ~~[AEP] docs: add wip proposal for kwok provider~~ [AEP] docs: add proposal for kwok provider Jun 19, 2023

vadasambar mentioned this pull request Jun 19, 2023

Jun 2023 vadafoss/daily-updates#10

Closed

docs: add better link for PoC implementation

34c16c5

Signed-off-by: vadasambar <surajrbanakar@gmail.com>

vadasambar marked this pull request as ready for review June 20, 2023 05:06

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 20, 2023

k8s-ci-robot requested a review from x13n June 20, 2023 05:07

docs: fix wording as of now -> as of writing this

f5e24f3

Signed-off-by: vadasambar <surajrbanakar@gmail.com>

wzshiming reviewed Jun 22, 2023

View reviewed changes

k8s-ci-robot assigned x13n Oct 10, 2023

vadasambar mentioned this pull request Oct 11, 2023

Oct 2023 vadafoss/daily-updates#14

Closed

vadasambar closed this Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AEP] docs: add proposal for `kwok` provider #5869

[AEP] docs: add proposal for `kwok` provider #5869

vadasambar commented Jun 19, 2023

k8s-ci-robot commented Jun 19, 2023

vadasambar commented Jun 20, 2023

wzshiming Jun 22, 2023 •

edited

Loading

vadasambar Jun 23, 2023 •

edited

Loading

wzshiming Jun 22, 2023

vadasambar Jun 23, 2023

wzshiming Jun 22, 2023

vadasambar Jun 23, 2023

vadasambar commented Jun 23, 2023

x13n commented Oct 10, 2023

vadasambar commented Oct 10, 2023 •

edited

Loading

x13n commented Oct 13, 2023

vadasambar commented Oct 13, 2023

[AEP] docs: add proposal for kwok provider #5869

[AEP] docs: add proposal for kwok provider #5869

Conversation

vadasambar commented Jun 19, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Jun 19, 2023

vadasambar commented Jun 20, 2023

wzshiming Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

vadasambar Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

wzshiming Jun 22, 2023

Choose a reason for hiding this comment

vadasambar Jun 23, 2023

Choose a reason for hiding this comment

wzshiming Jun 22, 2023

Choose a reason for hiding this comment

vadasambar Jun 23, 2023

Choose a reason for hiding this comment

vadasambar commented Jun 23, 2023

x13n commented Oct 10, 2023

vadasambar commented Oct 10, 2023 • edited Loading

x13n commented Oct 13, 2023

vadasambar commented Oct 13, 2023

[AEP] docs: add proposal for `kwok` provider #5869

[AEP] docs: add proposal for `kwok` provider #5869

wzshiming Jun 22, 2023 •

edited

Loading

vadasambar Jun 23, 2023 •

edited

Loading

vadasambar commented Oct 10, 2023 •

edited

Loading