Add support for annotations on Deployment/StatefulSet resources in DruidNodeSpec #145

SamWheating · 2024-02-29T21:10:38Z

Fixes #142

Description

Adding a ReplicationControllerAnnotations field to the DruidNodeSpec schema. This allows users to specify annotations to be applied to the StatefulSet / Deployment object.

I'm not 100% sure of the naming - what do you think? I didn't want to call it StatefulSetAnnotations since the user can opt for a deployment instead.

Also regenerated the API docs, which led to a huge diff as it appears they weren't regenerated after #53 (hence the huge diff)

This PR has:

been tested on a real K8S cluster to ensure creation of a brand new Druid cluster works.
been tested for backward compatibility on a real K*S cluster by applying the changes introduced here on an existing Druid cluster. If there are any backward incompatible changes then they have been noted in the PR description.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added documentation for new or modified features or behaviors.

I haven't tested this on a real kube cluster, but all of the e2e tests run. Is that sufficient for an overall minor/straightforward change?

AdheipSingh

Can we also add this field in druidSpec ?
You can merge the annotations if specified in both the spec druid nodeSpec and druidSpec. The latter is cluster scoped. Its common to all druid nodes.

SamWheating · 2024-03-01T01:08:42Z

You can merge the annotations if specified in both the spec druid nodeSpec and druidSpec

Done ✅

Any strong preference on how these are merged? In my opinion it makes sense for the finer-grained DruidNodeSpec-level annotations to override the DruidSpec-level annotations (see implementation), but I see that we do the opposite with PodLabels.

controllers/druid/handler.go

apis/druid/v1alpha1/druid_types.go

itamar-marom · 2024-03-11T13:17:59Z

@SamWheating does Reddit use the operator? if so, can we add you as users in the ADOPTERS.md ?

SamWheating · 2024-03-20T16:08:38Z

does Reddit use the operator?

Not in any official capacity, this work was completed on my own time and mostly just out of personal interest.

itamar-marom

Hi @SamWheating just fix the function name and I'll approve

itamar-marom · 2024-03-21T15:53:45Z

controllers/druid/handler.go

@@ -1287,6 +1289,22 @@ func makeLabelsForNodeSpec(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid,
 	return labels
 }

+// makeAnnotationsForReplicationController returns the annotations for a Deployment or StatefulSet
+// If a given key is set in both the DruidSpec and DruidNodeSpec, the node-scoped value will take precedence.
+func makeAnnotationsForReplicationController(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) map[string]string {


Suggested change

func makeAnnotationsForReplicationController(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) map[string]string {

func makeAnnotationsForWorkload(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) map[string]string {

Ah whoops, I totally missed this 🤦 thanks for spotting!

itamar-marom · 2024-03-21T15:54:42Z

controllers/druid/handler.go

@@ -1287,6 +1289,22 @@ func makeLabelsForNodeSpec(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid,
 	return labels
 }

+// makeAnnotationsForReplicationController returns the annotations for a Deployment or StatefulSet


``

Suggested change

// makeAnnotationsForReplicationController returns the annotations for a Deployment or StatefulSet

// makeAnnotationsForWorkload returns the annotations for a Deployment or StatefulSet

itamar-marom · 2024-03-21T15:54:53Z

controllers/druid/handler.go

-			Namespace: m.Namespace,
-			Labels:    ls,
+			Name:        fmt.Sprintf("%s", nodeSpecUniqueStr),
+			Annotations: makeAnnotationsForReplicationController(nodeSpec, m),


Suggested change

Annotations: makeAnnotationsForReplicationController(nodeSpec, m),

Annotations: makeAnnotationsForWorkload(nodeSpec, m),

itamar-marom · 2024-03-21T15:55:09Z

controllers/druid/handler.go

-			Namespace: m.Namespace,
-			Labels:    ls,
+			Name:        fmt.Sprintf("%s", nodeSpecUniqueStr),
+			Annotations: makeAnnotationsForReplicationController(nodeSpec, m),


Suggested change

Annotations: makeAnnotationsForReplicationController(nodeSpec, m),

Annotations: makeAnnotationsForWorkload(nodeSpec, m),

itamar-marom

LGTM!

…uidNodeSpec (datainfrahq#145) * Add support for annotations on Deployment/StatefulSet resources * Support setting ReplicationControllerAnnotations at the cluster-level * rename replicationControllerAnnotations to workloadAnnotations * suggestions from code review

* Ingestion Controller (#53) * ingestion spec acc to v3 * task creation * supprt native batch * fix router url * revert license change * revert go mod change * fix main * fix: made some changes as per review comments * fix: removed unused package from ingestion reconciler * rebase 1 * add example * add review * update dockerfile --------- Co-authored-by: avtarOPS <[email protected]> * Update Docs and Tutorials (#138) * docs and tutorials * Refactor/ordering (#123) * (ordering): refactor code * (ordering): refactor code * (ordering): testing * chore(branch): rebase branch with master * fix(tests): validate nodes order by regex * Bump controller-tools version (#140) * Utilize the DruidIngestion controller in e2e tests (#146) * adds needed volumes to eks deployment spec and improves getting started documentation by noting minio dependency (#149) * Add support for annotations on Deployment/StatefulSet resources in DruidNodeSpec (#145) * Add support for annotations on Deployment/StatefulSet resources * Support setting ReplicationControllerAnnotations at the cluster-level * rename replicationControllerAnnotations to workloadAnnotations * suggestions from code review * Add support for multi tier nodes with different PVC sizes (#106) (#152) Co-authored-by: Farhad Farahi <[email protected]> * fix: put Druid crds in the appropriate folder specified by Helm (#162) Signed-off-by: ahmed.g <[email protected]> Signed-off-by: TessaIO <[email protected]> * Adds service account name to each druid node optionally (#164) * Adds service account to each druid node optionally * Use controller-gen v0.11.2 --------- Signed-off-by: ahmed.g <[email protected]> Signed-off-by: TessaIO <[email protected]> Co-authored-by: AdheipSingh <[email protected]> Co-authored-by: avtarOPS <[email protected]> Co-authored-by: Itamar Marom <[email protected]> Co-authored-by: Jesper Larsson <[email protected]> Co-authored-by: Sam Wheating <[email protected]> Co-authored-by: Evan Jones <[email protected]> Co-authored-by: Farhad Farahi <[email protected]> Co-authored-by: Farhad Farahi <[email protected]> Co-authored-by: Sadananda Aithal <[email protected]>

AdheipSingh requested review from itamar-marom, AdheipSingh and cyril-corbon February 29, 2024 22:35

AdheipSingh assigned SamWheating Feb 29, 2024

AdheipSingh added the enhancement New feature or request label Feb 29, 2024

AdheipSingh requested changes Feb 29, 2024

View reviewed changes

SamWheating added 2 commits March 1, 2024 16:49

Add support for annotations on Deployment/StatefulSet resources

312f8ac

Support setting ReplicationControllerAnnotations at the cluster-level

ee37b18

SamWheating force-pushed the sw-add-annotations-on-sts branch from f5e647e to ee37b18 Compare March 2, 2024 00:50

itamar-marom reviewed Mar 3, 2024

View reviewed changes

controllers/druid/handler.go Outdated Show resolved Hide resolved

itamar-marom reviewed Mar 3, 2024

View reviewed changes

apis/druid/v1alpha1/druid_types.go Outdated Show resolved Hide resolved

rename replicationControllerAnnotations to workloadAnnotations

b71f372

SamWheating requested review from itamar-marom and AdheipSingh March 20, 2024 15:58

itamar-marom reviewed Mar 21, 2024

View reviewed changes

suggestions from code review

2510f11

itamar-marom approved these changes Mar 22, 2024

View reviewed changes

AdheipSingh approved these changes Mar 28, 2024

View reviewed changes

AdheipSingh merged commit 6eb2937 into datainfrahq:master Mar 28, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for annotations on Deployment/StatefulSet resources in DruidNodeSpec #145

Add support for annotations on Deployment/StatefulSet resources in DruidNodeSpec #145

SamWheating commented Feb 29, 2024

AdheipSingh left a comment

SamWheating commented Mar 1, 2024

itamar-marom commented Mar 11, 2024

SamWheating commented Mar 20, 2024

itamar-marom left a comment

itamar-marom Mar 21, 2024

SamWheating Mar 21, 2024

itamar-marom Mar 21, 2024

itamar-marom Mar 21, 2024

itamar-marom Mar 21, 2024

itamar-marom left a comment

	func makeAnnotationsForReplicationController(nodeSpec v1alpha1.DruidNodeSpec, m v1alpha1.Druid) map[string]string {
	func makeAnnotationsForWorkload(nodeSpec v1alpha1.DruidNodeSpec, m v1alpha1.Druid) map[string]string {

	// makeAnnotationsForReplicationController returns the annotations for a Deployment or StatefulSet
	// makeAnnotationsForWorkload returns the annotations for a Deployment or StatefulSet

	Annotations: makeAnnotationsForReplicationController(nodeSpec, m),
	Annotations: makeAnnotationsForWorkload(nodeSpec, m),

Add support for annotations on Deployment/StatefulSet resources in DruidNodeSpec #145

Add support for annotations on Deployment/StatefulSet resources in DruidNodeSpec #145

Conversation

SamWheating commented Feb 29, 2024

Description

AdheipSingh left a comment

Choose a reason for hiding this comment

SamWheating commented Mar 1, 2024

itamar-marom commented Mar 11, 2024

SamWheating commented Mar 20, 2024

itamar-marom left a comment

Choose a reason for hiding this comment

itamar-marom Mar 21, 2024

Choose a reason for hiding this comment

SamWheating Mar 21, 2024

Choose a reason for hiding this comment

itamar-marom Mar 21, 2024

Choose a reason for hiding this comment

itamar-marom Mar 21, 2024

Choose a reason for hiding this comment

itamar-marom Mar 21, 2024

Choose a reason for hiding this comment

itamar-marom left a comment

Choose a reason for hiding this comment