-
Notifications
You must be signed in to change notification settings - Fork 0
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
plugins configurable with values + decouple from dda #11
Conversation
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, dda-interface-scratch should not be mounted without dda. It has to be controlled one way or another.
As for the general approach of storing all configs in values, I do not think it makes much sense.
If this was commonly useful approach, there would be no point in configmaps, except for very basic mapping some sections of values to files.
values.yaml contains aspects which can be controlled on deployment. Adding entire configs means it some config parts should never be changed since they have to agree with chart internals: subchart dataservers, dummy directories.
which is why configmaps are used to construct config from "hardcoded" (as much as any consitent code coding is "hardcoding") necessary values and some values from values.yaml
Even just enabling plugins must agree with subcharts and dispatcher container, so adding "enable" fields just like this is misleading.
There are, however, some values which should be configurable.
To control this, maybe it is useful to be able to apply some "patches" to the configs, while most of the configs would come from configmaps, setting values as needed for consistency of the chart.
It should be possible.
If tricky with helm (thought I doubt it), maybe we could have some general approach for dispatcher app combining configs from several parts (with conventional names/directories/locations, you probably know it works in various .d directories)
values.yaml
Outdated
|
||
plugins: | ||
osa: | ||
ebabled: true |
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.
ebabled?
templates/deployment.yaml
Outdated
- mountPath: /data/dispatcher_scratch | ||
name: dispatcher-scratch-root | ||
{{- if .Values.plugins.osa.enabled }} |
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.
yes something like this this is needed without dda.
values.yaml
Outdated
dispatcher_mnt_point: /data | ||
data_server_cache: reduced/ddcache | ||
dummy_cache: /data/dummy_prods | ||
data_server_url: http://oda-dda-interface:8000 |
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.
now they are "hardcoded" in the values. Although values is config of the chart, they are not going to be updated.
So it is arguably worse for most plugins, since from these are configs of chart internals, which need to be agreed with other internal details, and should not be at all configurable.
values.yaml
Outdated
enabled: true | ||
instruments: | ||
antares: | ||
data_server_url: http://oda-antares:8000 |
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.
for example is this really supposed to be generally configurable on deployment, when antares backend is deployed along with the dispatcher?
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 it is deployed along with the dispatcher, it will be fixed, you are right. I just imagined the situation when backend is separate, like in case of spi.
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! that's why there below I pointed out that special case. That one option could be configurable.
It could be better if it is configurable with combined configs, and given as example.
Really, it is also possible that other backends are detached. E.g. we did run dda one rather separately.
So if general method is adopted could be nicer. But it's effort to make it consistent throughout.
Non-general method would be to just get fields from values, those that are assumed to be modifiable.
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.
by the way, small detail, no way for you to know, but SPI is an entirely different case than SPI-ACS : different science, different data reduction, software written in different places. They were even built in different countries. If you mention you have SPI analysis to some colleagues in France they might get quite confused.
values.yaml
Outdated
dispatcher_mnt_point: | ||
data_server_cache: | ||
dummy_cache: /data/dummy_prods | ||
data_server_url: https://www.astro.unige.ch/cdci/astrooda/dispatch-data/gw/integralhk/api/v1.0/genlc/ACS/{t0_isot}/{dt_s} |
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 is the only url which might be configurable, as it looks like for now.
To control this, maybe it is useful to be able to apply some "patches" to the configs, while most of the configs would come from configmaps, setting values as needed for consistency of the chart.
It should be possible.
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.
What do you mean by "patches"?
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.
config could be made of several dictionaries, with some precedence order. Maybe with https://helm.sh/docs/chart_template_guide/function_list/#merge-mustmerge ? I did not try it. I would just make sense.
values.yaml
Outdated
data_server_port: 8893 | ||
|
||
antares: | ||
enabled: true |
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 get that it is nice to be able to enable/disable plugins, but in this case it only enables/disables plugin configs, and what is installed in the dispatcher container still needs to be agreed with these values. Which is why I think both dispatcher container and full plugin config are internal business of the entire oda application and can not be easily configurable, unless a more coherent way to enable/disable also plugin installation and backend deployment.
values.yaml
Outdated
instruments: | ||
magic: | ||
data_server_url: http://oda-magic:8000 | ||
dummy_cache: /data/dummy_prods |
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.
what is point of making dummy_products configurable if they have to necessarily agree with what is built in the container? Generally in the deployment one does not provide the dummy products, so there is no point in pointing out their location.
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.
Frankly speaking, I am ignorant about this dummy_cache thing (and did not use it in antares plugin)
It is required in configuration, as you stated earlier. It is also present in the dispather config itself. But why, if generally no dummy products are provided?
And should this fields then concide in all configs?
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.
We placed them in containers when suitable. In generally, it might be useful to still have it, to enable testing when backend is not available. It might be redesigned though, it's kind of rigid.
OK. In general, you convince me that configs should be left in configmap. I will restore this part for now. Anyway, this PR started with another purpose in mind. I was just trying to install dispather + antares and nothing else. (with custom dispather container image, of course) And for the ability to not expose some instruments if manually stated or if backend is down, it would be good feature. I think we need to create issue in dispatcher-app for the time after release |
It will work, sure. For this particular config and field.
Indeed! It is useful to do that. We can name it like mount_ddcache or something like that. And make a comment that "required for dda plugin"
Absolutely! It can be done in several places at once for consistency. I add tentatively here: oda-hub/oda-chart#11 |
data_server_conf files were hardcoded in configmap.yaml, which is not good for configuration files.
Added configs for Antares and spi_acs
Besides previous tweaks of pvc.yaml it was still not possible to deploy dispatcher without dda.
Fixed this, but please inspect carefully, @volodymyrss