Fix configMap key value template #110

nuriel77 · 2021-12-23T08:37:52Z

Overview

In this commit, based on issue 103 I suggested to omit the toYaml and instead use key value iteration to produce the configMap configData.

This should solve the issue of duplicated keys in the configMap and also keep backward compatibility with values provided with two sets of double quotes.

@michaeljmarshall I wasn't sure if I have to bump the Chart's version or is that something your automation takes care of.

Tested Values

I have tested using helm template . while testing different ways of configuring the values for configData:

Option 1:

zookeeper:
  configData:
    PULSAR_MEM: >
      -Xms1g
      -Xmx1g
      -Dcom.sun.management.jmxremote
      -Djute.maxbuffer=10485760
    PULSAR_GC: >
      -XX:+UseG1GC
    PULSAR_LOG_LEVEL: "info"
    PULSAR_LOG_ROOT_LEVEL: "info"
    PULSAR_EXTRA_OPTS: "-Dzookeeper.tcpKeepAlive=true -Dzookeeper.clientTcpKeepAlive=true -Dpulsar.log.root.level=info"

Option 2:

zookeeper:
  configData:
    PULSAR_MEM: "\"-Xms1g -Xmx1g -Dcom.sun.management.jmxremote -Djute.maxbuffer=10485760\""
    PULSAR_GC: "\"-XX:+UseG1GC\""
    PULSAR_LOG_LEVEL: "info"
    PULSAR_LOG_ROOT_LEVEL: "info"
    PULSAR_EXTRA_OPTS: "-Dzookeeper.tcpKeepAlive=true -Dzookeeper.clientTcpKeepAlive=true -Dpulsar.log.root.level=info"

Option 3 (should be the same for single-quotes):

zookeeper:
  configData:
    PULSAR_MEM: "-Xms1g -Xmx1g -Dcom.sun.management.jmxremote -Djute.maxbuffer=10485760"
    PULSAR_GC: "-XX:+UseG1GC"
    PULSAR_LOG_LEVEL: "info"
    PULSAR_LOG_ROOT_LEVEL: "info"
    PULSAR_EXTRA_OPTS: "-Dzookeeper.tcpKeepAlive=true -Dzookeeper.clientTcpKeepAlive=true -Dpulsar.log.root.level=info"

Option 4:

zookeeper:
  configData:
    PULSAR_MEM: -Xms1g -Xmx1g -Dcom.sun.management.jmxremote -Djute.maxbuffer=10485760
    PULSAR_GC: -XX:+UseG1GC
    PULSAR_LOG_LEVEL: "info"
    PULSAR_LOG_ROOT_LEVEL: "info"
    PULSAR_EXTRA_OPTS: "-Dzookeeper.tcpKeepAlive=true -Dzookeeper.clientTcpKeepAlive=true -Dpulsar.log.root.level=info"

Outputs

I have tested the output of the configMaps that could be produced by the chart:

pulsar-bastion:

apiVersion: v1
kind: ConfigMap
metadata:
  name: "pulsar-bastion"
  namespace: monitoring
  labels:
    app: pulsar
    chart: pulsar-2.0.10
    release: RELEASE-NAME
    heritage: Helm
    component: bastion
    cluster: pulsar
data:
  tlsTrustCertsFilePath: "/etc/ssl/certs/ca-certificates.crt"
  PULSAR_EXTRA_OPTS: "-Dpulsar.log.root.level=info"
  PULSAR_GC: "-XX:+UseG1GC"
  PULSAR_LOG_LEVEL: "info"
  PULSAR_LOG_ROOT_LEVEL: "info"
  PULSAR_MEM: "-XX:+ExitOnOutOfMemoryError"

pulsar-broker:

apiVersion: v1
kind: ConfigMap
metadata:
  name: "pulsar-broker"
  namespace: monitoring
  labels:
    app: pulsar
    chart: pulsar-2.0.10
    release: RELEASE-NAME
    heritage: Helm
    component: broker
    cluster: pulsar
data:
  zookeeperServers:
    pulsar-zookeeper-ca:2181
  configurationStoreServers:
    pulsar-zookeeper-ca:2181
  clusterName: pulsar
  allowAutoTopicCreationType: "non-partitioned"
  PULSAR_EXTRA_OPTS: "-Dpulsar.log.root.level=info"
  PULSAR_GC: "-XX:+UseG1GC"
  PULSAR_LOG_LEVEL: "info"
  PULSAR_LOG_ROOT_LEVEL: "info"
  PULSAR_MEM: "-Xms2g -Xmx2g -XX:MaxDirectMemorySize=2g -Dio.netty.leakDetectionLevel=disabled -Dio.netty.recycler.linkCapacity=1024 -XX:+ExitOnOutOfMemoryError"
  backlogQuotaDefaultRetentionPolicy: "producer_exception"
  brokerDeduplicationEnabled: "false"
  exposeConsumerLevelMetricsInPrometheus: "false"
  exposeTopicLevelMetricsInPrometheus: "true"

pulsar-brokerSts:

apiVersion: v1
kind: ConfigMap
metadata:
  name: "pulsar-brokersts"
  namespace: monitoring
  labels:
    app: pulsar
    chart: pulsar-2.0.10
    release: RELEASE-NAME
    heritage: Helm
    component: brokersts
    cluster: pulsar
data:
  zookeeperServers:
    pulsar-zookeeper-ca:2181
  configurationStoreServers:
    pulsar-zookeeper-ca:2181
  clusterName: pulsar
  allowAutoTopicCreationType: "non-partitioned"
  PULSAR_EXTRA_OPTS: "-Dpulsar.log.root.level=info"
  PULSAR_GC: "-XX:+UseG1GC"
  PULSAR_LOG_LEVEL: "info"
  PULSAR_LOG_ROOT_LEVEL: "info"
  PULSAR_MEM: "-Xms2g -Xmx2g -XX:MaxDirectMemorySize=2g -Dio.netty.leakDetectionLevel=disabled -Dio.netty.recycler.linkCapacity=1024 -XX:+ExitOnOutOfMemoryError"
  backlogQuotaDefaultRetentionPolicy: "producer_exception"
  brokerDeduplicationEnabled: "false"
  exposeConsumerLevelMetricsInPrometheus: "false"
  exposeTopicLevelMetricsInPrometheus: "true"

pulsar-function-extra:

apiVersion: v1
kind: ConfigMap
metadata:
  name: "pulsar-function-extra"
  namespace: monitoring
  labels:
    app: pulsar
    chart: pulsar-2.0.10
    release: RELEASE-NAME
    heritage: Helm
    component: function
    cluster: pulsar
data:
  PULSAR_EXTRA_OPTS: "-Dpulsar.log.root.level=info"
  PULSAR_GC: "-XX:+UseG1GC"
  PULSAR_LOG_LEVEL: "info"
  PULSAR_LOG_ROOT_LEVEL: "info"
  PULSAR_MEM: "-Xms2g -Xmx2g -XX:MaxDirectMemorySize=2g -XX:+ExitOnOutOfMemoryError"

pulsar-proxy:

apiVersion: v1
kind: ConfigMap
metadata:
  name: "pulsar-proxy"
  namespace: monitoring
  labels:
    app: pulsar
    chart: pulsar-2.0.10
    release: RELEASE-NAME
    heritage: Helm
    component: proxy
    cluster: pulsar
data:
  brokerServiceURL: "pulsar://pulsar-broker:6650"
  brokerServiceURLTLS: "pulsar+ssl://pulsar-broker:6651"
  brokerWebServiceURL: "http://pulsar-broker:8080"
  brokerWebServiceURLTLS: "https://pulsar-broker:8443"
  zookeeperServers:
    pulsar-zookeeper-ca:2181
  configurationStoreServers:
    pulsar-zookeeper-ca:2181
  PULSAR_EXTRA_OPTS: "-Dpulsar.log.root.level=info"
  PULSAR_GC: "-XX:+UseG1GC"
  PULSAR_LOG_LEVEL: "info"
  PULSAR_LOG_ROOT_LEVEL: "info"
  PULSAR_MEM: "-Xms1g -Xmx1g -XX:MaxDirectMemorySize=1g"
  numHttpServerThreads: "10"

pulsar-proxy-ws

apiVersion: v1
kind: ConfigMap
metadata:
  name: "pulsar-proxy-ws"
  namespace: monitoring
  labels:
    app: pulsar
    chart: pulsar-2.0.10
    release: RELEASE-NAME
    heritage: Helm
    component: proxy
    cluster: pulsar
data:
  brokerServiceUrl: "pulsar://pulsar-broker:6650"
  brokerServiceUrlTls: "pulsar+ssl://pulsar-broker:6651"
  serviceUrl: "http://pulsar-broker:8080"
  serviceUrlTls: "https://pulsar-broker:8443"
  zookeeperServers:
    "pulsar-zookeeper-ca:2181"
  configurationStoreServers:
    "pulsar-zookeeper-ca:2181"
  clusterName: pulsar
  webServicePort: "8000"
  PULSAR_EXTRA_OPTS: "-Dpulsar.log.root.level=info"
  PULSAR_GC: "-XX:+UseG1GC"
  PULSAR_LOG_LEVEL: "info"
  PULSAR_LOG_ROOT_LEVEL: "info"
  PULSAR_MEM: "-Xms1g -Xmx1g -XX:MaxDirectMemorySize=1g"
  numHttpServerThreads: "10"

pulsar-zookeepernp

apiVersion: v1
kind: ConfigMap
metadata:
  name: "pulsar-zookeepernp"
  namespace: monitoring
  labels:
    app: pulsar
    chart: pulsar-2.0.10
    release: RELEASE-NAME
    heritage: Helm
    component: zookeepernp
    cluster: pulsar
data:
  PULSAR_EXTRA_OPTS: "-Dzookeeper.tcpKeepAlive=true -Dzookeeper.clientTcpKeepAlive=true -Dpulsar.log.root.level=info"
  PULSAR_GC: "-XX:+UseG1GC"
  PULSAR_LOG_LEVEL: "info"
  PULSAR_LOG_ROOT_LEVEL: "info"
  PULSAR_MEM: "-Xms1g -Xmx1g -Dcom.sun.management.jmxremote -Djute.maxbuffer=10485760"

pulsar-zookeeper:

apiVersion: v1
kind: ConfigMap
metadata:
  name: "pulsar-zookeeper"
  namespace: monitoring
  labels:
    app: pulsar
    chart: pulsar-2.0.10
    release: RELEASE-NAME
    heritage: Helm
    component: zookeeper
    cluster: pulsar
data:
  PULSAR_EXTRA_OPTS: "-Dzookeeper.tcpKeepAlive=true -Dzookeeper.clientTcpKeepAlive=true -Dpulsar.log.root.level=info"
  PULSAR_GC: "-XX:+UseG1GC"
  PULSAR_LOG_LEVEL: "info"
  PULSAR_LOG_ROOT_LEVEL: "info"
  PULSAR_MEM: "-Xms1g -Xmx1g -Dcom.sun.management.jmxremote -Djute.maxbuffer=10485760"

michaeljmarshall

@nuriel77 - thank you for your contribution. I verified these changes locally on my machine, as well. The changes look good, except for one more place that needs to be updated. Can you also update this template too? Thanks.

pulsar-helm-chart/helm-chart-sources/pulsar/templates/bookkeeper/bookkeeper-configmap.yaml

Lines 71 to 74 in 3fd374a

    
           {{ toYaml .Values.bookkeeper.configData | indent 2 }} 
        
             # Workaround for double-quoted values in old values files 
        
             BOOKIE_MEM: {{ .Values.bookkeeper.configData.BOOKIE_MEM }} 
        
             BOOKIE_GC: {{ .Values.bookkeeper.configData.BOOKIE_GC }}

nuriel77 · 2021-12-23T20:56:43Z

@michaeljmarshall you're right. I missed those because I only searched for PULSAR_(GC|MEM) :)
Fixed (found autorecovery as well)

michaeljmarshall · 2021-12-23T20:58:31Z

@michaeljmarshall I wasn't sure if I have to bump the Chart's version or is that something your automation takes care of.

I'll take care of bumping the version.

@michaeljmarshall you're right. I missed those because I only searched for PULSAR_(GC|MEM) :) Fixed (found autorecovery as well)

Thanks for finding that other reference.

michaeljmarshall

LGTM

michaeljmarshall · 2021-12-23T21:09:54Z

It looks like our Circle CI tests didn't run. I think that is likely because this is from a new fork of our helm chart. I'll need to look into this before we can merge the PR.

michaeljmarshall · 2021-12-23T21:17:36Z

@nuriel77 - based on reading through this doc https://circleci.com/docs/2.0/oss/#build-pull-requests-from-forked-repositories, I am wondering if the following applies to your fork:

Note: If a user submits a pull request to your repository from a fork, but no pipeline is triggered, then the user most likely is following a project fork on their personal account rather than the project itself of CircleCi, causing the jobs to trigger under the user’s personal account and not the organization account. To resolve this issue, have the user unfollow their fork of the project on CircleCI and instead follow the source project. This will trigger their jobs to run under the organization when they submit pull requests.

We have Build forked pull requests set to true or enabled in our Circle CI account. If that's not the case, I'll need to keep debugging. Thanks!

nuriel77 · 2021-12-24T12:04:57Z

I am wondering if the following applies to your fork:

I guess it is. Never worked with CircleCI.

In any case, I don't think I could have pushed a PR directly to the repository, but only via a fork, right?

michaeljmarshall · 2022-01-04T07:04:10Z

I guess it is. Never worked with CircleCI.

That's helpful. I assumed that the documentation meant something specific for CircleCI. Since you've never worked with it before, maybe this line (then the user most likely is following a project fork on their personal account rather than the project itself of CircleCi) means something GitHub specific.

In any case, I don't think I could have pushed a PR directly to the repository, but only via a fork, right?

That is correct. I have definitely seen tests run for PRs opened in forks, so it should work. I'll take a closer look in about 12 hours.

michaeljmarshall · 2022-01-04T07:13:08Z

I am going to close and reopen this PR to see if it triggers the CI. It's possible that my initial "Request Changes" prevented CI from running.

Fix configMap key value template

3d50bfe

michaeljmarshall requested review from michaeljmarshall, cdbartholomew and lhotari December 23, 2021 16:15

michaeljmarshall requested changes Dec 23, 2021

View reviewed changes

Add fix for bookeeper and autorecovery configMaps

d949eca

michaeljmarshall approved these changes Dec 23, 2021

View reviewed changes

michaeljmarshall closed this Jan 4, 2022

michaeljmarshall reopened this Jan 4, 2022

lhotari approved these changes Jan 4, 2022

View reviewed changes

lhotari merged commit 7869558 into datastax:master Jan 4, 2022

lhotari mentioned this pull request Jan 4, 2022

Remove duplicate PULSAR_MEM and PULSAR_GC keys from ConfigMaps #107

Closed

michaeljmarshall mentioned this pull request Jan 4, 2022

Key duplication in configMaps causing errors on some deployment tools #103

Closed

michaeljmarshall linked an issue Jan 4, 2022 that may be closed by this pull request

Key duplication in configMaps causing errors on some deployment tools #103

Closed

nuriel77 mentioned this pull request Mar 21, 2022

Fix/config quotes datastax/pulsar#43

Closed

4 tasks

lhotari mentioned this pull request Mar 21, 2022

Remove invalid calls to "bin/apply-config-from-env.py conf/bkenv.sh" #163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix configMap key value template #110

Fix configMap key value template #110

nuriel77 commented Dec 23, 2021 •

edited

Loading

michaeljmarshall left a comment

nuriel77 commented Dec 23, 2021

michaeljmarshall commented Dec 23, 2021

michaeljmarshall left a comment

michaeljmarshall commented Dec 23, 2021

michaeljmarshall commented Dec 23, 2021

nuriel77 commented Dec 24, 2021

michaeljmarshall commented Jan 4, 2022

michaeljmarshall commented Jan 4, 2022

	{{ toYaml .Values.bookkeeper.configData \| indent 2 }}
	# Workaround for double-quoted values in old values files
	BOOKIE_MEM: {{ .Values.bookkeeper.configData.BOOKIE_MEM }}
	BOOKIE_GC: {{ .Values.bookkeeper.configData.BOOKIE_GC }}

Fix configMap key value template #110

Fix configMap key value template #110

Conversation

nuriel77 commented Dec 23, 2021 • edited Loading

Overview

Tested Values

Outputs

michaeljmarshall left a comment

Choose a reason for hiding this comment

nuriel77 commented Dec 23, 2021

michaeljmarshall commented Dec 23, 2021

michaeljmarshall left a comment

Choose a reason for hiding this comment

michaeljmarshall commented Dec 23, 2021

michaeljmarshall commented Dec 23, 2021

nuriel77 commented Dec 24, 2021

michaeljmarshall commented Jan 4, 2022

michaeljmarshall commented Jan 4, 2022

nuriel77 commented Dec 23, 2021 •

edited

Loading