[BUG] KNN memory circuit breaker limit in opensearch.yaml

[frejonb]

Describe the bug
The setting knn.memory.circuit_breaker.limit doesn't seem to be respected when set in the opensearch.yaml. Even setting it in /_cluster/settings does not work unless I remove the setting from opensearch.yaml

To Reproduce
Steps to reproduce the behavior:

1. Set knn.memory.circuit_breaker.limit: 56200000KB in opensearch.yaml
2. Warm up an index
3. Check /_plugins/_knn/*/stats and calculate graph_memory_usage*100/graph_memory_usage_percentage. This will be 50% of the native memory.
4. Set knn.memory.circuit_breaker.limit: 56200000KB in the persistent settings (/_cluster/settings) and repeat 2-3.
5. Now remove knn.memory.circuit_breaker.limit: 56200000KB from opensearch.yaml and repeat 2-3. This time the result should be 56200000.

Expected behavior
The opensearch.yaml setting should be picked up correctly.

Plugins
Please list all plugins currently enabled.

Screenshots
If applicable, add screenshots to help explain your problem.

Host/Environment (please complete the following information):

• OS: [e.g. iOS]
• Version [e.g. 22]

Additional context
Add any other context about the problem here.

[saratvemulapalli]

Thanks @frejonb settings is something we use everyday, lets start with how K-NN is handling the settings consumer and traceback if OpenSearch has problems.

[vamshin]

Hi @frejonb, we read from cluster settings to enable disable circuit breaker. Please use just cluster settings.

[frejonb]

@vamshin but it’s harder to have IaC if I can’t set it in the config file

[vamshin]

@frejonb sorry whats laC? Having it part of the config file requires process restart to read updated configuration which is not desirable. Also if there are multiple nodes this becomes even harder to update config. So we have cluster settings that can be changed dynamically with out having to do any restarts and do not have to worry about changing across all the nodes manually.

[frejonb]

@vamshin thanks for the quick reply. I meant infrastructure as code. It's easier to have all my configuration manifests in source control and then easily and quickly deploy properly functional clusters on demand. Having one config file per node is not an issue because all nodes get the same config automatically (though a statefulset for example).

I'm not suggesting to remove the ability to dynamically change the settings, but to support modern DevOps practices. Also other configuration (from ES) seems to work that way.

[jmazanec15]

Hi @frejonb, Im looking into this now. How much RAM does your machine have? I did a quick test of this and it appears the setting can be set in the opensearch yml file and show up in the cluster settings. That being said, could you check if when you set "knn.memory.circuit_breaker.limit": "1%", the same behavior shows up?

[jmazanec15]

Actually, I was able to reproduce the behavior. Checking what might be the cause.

[jmazanec15]

I see the problem. Instead of pulling the value for knn.memory.circuit_breaker.limit from the settings, we pull it from a map that sits on top of the settings (ref). This map will actually get updated with the latest settings before the settings are applied, because we update it as a settings update consumer (ref).

The problem is that when reading the settings from "opensearch.yml", this update settings consumer never gets called, so latest settings is never updated, so the cache uses the default value for the circuit breaker limit.

[frejonb]

@jmazanec15 thanks for the explanation. Is this the only setting affected by this?

[jmazanec15]

Yes, the other dynamic settings will also be impacted. Im working on fix now. Might require some refactoring in KNNSettings class.

[jmazanec15]

Merged the fix. This will be fixed in 2.6.