Replace Bitnami with Strimzi Kafka #1005

nasark · 2023-10-23T21:26:47Z

install Strimzi by creating a subscription from the community-operators OLM CatalogSource
move miq-components/kafka.go to its own package miq-components/kafka/kafka.go, this will allow the CRs to be used elsewhere (i.e. downstream)

Depends on:

TODO:

Since Strimzi requires SSL, need to mount the ca.crt in orchestrator/workers regardless of whether internal-certificate-secret is present or not
set node affinity in Kafka CR
drop bitnami references in CR (marked as deprecated in CR)
topic management to be handled by orchestrator? (not needed for now)

@miq-bot assign @bdunne
@miq-bot add_reviewer @Fryguy
@miq-bot add_label enhancement

nasark · 2023-10-23T21:29:31Z

manageiq-operator/api/v1alpha1/helpers/miq-components/kafka/kafka.go

+	kafkaResourceRequests["memory"] = "1Gi"
+	kafkaResourceRequests["cpu"] = "200m"


@bdunne Is there a reason why we don't use default values for cr.Spec.KafkaMemoryRequest etc in upstream? I was trying to use those values here but its not in the CR spec

Upstream we usually only set requests and limits if cr.Spec.EnforceWorkerResourceConstraints is set to allow running on lower resource clusters

If set, we should primarily pull the values from the CR cr.Spec.KafkaMemoryLimit, cr.Spec.KafkaMemoryRequest, cr.Spec.KafkaCpuLimit, cr.Spec.KafkaCpuRequest otherwise have some default values.

Also, this should be done inside the mutate function in case the CR values are updated

nasark · 2023-10-23T21:30:53Z

manageiq-operator/api/v1alpha1/helpers/miq-components/kafka/kafka.go

+	kafkaSubscriptionSpec := map[string]interface{}{
+		"channel":         "strimzi-0.35.x",
+		"name":            "strimzi-kafka-operator",
+		"source":          "community-operators",
+		"sourceNamespace": "openshift-marketplace",
+	}


Fixed the strimzi release to 0.35.1 to match downstream, also to prevent breaking changes that may be introduced in latest releases

We'll need to come up with an upgrade plan at some point. Do you just change the channel name?

nasark · 2023-11-02T19:31:39Z

manageiq-operator/api/v1alpha1/helpers/miq-components/kafka/kafka.go

+// If manual certificates are introduced after Strimzi has generated its own certificates,
+// then the certificates must be "renewed" to replace the old. This process is outlined here:
+// https://strimzi.io/docs/operators/in-development/deploying#proc-replacing-your-own-private-keys-str
+func renewKafkaCASecret(cr *miqv1alpha1.ManageIQ, client client.Client, scheme *runtime.Scheme) error {


FYI key change that differentiates this from downstream. Added a comment in the code as I thought it would be beneficial but I can remove if not necessary

Also see https://github.com/ManageIQ/manageiq-pods/pull/1005/files#diff-64db691596174379fdf471b9867e0698ac2c8a8fb0948b10766bf4e75688b054R312-R320

nasark · 2023-11-16T20:01:19Z

@bdunne @Fryguy Please review

bdunne · 2023-11-28T14:09:23Z

Close/reopen to kick tests

bdunne · 2023-11-28T20:43:42Z

manageiq-operator/api/v1alpha1/helpers/miq-components/kafka/kafka.go

+	"strconv"
+)
+
+var caGen = 0


Do we need to store this somewhere? Every time the operator boots, we start at 0 again, right?

If the operator is redeployed then yes, we start at 0 again which I think is fine because the certs will be cleaned up and redeployed again? Let me know if that is okay. The intent behind this global var is so that the value persists between reconciles, so if a cert needs to be renewed we can increment the previous value

Well, the operator could be rescheduled for any reason (host going down for maintenance, resource usage, etc), so every time a new one starts, it will start at 0 and copying the certs causing kafka to be restarted could be a problem

nasark · 2023-11-30T22:08:27Z

Applied the following feedback:

Limit/requests with cr.Spec.EnforceWorkerResourceConstraints, move to mutate function
Use addLabel function for labels (dependent PR AddLabels util function #1020)
Productize names
Use subscription api instead of unstructured objects

nasark · 2023-12-06T16:53:40Z

Applied the following:

reworked renewKafkaCASecret() and relevant code to use caGen stored in last-known-revision annotation
- also a bug fix for updating unstructured kafka objects (explained below)
Check if openshift is being used by looking for openshift-marketplace namespace and relevant catalog source
Create operatorgroup in order to allow creation of strimzi subscription from catalogsource

nasark · 2023-12-06T17:07:40Z

manageiq-operator/api/v1alpha1/helpers/miq-components/kafka/kafka.go

+func updateKafka(cr *miqv1alpha1.ManageIQ, client client.Client, scheme *runtime.Scheme, update func(*unstructured.Unstructured) *unstructured.Unstructured) error {
+	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		kafka := miqutilsv1alpha1.FindKafka(client, scheme, cr.Namespace, cr.Spec.AppName)
+		kafka = update(kafka)
+
+		err := client.Update(context.TODO(), kafka)
+		return err
+	})
+	if err != nil {
+		return err
+	}
+
+	return nil
+}


Introduced this function since updates are being made to the Kafka CR outside of reconciliation which can cause conflicts (metadata like creationTimestamp, uid), especially when dealing with unstructured objects since they are considered "dynamic" types. The recommended solution is to simply retry until the update succeeds. In my testing if there is a conflict the update usually succeeds on the 2nd retry.

You can read more about this here:

https://github.com/kubernetes/client-go/blob/master/examples/dynamic-create-update-delete-deployment/README.md#typed-vs-dynamic

https://github.com/kubernetes/client-go/blob/master/examples/dynamic-create-update-delete-deployment/main.go#L117-L129

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#concurrency-control-and-consistency

manageiq-operator/internal/controller/manageiq_controller.go

nasark · 2023-12-06T17:15:42Z

manageiq-operator/api/v1alpha1/helpers/miq-components/kafka/kafka.go

+	olmv1 "github.com/operator-framework/api/pkg/operators/v1"
+	olmv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"


For some reason olmv1 doesn't contain the kinds available in olmv1alpha1 but it provides OperatorGroup which is not available in olmv1alpha1, and so we need to import both packages unfortunately 🤕

nasark · 2023-12-19T21:17:29Z

@miq-bot add_label quinteros/yes?

bdunne

Do any of the new objects need to be backed up? If so, we should add backup labels to them

manageiq-operator/api/v1alpha1/helpers/miq-components/kafka/kafka.go

bdunne · 2024-01-03T22:43:46Z

manageiq-operator/api/v1alpha1/helpers/miq-components/kafka/kafka.go

+	zookeeperResourceLimits["memory"] = "512Mi"
+	zookeeperResourceLimits["cpu"] = "250m"
+
+	kafkaCRSpec = miqutilsv1alpha1.SetKafkaNodeAffinity(kafkaCRSpec, []string{"amd64"})


That linked comment was about the platforms that our operator can run on. We only build for ~~x86_64~~amd64 right now. This is determining placement for kafka and zookeeper, so it should match the platforms that they are built for.

bdunne · 2024-01-03T22:44:18Z

manageiq-operator/api/v1alpha1/helpers/miq-components/kafka/kafka.go

+	zookeeperResourceLimits["memory"] = "512Mi"
+	zookeeperResourceLimits["cpu"] = "250m"
+
+	kafkaCRSpec = miqutilsv1alpha1.SetKafkaNodeAffinity(kafkaCRSpec, []string{"amd64"})


Also, it's the same as 2 lines up

manageiq-operator/api/v1alpha1/helpers/miq-components/kafka/kafka.go

manageiq-operator/api/v1alpha1/helpers/miq-components/orchestrator.go

bdunne · 2024-01-03T23:17:42Z

manageiq-operator/api/v1alpha1/helpers/miq-components/orchestrator.go

@@ -359,5 +362,10 @@ func addInternalRootCertificate(cr *miqv1alpha1.ManageIQ, d *appsv1.Deployment,
 			d.Spec.Template.Spec.Containers[0].Env = addOrUpdateEnvVar(d.Spec.Template.Spec.Containers[0].Env, corev1.EnvVar{Name: "MEMCACHED_ENABLE_SSL", Value: "true"})
 			d.Spec.Template.Spec.Containers[0].Env = addOrUpdateEnvVar(d.Spec.Template.Spec.Containers[0].Env, corev1.EnvVar{Name: "MEMCACHED_SSL_CA", Value: "/etc/pki/ca-trust/source/anchors/root.crt"})
 		}
+	} else {


Should this be an else if checking that the manageiq-cluster-ca-cert secret exists since we have the option to deploy without the messaging service in the CRD?

manageiq-operator/go.mod

bdunne · 2024-01-04T18:48:54Z

manageiq-operator/api/v1alpha1/helpers/miq-components/kafka/kafka.go

+			CatalogSource:          "community-operators",
+			CatalogSourceNamespace: "openshift-marketplace",
+			Package:                "strimzi-kafka-operator",
+			Channel:                "strimzi-0.35.x",


0.35 doesn't support ppc64le, 0.37.0 looks like the first version that does:

... "0.35.0-rc1-amd64", "0.35.0-rc1-arm64", "0.35.0-rc1-s390x", "0.35.0-rc1", "0.35.0-0-amd64", "0.35.0-0-arm64", "0.35.0-0-s390x", "0.35.0-0", "0.35.0-amd64", "0.35.0-arm64", "0.35.0-s390x", "0.35.0", "0.35.1-rc1-amd64", "0.35.1-rc1-arm64", "0.35.1-rc1-s390x", "0.35.1-rc1", "0.35.1-0-amd64", "0.35.1-0-arm64", "0.35.1-0-s390x", "0.35.1-0", "0.35.1-1-amd64", "0.35.1-1-arm64", "0.35.1-1-s390x", "0.35.1-1", "0.35.1-amd64", "0.35.1-arm64", "0.35.1-s390x", "0.35.1", ...

bdunne · 2024-01-04T20:21:35Z

manageiq-operator/api/v1alpha1/helpers/miq-components/kafka/kafka.go

+	kafkaCRSpec = miqutilsv1alpha1.SetKafkaNodeAffinity(kafkaCRSpec, []string{"amd64"})
+
+	kafkaCRSpec = miqutilsv1alpha1.SetKafkaNodeAffinity(kafkaCRSpec, []string{"amd64"})


Suggested change

kafkaCRSpec = miqutilsv1alpha1.SetKafkaNodeAffinity(kafkaCRSpec, []string{"amd64"})

kafkaCRSpec = miqutilsv1alpha1.SetKafkaNodeAffinity(kafkaCRSpec, []string{"amd64"})

kafkaCRSpec = miqutilsv1alpha1.SetKafkaNodeAffinity(kafkaCRSpec, []string{"amd64", "arm64", "ppc64le", "s390x"})

@bdunne arm? It can't hurt to add, but we don't need it right?

🤷🏻‍♂️ It's a scheduling limitation. Since they build it, we may as well allow running there if the resources are available.

miq-bot · 2024-01-04T20:45:20Z

Checked commits nasark/manageiq-pods@80790f6~...a1fb2fb with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
1 file checked, 1 offense detected

**

💣 💥 🔥 🚒 - Linter/Yaml - missing config files

Fryguy · 2024-01-05T15:20:03Z

Backported to quinteros in commit 22aa97a.

commit 22aa97ad00f6db6097eb1e65300aff2a894fddd5
Author: Brandon Dunne <[email protected]>
Date:   Thu Jan 4 17:18:11 2024 -0500

    Merge pull request #1005 from nasark/strimzi_kafka
    
    Replace Bitnami with Strimzi Kafka
    
    (cherry picked from commit 0ccfda19e605fa147218ce6ee3881c2f1934ca83)

Replace Bitnami with Strimzi Kafka (cherry picked from commit 0ccfda1)

nasark requested a review from bdunne as a code owner October 23, 2023 21:26

miq-bot assigned bdunne Oct 23, 2023

miq-bot requested a review from Fryguy October 23, 2023 21:27

miq-bot added the enhancement label Oct 23, 2023

nasark commented Oct 23, 2023

View reviewed changes

nasark force-pushed the strimzi_kafka branch 2 times, most recently from 843154a to 3a51235 Compare October 23, 2023 21:34

nasark mentioned this pull request Oct 24, 2023

Add Node affinity #1003

Merged

miq-bot added wip unmergeable labels Oct 31, 2023

nasark force-pushed the strimzi_kafka branch 4 times, most recently from f5cadb8 to 094322f Compare November 2, 2023 16:25

This was referenced Nov 2, 2023

Mount messaging cert if root not present ManageIQ/manageiq#22773

Merged

Add or update unique volume mount #1008

Merged

nasark commented Nov 2, 2023

View reviewed changes

miq-bot removed the unmergeable label Nov 6, 2023

nasark mentioned this pull request Nov 6, 2023

Enforce and default Kafka configuration #896

Merged

3 tasks

nasark force-pushed the strimzi_kafka branch from 475df14 to 32fa54f Compare November 6, 2023 16:54

nasark changed the title ~~[WIP] Replace Bitnami with Strimzi Kafka~~ Replace Bitnami with Strimzi Kafka Nov 6, 2023

miq-bot removed the wip label Nov 6, 2023

bdunne closed this Nov 28, 2023

bdunne reopened this Nov 28, 2023

nasark force-pushed the strimzi_kafka branch 2 times, most recently from 84432a9 to 68d0602 Compare November 28, 2023 14:39

bdunne reviewed Nov 28, 2023

View reviewed changes

nasark force-pushed the strimzi_kafka branch from 68d0602 to 66c7d7c Compare November 30, 2023 22:06

nasark force-pushed the strimzi_kafka branch 2 times, most recently from 331642b to 39eeed7 Compare December 6, 2023 16:49

nasark commented Dec 6, 2023

View reviewed changes

manageiq-operator/internal/controller/manageiq_controller.go Show resolved Hide resolved

nasark commented Dec 6, 2023

View reviewed changes

nasark closed this Dec 19, 2023

nasark reopened this Dec 19, 2023

nasark force-pushed the strimzi_kafka branch from 39eeed7 to 0f53d3b Compare December 19, 2023 18:28

miq-bot added the quinteros/yes? label Dec 19, 2023

bdunne reviewed Jan 3, 2024

View reviewed changes

nasark force-pushed the strimzi_kafka branch 2 times, most recently from c023987 to 6b94a2f Compare January 4, 2024 16:44

replace bitnami with strimzi kafka

80790f6

nasark force-pushed the strimzi_kafka branch from 6b94a2f to 07efb91 Compare January 4, 2024 16:56

bdunne reviewed Jan 4, 2024

View reviewed changes

add kafka node affinity

a1fb2fb

nasark force-pushed the strimzi_kafka branch from 07efb91 to a1fb2fb Compare January 4, 2024 20:41

bdunne merged commit 0ccfda1 into ManageIQ:master Jan 4, 2024
2 checks passed

Fryguy added quinteros/yes and removed quinteros/yes? labels Jan 5, 2024

Fryguy pushed a commit that referenced this pull request Jan 5, 2024

Merge pull request #1005 from nasark/strimzi_kafka

22aa97a

Replace Bitnami with Strimzi Kafka (cherry picked from commit 0ccfda1)

Fryguy added quinteros/backported and removed quinteros/yes labels Jan 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Bitnami with Strimzi Kafka #1005

Replace Bitnami with Strimzi Kafka #1005

nasark commented Oct 23, 2023 •

edited by Fryguy

Loading

nasark Oct 23, 2023

bdunne Nov 28, 2023

bdunne Nov 28, 2023

nasark Oct 23, 2023 •

edited

Loading

bdunne Nov 28, 2023

nasark Nov 2, 2023 •

edited

Loading

nasark commented Nov 16, 2023

bdunne commented Nov 28, 2023

bdunne Nov 28, 2023

nasark Nov 29, 2023

bdunne Nov 29, 2023

nasark commented Nov 30, 2023 •

edited

Loading

nasark commented Dec 6, 2023

nasark Dec 6, 2023 •

edited

Loading

nasark Dec 6, 2023 •

edited

Loading

nasark commented Dec 19, 2023

bdunne left a comment

bdunne Jan 3, 2024 •

edited

Loading

bdunne Jan 3, 2024

bdunne Jan 3, 2024

bdunne Jan 4, 2024

bdunne Jan 4, 2024

Fryguy Jan 4, 2024

bdunne Jan 4, 2024

miq-bot commented Jan 4, 2024

Fryguy commented Jan 5, 2024

		kafkaResourceRequests["memory"] = "1Gi"
		kafkaResourceRequests["cpu"] = "200m"

		olmv1 "github.com/operator-framework/api/pkg/operators/v1"
		olmv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"

		kafkaCRSpec = miqutilsv1alpha1.SetKafkaNodeAffinity(kafkaCRSpec, []string{"amd64"})

		kafkaCRSpec = miqutilsv1alpha1.SetKafkaNodeAffinity(kafkaCRSpec, []string{"amd64"})

Replace Bitnami with Strimzi Kafka #1005

Replace Bitnami with Strimzi Kafka #1005

Conversation

nasark commented Oct 23, 2023 • edited by Fryguy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasark Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasark Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

nasark commented Nov 16, 2023

bdunne commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasark commented Nov 30, 2023 • edited Loading

nasark commented Dec 6, 2023

nasark Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

nasark Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

nasark commented Dec 19, 2023

bdunne left a comment

Choose a reason for hiding this comment

bdunne Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jan 4, 2024

Fryguy commented Jan 5, 2024

nasark commented Oct 23, 2023 •

edited by Fryguy

Loading

nasark Oct 23, 2023 •

edited

Loading

nasark Nov 2, 2023 •

edited

Loading

nasark commented Nov 30, 2023 •

edited

Loading

nasark Dec 6, 2023 •

edited

Loading

nasark Dec 6, 2023 •

edited

Loading

bdunne Jan 3, 2024 •

edited

Loading