search

k8stopologyawareschedwg / noderesourcetopology-api

This repository contains an implementation for CRD (https://docs.google.com/document/d/12kj3fK8boNuPNqob6F_pPU9ZTaNEnPGaXEooW1Cilwg/edit)
Apache License 2.0
8 stars 6 forks source link

CRD: make cluster scoped #18

Closed ffromani closed 2 years ago

ffromani commented 3 years ago

NodeResourceTopology is an extension of node objects, thus it seems natural for it to have the same scope of node objects, so this PR makes it cluster scoped. This incidentally solves quite some issues we had in both reporting and consuming side about figuring out/agreeing about which namespace we should watch.

swatisehgal commented 2 years ago

Since an NRT object corresponds to a node which is cluster scoped, it is reasonable to make NRT cluster scoped as well. I am happy to approve but would like to get @AlexeyPerevalov's take on this.

ffromani commented 2 years ago

Since an NRT object corresponds to a node which is cluster scoped, it is reasonable to make NRT cluster scoped as well. I am happy to approve but would like to get @AlexeyPerevalov's take on this.

Thanks! If we decide to do this change, I'll be happy to send patches to all the related components, at very least to the scheduler plugin.

marquiz commented 2 years ago

I think this makes sense. Hard to justify or think of scenarios where we would advertise different "views" of the node resources 🤔

AlexeyPerevalov commented 2 years ago

LGTM but it requires scheduler plugin changes too.

ffromani commented 2 years ago

LGTM but it requires scheduler plugin changes too.

Absolutely. I'll be happy to submit these changes. Will link them in this PR and will (obviously) CC you in all of them.

swatisehgal commented 2 years ago

/lgtm /approve

AlexeyPerevalov commented 2 years ago

I would like also to mention. The past scheme with namespaces allowed to implement multi-tenancy support, but it mostly hypothetical availability, since now it requires also a dedicated pools of resource on the worker node. So it reasonable now to keep it cluster scoped, until it will be required.

swatisehgal commented 2 years ago

I agree with you @AlexeyPerevalov. In the current context it makes sense have it as a cluster scoped resource. We can change it back to namespace scope if there are use cases that require us to. In addition to the multi tenancy support, given the hierarchical nature of the API another possible use of this API could be such that each CR is used to represent a rack in a data center. In that case namespaces can be used to represent data centers/geographical zones so having a namespace might be beneficial in that case if the admin wants visibility into a specific data center/geographical zone and restrict access of users.

Let's revisit this in the future if the need arises.