Add SessionAffinity and SessionAffinityConfig fields to ServiceOptions#571
Conversation
There was a problem hiding this comment.
This is a great start!
So what you have done is make the configuration available in the CRD. So the user can now provide it, however it doesn't actually set the configuration on the underlying Services that are created for the SolrCloud/PrometheusExporter.
So we still need to:
- add the controller logic to set these fields when creating services
- make sure the fields are copied correctly when updating the services
- add tests to ensure the fields are propagated correctly
- Add helm chart options
If you want an example of a PR that does something very similar, you can look here: #350
I'm happy to contribute too, I just don't want to step on your toes! Let me know how I can help 🙂
|
|
||
| // sessionAffinityConfig contains the configurations of session affinity. | ||
| // +optional | ||
| SessionAffinityConfig corev1.SessionAffinityConfig `json:"sessionAffinityConfig,omitempty"` |
There was a problem hiding this comment.
| SessionAffinityConfig corev1.SessionAffinityConfig `json:"sessionAffinityConfig,omitempty"` | |
| SessionAffinityConfig *corev1.SessionAffinityConfig `json:"sessionAffinityConfig,omitempty"` |
Making this field a pointer, makes it easier for us to know whether the user provided one or not!
Thanks for the feedback. I think I addressed updating the AFAICT it looks like I need to also update Also, |
HoustonPutman
left a comment
There was a problem hiding this comment.
The changes look good so far! Just a few comments.
Now for actually implementing and testing!
There are three places to implement this logic.
In controllers/util/solr_util.go:GenerateCommonService(). That is what generates the common service for the solrCloud, so that's where we need to take these options from the SolrCloud and pass it to the Service.
In controllers/util/prometheus_exporter_util.go:GenerateSolrMetricsService(). This should be exactly (or almost exactly) the same logic as used above. We just need to support the same thing for the prometheus exporter, since it's a shared option amongst all services.
In controllers/util/common.go:CopyServiceFields(). This is where we update the existing service with what our "expected state" of the service is. So we are copying from the from service to the to service. You can basically copy and paste those if statements, using new ones for sessionAffinity and sessionAffinityConfig.
Then our last step will be testing!
| // sessionAffinityConfig contains the configurations of session affinity. | ||
| // +optional | ||
| SessionAffinityConfig corev1.SessionAffinityConfig `json:"sessionAffinityConfig,omitempty"` | ||
| // +kubebuilder:default=None |
There was a problem hiding this comment.
These (the default and enum lines) were meant to be put up above with sessionAffinity, not sessionAffinityConfig.
|
Also just a thought, but we are giving users the option for the headlessService and podService, but those really don't need sessionAffinity, because they are meant to send requests to a single pod, so affinity doesn't matter. So I'm fine leaving those undocumented and unused. |
Move the Enum/default markers onto the SessionAffinity field and use the typed corev1.ServiceAffinity, then regenerate the CRDs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Set SessionAffinity/SessionAffinityConfig on the common, headless, node and metrics Services, and reconcile them in CopyServiceFields (only when desired, to avoid fighting the API server's defaulting). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add sessionAffinity/sessionAffinityConfig to the headless and node service options, and document all three in the chart README. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I went ahead and added the rest of the tasks, with help from Claude. I think this should be good to go for the next release. |
Attempts to address: #535