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

Adds initContainer option to the helmcharts and allows default QW_CONFIG to be overridden by chart values. #92

Merged

Conversation

hgudladona
Copy link

PR for #91

@fmassot fmassot requested a review from idrissneumann June 26, 2024 14:52
charts/quickwit/templates/metastore-deployment.yaml Outdated Show resolved Hide resolved
charts/quickwit/templates/searcher-statefulset.yaml Outdated Show resolved Hide resolved
charts/quickwit/templates/job-create-sources.yaml Outdated Show resolved Hide resolved
charts/quickwit/templates/job-create-indices.yaml Outdated Show resolved Hide resolved
charts/quickwit/templates/janitor-deployment.yaml Outdated Show resolved Hide resolved
charts/quickwit/templates/indexer-statefulset.yaml Outdated Show resolved Hide resolved
charts/quickwit/templates/control-plane-deployment.yaml Outdated Show resolved Hide resolved
@idrissneumann
Copy link
Collaborator

Hi thanks for your contribution, I made few comments following some tests.

Thanks in advance :)

@hgudladona
Copy link
Author

Hello, Thanks for the review, I made the fixes based on your suggestions. However, I am still working on testing the changes. I will keep you posted once I confirm everything is working as expected.

@fmassot
Copy link
Contributor

fmassot commented Jun 27, 2024

@hgudladona do you need another look on your PR?

@hgudladona
Copy link
Author

I have fixed the syntax issues and tested the rendering. I am working on rolling out the components onto my EKS cluster and ensure functionality. I will let you know after this test. I am pretty close. Would you like me to move this PR to draft state?

@fmassot
Copy link
Contributor

fmassot commented Jun 27, 2024

I have fixed the syntax issues and tested the rendering. I am working on rolling out the components onto my EKS cluster and ensure functionality. I will let you know after this test. I am pretty close. Would you like me to move this PR to draft state

NIce. Keep it as is, that's fine. Let us know the outcome of your tests on your EKS cluster!

Copy link
Collaborator

@idrissneumann idrissneumann left a comment

Choose a reason for hiding this comment

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

Hi. I made other feedbacks and mark the previous ones as solved.

Thanks :)

charts/quickwit/values.yaml Outdated Show resolved Hide resolved
charts/quickwit/values.yaml Outdated Show resolved Hide resolved
charts/quickwit/values.yaml Outdated Show resolved Hide resolved
charts/quickwit/values.yaml Outdated Show resolved Hide resolved
charts/quickwit/values.yaml Outdated Show resolved Hide resolved
charts/quickwit/values.yaml Outdated Show resolved Hide resolved
charts/quickwit/values.yaml Outdated Show resolved Hide resolved
charts/quickwit/values.yaml Outdated Show resolved Hide resolved
charts/quickwit/values.yaml Outdated Show resolved Hide resolved
charts/quickwit/values.yaml Outdated Show resolved Hide resolved
@hgudladona hgudladona marked this pull request as draft June 28, 2024 14:02
@hgudladona hgudladona force-pushed the quickwit-oss-helm-chart-91 branch from 4830aa1 to ac9d0bf Compare July 2, 2024 22:03
@hgudladona hgudladona marked this pull request as ready for review July 2, 2024 22:06
@hgudladona
Copy link
Author

hgudladona commented Jul 2, 2024

Hello, I have made necessary fixes and tested these templates while integrating to export my spark logs via Vector --> Warpstream --> Quickwit on AWS. Please take a look at the proposed changes and let me know.

The init container is particularly useful as follows. Having this in the jobs template allows a new config file to be created while substituting AWS_AVAILABILITY_ZONE with the availability_zone from instance metadata during startup from the source config below. This allows the indexers to pull logs from warpstream rack locally (AZ local) preventing unnecessary data transfer costs. Reference to Warpstream docs is here

jobs:
  sources:
    command: ["/bin/bash", "-c", "quickwit source describe --index hudi-spark-logs --source hudi-spark-logs-warpstream --endpoint ${QW_CLUSTER_ENDPOINT} || quickwit source create --index hudi-spark-logs --source-config /quickwit/shared/hudi-spark-logs-warpstream.yaml --endpoint ${QW_CLUSTER_ENDPOINT}"]
    initContainers:
      - name: quickwit-rack-aware-kafka-client
        image: alpine:3.20
        command:
          - "sh"
          - "-c"
          - "apk --update add curl && \
             cp /quickwit/hudi-spark-logs-warpstream.yaml /quickwit/shared/ && \
             TOKEN=$(curl -XPUT \"http://169.254.169.254/latest/api/token\" -H \"X-aws-ec2-metadata-token-ttl-seconds: 21600\") && \
             AVAILABILITY_ZONE=$(curl -XGET -H \"X-aws-ec2-metadata-token: $TOKEN\" http://169.254.169.254/latest/meta-data/placement/availability-zone) && \
             sed -i.bak s/AWS_AVAILABILITY_ZONE/$AVAILABILITY_ZONE/g /quickwit/shared/hudi-spark-logs-warpstream.yaml"
        volumes:
          - name: source
            configMap:
              name: quickwit-bootstrap
              items:
                  - key: hudi-spark-logs-warpstream.yaml
                    path: hudi-spark-logs-warpstream.yaml
        volumeMounts:
          - name: source
            mountPath: /quickwit/hudi-spark-logs-warpstream.yaml
            subPath: hudi-spark-logs-warpstream.yaml
          - name: source-shared
            mountPath: /quickwit/shared/

    # A shared volume for the init container to copy the configuration files to
    extraVolumes:
      - name: source-shared
        emptyDir: {}

    # A shared volume mount for the init container to copy the configuration files to
    extraVolumeMounts:
      - name: source-shared
        mountPath: /quickwit/shared/
        readOnly: true

...

  sources:
    - index: hudi-spark-logs
      source:
        version: 0.8
        source_id: hudi-spark-logs-warpstream
        source_type: kafka
        num_pipelines: 5
        params:
          topic: hudi-spark-logs
          client_params:
            client.id: warpstream_az=AWS_AVAILABILITY_ZONE,warpstream_interzone_lb=true
            bootstrap.servers: warpstream-agent.warpstream:9092

@idrissneumann
Copy link
Collaborator

Hi LGTM, I'll run some tests with kind ASAP (probably tomorrow) and merge it if it's okay.

Thanks for your work!

@idrissneumann idrissneumann self-requested a review July 4, 2024 08:30
Copy link
Collaborator

@idrissneumann idrissneumann left a comment

Choose a reason for hiding this comment

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

Following the result of the tests, we still have some changes to make ^^

Thanks

charts/quickwit/templates/metastore-deployment.yaml Outdated Show resolved Hide resolved
Use nodeSelector, affinity, tolerations from bootstrap config values. This ensures pods generated from k8s Job have these settings propagated.
Allow command and args to be overridden via values files. This helps in providing custom config files generated from templates as necessary
@hgudladona hgudladona force-pushed the quickwit-oss-helm-chart-91 branch from ac9d0bf to 198152e Compare July 8, 2024 14:17
@idrissneumann idrissneumann self-requested a review July 9, 2024 13:41
@idrissneumann
Copy link
Collaborator

idrissneumann commented Jul 10, 2024

Tests with this value file:

---
config:
  default_index_root_uri: s3://qw-indexes
  storage:
    s3:
      endpoint: https://s3.fr-par.scw.cloud
      region: fr-par
      access_key_id: SCWXXXXXXX
      secret_access_key: XXXXXXXXXXX

environment:
  QW_METASTORE_URI: s3://qw-indexes

indexer:
  initContainers: &initContainers
    - name: hello
      image: busybox:1.28
      command: ['echo', 'heyyyyy']

searcher:
  initContainers: *initContainers

janitor:
  initContainers: *initContainers

control_plane:
  initContainers: *initContainers

metastore:
  initContainers: *initContainers

Result ok ✅:

$ helm template . --values values.yaml --values values2.yaml --namespace=quickwit|kubectl -n quickwit apply -f -
$ kubectl -n quickwit logs release-name-quickwit-searcher-0 -c hello
heyyyyy
$ kubectl -n quickwit logs release-name-quickwit-indexer-0 -c hello
heyyyyy

@idrissneumann
Copy link
Collaborator

idrissneumann commented Jul 10, 2024

Other test changing the location of the config file :

---
configLocation: /tmp/yo.yml
config:
  default_index_root_uri: s3://qw-indexes
  storage:
    s3:
      endpoint: https://s3.fr-par.scw.cloud
      region: fr-par
      access_key_id: SCWXXXXXX
      secret_access_key: XXXXXXXX

environment:
  QW_METASTORE_URI: s3://qw-indexes

indexer:
  initContainers: &initContainers
    - name: show
      image: busybox:1.28
      command: ['cat', '/tmp/yo.yaml']
    - name: move
      image: busybox:1.28
      command: ['cp', '/tmp/yo.yaml', '/quickwit/node.yaml']

searcher:
  initContainers: *initContainers

janitor:
  initContainers: *initContainers

control_plane:
  initContainers: *initContainers

metastore:
  initContainers: *initContainers

Test ko ❌:

$ kubectl -n quickwit logs release-name-quickwit-searcher-0 -c show
cat: can't open '/tmp/yo.yaml': No such file or directory

The value field configLocation is only overriding the environment variable:

- name: QW_CONFIG
  value: {{ .Values.configLocation }}

But I think it should also change the configmap path mounted into the pods. Maybe It'll be good if we get the opinion of other reviewers on this. On my side, I think changing only the environment variable to tell quickwit "the config file is there" on one side and continue to write it in /quickwit/node.yaml on the other side seems a bit strange to me even if I understand you want to regenerate a new file autonomously from your initContainers.

Copy link
Collaborator

@idrissneumann idrissneumann left a comment

Choose a reason for hiding this comment

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

See the last test

@hgudladona
Copy link
Author

Usually config maps are immutable from the pod itself, a more probable usage would be to mount an emptyDir to the pod's init and main containers where the mutated config is copied to. If the user IS doing the mutation of the config I'd expect they would also update the configLocation.

@idrissneumann
Copy link
Collaborator

idrissneumann commented Jul 11, 2024

Usually config maps are immutable from the pod itself, a more probable usage would be to mount an emptyDir to the pod's init and main containers where the mutated config is copied to. If the user IS doing the mutation of the config I'd expect they would also update the configLocation.

Explanation good enough to me, I agree for the configmap immutability. Let's see if in the future other users will have issues with that. For now, we can deal with overriding completely the config from an initContainers with this version.

So LGTM, I think there is some linter issues to fix (see the github action pipelines) and we're good to merge :)

Thanks for your work

@idrissneumann idrissneumann self-requested a review July 11, 2024 08:46
Copy link
Collaborator

@idrissneumann idrissneumann left a comment

Choose a reason for hiding this comment

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

We just have to fix the linter issues and increment the Chart.yaml version (we can increment the last digit because there's no breaking changes, so 0.6.2 at the time of writing).

EDIT: the linter issue is only the missing bump of the Chart.yaml, so only one thing remaining and we're good to go ^^

Thanks in advance

@hgudladona hgudladona requested a review from idrissneumann July 11, 2024 14:09
@idrissneumann idrissneumann merged commit 45e10ba into quickwit-oss:main Jul 11, 2024
1 check passed
@fmassot
Copy link
Contributor

fmassot commented Jul 11, 2024

thank you @hgudladona and @idrissneumann for you work!

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.

4 participants