zilliztech / milvus-helm

Apache License 2.0
54 stars 41 forks source link

[milvus] Ability to use user specified ConfigMap for Milvus configuration #97

Closed nustiueudinastea closed 2 months ago

nustiueudinastea commented 3 months ago

What this PR does / why we need it:

This PR adds an extra config field which allows users to use a completely custom ConfigMap for configuring Milvus. This is useful for situation where dynamic info needs to be added to the configmap for various reasons (example: configuring storage account ID based on some automatically provisioned resource).

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

sre-ci-robot commented 3 months ago

Welcome @nustiueudinastea! It looks like this is your first PR to zilliztech/milvus-helm 🎉

LoveEachDay commented 3 months ago

@nustiueudinastea This is not working as you expected. By default milvus helm chart will generate a default.yaml config which will generates all the dependencies configurations like etcd, minio, and message queue. In your pull request, you mount to the same path which will generate side effect.

It's recommended to provide custom configuration through extraConfigFiles.user.yaml instead.

nustiueudinastea commented 3 months ago

@LoveEachDay as far as I understand, it does what I think it does. It allows a user to TOTALLY override the config and provide a fully customised config. This is very useful in some situations because you can't dynamically pass values to the chart, but you can easily create dynamic config maps with custom tailored configs. We have an operator that creates storage accounts for Milvus and that way we can customise the deployment without having to pass values (which are static).

The extraConfigFiles.user.yaml is yet another static way of configuring the chart, by passing values.

nustiueudinastea commented 2 months ago

hey @LoveEachDay any chance we can continue with this?

haorenfsa commented 2 months ago

Hi @nustiueudinastea, it seems reasonable feature to me. 2 things to note:

  1. The mounted file better be named as user.yaml which will have the highest priority when being processed by Milvus, thus make sure the configuration is not overwrited ( the default.yaml is to keep the default configs of this Milvus version. )
  2. By using subpath, kubelet won't be able to dynamically update it in the Pod. For now you still have to exec into the pod to make the actual dynamic change happen. There will be a patch later to solve this. #94
haorenfsa commented 2 months ago

Oh, by the way, if possible, plz also add an values file in https://github.com/zilliztech/milvus-helm/tree/master/charts/milvus/ci for integration test.

haorenfsa commented 2 months ago

Hi @nustiueudinastea, the changes looks good. Before we could merge it, please resolve the conflicts with current master by git rebase,

nustiueudinastea commented 2 months ago

@haorenfsa ready.

haorenfsa commented 2 months ago

We're getting close. Please also bump the version like other PRs did, check this for example: https://github.com/zilliztech/milvus-helm/pull/89/files @nustiueudinastea

nustiueudinastea commented 2 months ago

@haorenfsa bumped version as well. I bumped the patch version number, even though this PR includes an extra parameter. Should I bump the minor version?

haorenfsa commented 2 months ago

@nustiueudinastea It's OK. Thank you again for the contribution!

sre-ci-robot commented 2 months ago

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: haorenfsa, nustiueudinastea

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/zilliztech/milvus-helm/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment