operator v1: API only: add NodePools to Cluster CRD #221
Conversation
This introduces NodePools to the Cluster CRD. NodePools allow configuration of independent pod-sets, that are often backed by specific NodePools. This allows for safe blue/green deployments end-to-end; from cloud provider NodePool to the Pods in the Kubernetes Cluster. Despite this being discussed, this change does not add a PodTemplateSpec to NodePoolSpec. It sounds overly generic, and it is not used in the existing code either. Our goal here is to stay as close as possible to existing practices (and we have no concrete use for PodTemplateSpec at the moment anyway). In addition, we want to re-use the existing RedpandaResourceRequirements type, just as it used before this patch.
birdayz
changed the title
| @@ -116,7 +116,7 @@ type ClusterSpec struct { | |||
| // containers have separate resources settings and the amount of resources | |||
| // assigned to these containers will be required on the cluster on top of | |||
| // the resources defined here | |||
| Resources RedpandaResourceRequirements `json:"resources"` | |||
| Resources RedpandaResourceRequirements `json:"resources,omitempty"` | |||
There was a problem hiding this comment.
this needs to receive omitempty, because we need to make it optional AFAIK. so we can have a CR without it (w/o a default nodepool basically, only ones listed in spec.nodePools)
There was a problem hiding this comment.
If you leave that as is you will have Resources of type RedpandaResourceRequirements which if not provided it will default to 0. To differentiate not provided value and zero value I'm used to making it a pointer to a struct.
There was a problem hiding this comment.
is this an allowed, compatible change? making it pointer? i am not sure about the wire format implications; i guess only that null/absence is also possible
There was a problem hiding this comment.
I think it is compatible change as any user that used or did not use resources would not see any difference.
Never the less Cluster custom resource is not defined in our product. Only cloud is using it and K8S team maintains only Redpanda CRD.
There was a problem hiding this comment.
i updated it to pointer.
please do not merge yet, i will merge once i've tested this on our end regarding compatibility. pretty sure you are right, it's fine, but want to test just in case.
There was a problem hiding this comment.
changed back to what it was, non pointer.
all the code already treats this struct as de facto optional, eg:
requestedCores := requests.RedpandaCPU().Value()
// When cpu is not set, all cores are used
if requestedCores > 0 {
args["smp"] = strconv.FormatInt(requestedCores, 10)
}
it is already possible to not provide this field. No need to change it to pointer. Changing to pointer requires a lot of changes along the way, because it's then nil-able.
There was a problem hiding this comment.
From my side it looks good. Maybe @andrewstucki or @chrisseto could double check.
Co-authored-by: Rafal Korepta <[email protected]>
| // Replicas, so they can be controlled independently | ||
| // Resources, because this is tied strongly to the actual machine shape backing the NodePool. | ||
| type NodePoolSpec struct { | ||
| Name string `json:"name"` |
There was a problem hiding this comment.
I think we discussed this but I don't recall the outcome, did you consider placing a PodTemplate here instead of only allowing Tolerations and NodeSelector to be overridden?
There was a problem hiding this comment.
i added a comment here already:
Despite this being discussed, this change does not add a PodTemplateSpec to NodePoolSpec. It sounds overly generic, and it is not used in the existing code either. Our goal here is to stay as close as possible to existing practices (and we have no concrete use for PodTemplateSpec at the moment anyway). In addition, we want to re-use the existing RedpandaResourceRequirements type, just as it used before this patch.
i would prefer staying as close as possible to the existing CRD and code, and not introduce new "nice" things. unless there's an actual use case for it.
so, saving such more structural changes for a "operator v3" or "cloud operator CRD"
| // Replicas, so they can be controlled independently | ||
| // Resources, because this is tied strongly to the actual machine shape backing the NodePool. | ||
| type NodePoolSpec struct { | ||
| Name string `json:"name"` |
There was a problem hiding this comment.
Maybe worth including a validation on minimum length here so we don't forget to specify this given that an empty string would be valid? Or is that intentional?
|
thanks for the feedback. i will address the comments and do another cleanup tomorrow. |
birdayz
force-pushed
the
jb/nodepools-api-changes-only
branch
2 times, most recently
from
98ae0bc to
58151c2
Compare
birdayz
force-pushed
the
jb/nodepools-api-changes-only
branch
from
58151c2 to
688cf6b
Compare
This introduces NodePools to the Cluster CRD.
NodePools allow configuration of independent pod-sets, that are often backed by specific CloudProvider NodePools. This allows for safe blue/green deployments end-to-end; from cloud provider NodePool to the Pods in the Kubernetes Cluster.
Despite this being discussed, this change does not add a PodTemplateSpec to NodePoolSpec. It sounds overly generic, and it is not used in the existing code either. Our goal here is to stay as close as possible to existing practices (and we have no concrete use for PodTemplateSpec at the moment anyway). In addition, we want to re-use the existing RedpandaResourceRequirements type, just as it used before this patch.
There will probably later follow an additional CloudStorage/CacheStorage related field, however there's an unsolved problem at the moment, that will not make it very useful yet. It will be added separately.
The actual implementation will follow in a later PR. Having the API spec merged ahead of the implementation will make it much easier for cloud to ship a feature flag that makes use of the new implementation.