Feat: Add Multi-Tenant Manager to CCM #935
Feat: Add Multi-Tenant Manager to CCM #935priyapande wants to merge 4 commits intokubernetes:masterfrom
Conversation
|
This issue is currently awaiting triage. If the repository mantainers determine this is a relevant issue, they will accept it by applying the The DetailsInstructions 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-sigs/prow repository. |
|
Welcome @priyapande! |
|
Hi @priyapande. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
/assign |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: priyapande The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Can we split off the vendor change in a separate commit? Right now it's 300 files, but I'm assuming most of it is in vendor/ |
| klog.Infof("[%s] Creating OSS Cloud Node Controller...", pcKey) | ||
| nodeController, err := node.NewCloudNodeController( | ||
| filteredFactory.Core().V1().Nodes(), | ||
| m.kubeClient, | ||
| scopedCloud, | ||
| m.config.ComponentConfig.NodeStatusUpdateFrequency.Duration, | ||
| m.config.ComponentConfig.NodeController.ConcurrentNodeSyncs, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
there has to be two independent launchers, and users should opt-in via the standards flags to enable one or other controllers, check this for prior art https://github.com/kubernetes/cloud-provider-gcp/pull/895/files
The litmus test is that the addition of this new controller/folder will have zero impact, and the behavior is governed by the flags
|
We met offline, to summarize our discussion, try to follow the same approached used in #895
|
92f9007 to
6b804f7
Compare
66e6b19 to
b00b173
Compare
| klog.Errorf("Failed to create network client: %v", err) | ||
| return nil, false, err | ||
| } | ||
| networkInformerFactory := networkinformers.NewSharedInformerFactory(networkClient, 12*time.Hour) |
There was a problem hiding this comment.
are we doing this resync every 12 hours on purpose? this has created some internal issues in other. controllers and nobody was able to explain me the reason to add this .. if we want to periodically go over the elements in the cache then this should be much lower, if we do not need that then this should be 0
There was a problem hiding this comment.
That's a valid point, changed it to zero as the controller can rely on the event stream to process the required changes and a full walkthrough of cache isn't necessary. Thanks
|
|
||
| // StartNodeIpamController starts the NodeIPAM controller. | ||
| // It returns the controller interface, a boolean indicating if it started, and an error if any. | ||
| func StartNodeIpamController( |
There was a problem hiding this comment.
it feels like this nodeipam starts should not be mixed in the same commit or even the same PR, if something goes wrong we have to revert the whole thing
There was a problem hiding this comment.
moved refactoring & vendor logic to separate PR - #966
9e538fd to
088172d
Compare
Add support for a tenant controller manager that launches node controllers separately for each tenant that are scoped to it's tenant authentication & filtered nodes view.