Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only download kubectl and provide volume if graylog version is < 4.2.0-0 #154

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tuxBurner
Copy link

Only download kubectl and provide volume if graylog version is < 4.2.0-0

I recognized that the kubectl is always downloaded. But it is only used in the lifecycle whe tne graylog version is < 4.2.0-0

Which issue this PR fixes

Special notes for your reviewer

Checklist

  • [x ] DCO signed
  • [ x] Chart Version bumped

@KongZ
Copy link
Owner

KongZ commented Jan 2, 2024

The kubectl is using for setting the label to master node. The chart uses label mechanism to choose the master node.

@tuxBurner
Copy link
Author

Aaah okay sorry have overseen this

@tuxBurner
Copy link
Author

@KongZ do we really need this mechamism ?

As stated in https://go2docs.graylog.org/5-0/downloading_and_installing_graylog/docker_installation.htm#KubernetesAutomaticMasterSelection we only need to set the POD_NAME env var.

The /docker-entrypoint.sh has a part like this

# check if we are inside kubernetes, Graylog should be run as statefulset and $POD_NAME env var should be defined like this
#          env:
#          - name: POD_NAME
#            valueFrom:
#              fieldRef:
#                fieldPath: metadata.name
# First stateful member is having pod name ended with -0, so
if [[ ! -z "${POD_NAME}" ]]
then
 if echo "${POD_NAME}" | grep "\\-0$" >/dev/null
 then
   export GRAYLOG_IS_LEADER="true"
 else
   export GRAYLOG_IS_LEADER="false"
 fi
fi

@KongZ
Copy link
Owner

KongZ commented Jan 2, 2024

The solution described in the doc above will hard-code pod-0 to be the master.
The solution in this chart will automatically set the other pod to be the master if pod-0 is unable to run.
Yes, it is not necessary if pod-0 is always running. The chart was originally created from Graylog version 2 where the container and application may not be stable. The mechanism to elect the master from the other pod was necessary. 

I believe the recent Graylog versions are stable and we should be able to use pod-0 as the master but I do not have a chance to test them. 

@tuxBurner
Copy link
Author

@KongZ i commented your script out which determines which is the master node and added the suggested env var POD_NAME

It works on my installation :)

@metron2
Copy link
Contributor

metron2 commented Apr 19, 2024

I'm using this change as well and it works much better, especially with the pod security settings (chown is disallowed) but I get an error that there are multiple leaders in the cluster.

/k8s/kubectl --namespace {{ .Release.Namespace }} label --overwrite pod $HOSTNAME graylog-role="coordinating"
fi
fi
#for i in {0..2}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tuxBurner sorry for being late. Instead of commenting out, it would be better to delete them from manifest.

@@ -72,12 +73,14 @@ spec:
rm -rf /usr/share/graylog/data/journal/messagejournal-0
rm -rf /usr/share/graylog/data/journal/recovery-point-offset-checkpoint
{{- end }}
{{- if semverCompare "< 4.2.0-0" ( $graylogVersion ) }}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Graylog 4 is very out of date. We can say the chart no longer supports Graylog version < 4.2.
You can remove all kubectl related.

@@ -1,7 +1,7 @@
apiVersion: v2
name: graylog
home: https://www.graylog.org
version: 2.3.4
version: 2.3.5
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please bump chart version one more.

@metron2
Copy link
Contributor

metron2 commented Jun 11, 2024

@KongZ can close this out now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants