Charts improvements and bug fixes #164

iosdev747 · 2024-09-16T07:34:21Z

PR changes:

NPE fixes
added JVM remote debug flags
wget cert validation under flag for VPN issues (blocked in internal N/W)
added values file for local setup

TODO:

cleanup
readme update
remote args under flag

kmrdhruv · 2024-09-16T07:57:01Z

README.md

@@ -62,6 +62,11 @@ OR

 Start [varadhi local \[run\]](.run%2Fvaradhi%20local%20%5Brun%5D.run.xml) IntelliJ run profile.

+## k8s Deployment
+
+```helm install varadhi setup/helm/varadhi -f setup/helm/varadhi/fcp.values.yaml```


This is server ? Should we call it indicate that ?.

setup/docker/Dockerfile

kmrdhruv · 2024-09-16T08:16:43Z

setup/docker/Dockerfile

@@ -66,5 +75,7 @@ RUN sed -i 's|src/main/resources/|/etc/varadhi/|' /etc/varadhi/configuration.yml
 USER varadhi
 WORKDIR /usr/share/varadhi

+RUN env


This is for debug purposes ? If yes, should it be at the beginning after env block ?

removed it, accidentally added it

kmrdhruv · 2024-09-16T08:28:37Z

setup/docker/scripts/entrypoint.sh

@@ -15,5 +15,8 @@ JAVA_OPTS="$JAVA_OPTS -Dcom.sun.management.jmxremote.port=9990"
 JAVA_OPTS="$JAVA_OPTS -Dcom.sun.management.jmxremote.authenticate=false"
 JAVA_OPTS="$JAVA_OPTS -Dcom.sun.management.jmxremote.ssl=false"
 JAVA_OPTS="$JAVA_OPTS -Dlog4j2.configurationFile=/etc/varadhi/log4j2.xml"
+if "$APP_DEBUG"; then
+    JAVA_OPTS="$JAVA_OPTS -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:5005"


let port be configurable with default ?
Also thinking if we should follow a convention, current environment variables don't. Something like [] ?

kmrdhruv · 2024-09-17T10:45:01Z

setup/helm/test/service-server.yaml

@@ -0,0 +1,25 @@
+apiVersion: v1


is this intended ? There is already service-service.yaml

no, I left it here for my reference. Will remove it.

kmrdhruv · 2024-09-17T10:49:58Z

setup/helm/varadhi/templates/_server.tpl

@@ -65,4 +65,9 @@ featureFlags:

 {{ template "configMap.metastore.zookeeper" . }}

+{{ with .Values.varadhi.app.controller }}


may be we shouldn't call this file as server anymore as it will be common for all roles ?

kmrdhruv · 2024-09-17T10:51:41Z

setup/helm/varadhi/templates/app-configmap.yaml

@@ -12,6 +12,6 @@ metadata:
    component: {{ .Values.deployment.name }}
 data:
  configuration.yml: |
-    {{- if eq .Values.deployment.name "server" }}
+    {{- if or (eq .Values.deployment.name "server") (eq .Values.deployment.name "controller") (eq .Values.deployment.name "consumer") }}


same as above, i think we can keep it as global app config and avoid component specific conditions ?

kmrdhruv · 2024-09-17T10:55:51Z

setup/helm/varadhi/templates/deployment.yaml

@@ -57,6 +57,13 @@ spec:
          {{- end }}
          resources:
          {{- toYaml .Values.deployment.resources | nindent 12 }}
+          ports:
+            - containerPort: 9990 # jmx remote port


configurable port numbers ?

kmrdhruv · 2024-09-17T11:21:33Z

setup/helm/varadhi/values.yaml

@@ -212,12 +219,12 @@ messaging:
  configMapName: messaging
  pulsar:
    adminOptions:
-      serviceHttpUrl: "http://192.168.1.3:8080"
+      serviceHttpUrl: "http://host.docker.internal:8080"


may be time to decide the hierarchy of values files. Suggestion/TBD : values -> fcp.values -> role specific i.e (controller|consumer|server|standalone) and override only required content in higher level files ?

kmrdhruv · 2024-09-17T11:29:14Z

setup/docker/scripts/entrypoint.sh

@@ -16,4 +16,11 @@ JAVA_OPTS="$JAVA_OPTS -Dcom.sun.management.jmxremote.authenticate=false"
 JAVA_OPTS="$JAVA_OPTS -Dcom.sun.management.jmxremote.ssl=false"
 JAVA_OPTS="$JAVA_OPTS -Dlog4j2.configurationFile=/etc/varadhi/log4j2.xml"

+# If debug flag is set(1 or true) then enable remote debugging
+if [[ ! -z "$APP_DEBUG" ]]; then
+    if [[ "$APP_DEBUG" == "1" || "$APP_DEBUG" == "true" ]]; then


any issue being it just set to any value ?

no, idea was if someone sets to false or 0 or any other invalid value, we don't enable it.

Why two levels of if block needed, only inner block should suffice ?

kmrdhruv · 2024-09-17T12:06:06Z

server/src/main/java/com/flipkart/varadhi/config/MemberConfig.java

@@ -9,4 +9,6 @@ public class MemberConfig {
    private int clusterPort;
    private int maxQps;
    private int networkMBps;
+    private int cpuCount;
+    private int nicMBps;
 }


instead we can adjust the .values file to have new config.

kmrdhruv · 2024-10-17T06:18:43Z

setup/docker/scripts/entrypoint.sh

@@ -16,4 +16,11 @@ JAVA_OPTS="$JAVA_OPTS -Dcom.sun.management.jmxremote.authenticate=false"
 JAVA_OPTS="$JAVA_OPTS -Dcom.sun.management.jmxremote.ssl=false"
 JAVA_OPTS="$JAVA_OPTS -Dlog4j2.configurationFile=/etc/varadhi/log4j2.xml"

+# If debug flag is set(1 or true) then enable remote debugging
+if [[ ! -z "$APP_DEBUG" ]]; then
+    if [[ "$APP_DEBUG" == "1" || "$APP_DEBUG" == "true" ]]; then


Why two levels of if block needed, only inner block should suffice ?

kmrdhruv · 2024-10-17T06:52:51Z

setup/helm/varadhi/values.yaml

      connectionTimeoutMs: 2000
      readTimeoutMs: 2000
      requestTimeoutMs: 2000
    clientOptions:
-      serviceUrl: "http://192.168.1.3:8080"
+      serviceUrl: "http://host.docker.internal:8080"


create standalone values file to override this, as value is specific to the standalone setup.

kmrdhruv · 2024-10-17T07:09:14Z

setup/helm/varadhi/values/controller_values.yaml

@@ -0,0 +1,240 @@
+# Default values for varadhi.


This is duplicating the entire values files for each component, that shouldn't be needed. What is the proposed hierarchy ?

iosdev747 added 3 commits September 16, 2024 12:44

add debug options and NPE fixes

45c8415

keep app configs in all members

4f390b1

add debug flags adding ports and java args

0e44230

kmrdhruv requested changes Sep 17, 2024

View reviewed changes

kmrdhruv reviewed Sep 17, 2024

View reviewed changes

iosdev747 added 4 commits September 18, 2024 14:01

add env vars for jvm args and debug mode

f528ae0

Merge remote-tracking branch 'upstream/master' into charts_improvements

d7499d3

add otlp configs in helm charts

4fc3c8a

port related minor refactors

508336d

kmrdhruv requested changes Oct 17, 2024

View reviewed changes

anuj-flipkart linked an issue Oct 24, 2024 that may be closed by this pull request

Charts improvements and bug fixes #175

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Charts improvements and bug fixes #164

Charts improvements and bug fixes #164

iosdev747 commented Sep 16, 2024

kmrdhruv Sep 16, 2024

kmrdhruv Sep 16, 2024

iosdev747 Sep 18, 2024

kmrdhruv Sep 16, 2024

kmrdhruv Sep 17, 2024

iosdev747 Sep 18, 2024

kmrdhruv Sep 17, 2024

kmrdhruv Sep 17, 2024

kmrdhruv Sep 17, 2024

kmrdhruv Sep 17, 2024

kmrdhruv Sep 17, 2024

iosdev747 Oct 12, 2024

kmrdhruv Oct 17, 2024

kmrdhruv Sep 17, 2024 •

edited

Loading

kmrdhruv Oct 17, 2024

kmrdhruv Oct 17, 2024

kmrdhruv Oct 17, 2024

		@@ -65,4 +65,9 @@ featureFlags:

		{{ template "configMap.metastore.zookeeper" . }}

		{{ with .Values.varadhi.app.controller }}

Charts improvements and bug fixes #164

Are you sure you want to change the base?

Charts improvements and bug fixes #164

Conversation

iosdev747 commented Sep 16, 2024

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

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

kmrdhruv Sep 17, 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

kmrdhruv Sep 17, 2024 •

edited

Loading