-
Notifications
You must be signed in to change notification settings - Fork 267
feat: support setting default http lua_shared_dict #822
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
Conversation
bzp2010
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solves problems partially, but does not include the prometheus-metrics mentioned in the comment
| # - name: prometheus-metrics | ||
| # size: 20m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example is inappropriate, prometheus is defined using the special command for meta { lua_shared_dict }, which cannot be overridden by http { lua_shared_dict }, but it does override other dict values, such as those used by limit-count.
ref: https://github.com/apache/apisix/blob/master/conf/config.yaml.example#L165-L168
This is a special case for creating shared memory between the HTTP subsystem and the stream subsystem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bzp2010 I actually tried meta but it doesn't work. And overriding it under http does
At least it didn't work for me. Maybe I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can move to another example case that is sure to be ok, so we can merge it first, and if it works on prometheus-metrics, that would solve your problem, and you can report it to community
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dennispan, can you provide your test steps? Maybe we can find the problem from them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used this to test (after a re-deployment to pick up the configuration changes):
/usr/local/openresty/nginx/sbin/nginx -T \
-p /usr/local/apisix/ \
-c conf/nginx.conf \
| grep lua_shared_dict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Baoyuantop @bzp2010 any update on this? Anything else needed from me to move this forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the Prometheus plugin is not enabled under the stream subsystem (default behavior), http { lua_shared_dict } is useful: https://github.com/apache/apisix/blob/58066abc88df37a490f6c04011ed9588a0bda0d1/apisix/cli/ngx_tpl.lua#L329-L331
so this example can be retained.
@dennispan can you fix CI of this PR? you can run make helm-docs in your local and commit changes of docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not work.
The nginx config is like this
lua_shared_dict:
prometheus-metrics: 20m
This created config like this
lua_shared_dict:
- prometheus-metrics: 20m
so this should be
{{- range $dict := .Values.apisix.nginx.luaSharedDicts }}
{{ $dict.name }}: {{ $dict.size }}
https://github.com/apache/apisix/blob/master/conf/config.yaml.example#L250-L276
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Here's the pr: #832
This addresses the need from #769