plugins configurable with values + decouple from dda #11

dsavchenko · 2021-05-04T12:36:32Z

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

volodymyrss

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)

volodymyrss · 2021-05-04T12:42:17Z

values.yaml

+
+plugins:
+  osa:
+    ebabled: true


volodymyrss · 2021-05-04T12:44:15Z

templates/deployment.yaml

          - mountPath: /data/dispatcher_scratch
            name: dispatcher-scratch-root
+          {{- if .Values.plugins.osa.enabled }}


yes something like this this is needed without dda.

volodymyrss · 2021-05-04T13:06:50Z

values.yaml

+        dispatcher_mnt_point: /data
+        data_server_cache: reduced/ddcache
+        dummy_cache: /data/dummy_prods
+        data_server_url: http://oda-dda-interface:8000


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.

volodymyrss · 2021-05-04T13:07:51Z

values.yaml

+    enabled: true
+    instruments:
+      antares:
+        data_server_url: http://oda-antares:8000


for example is this really supposed to be generally configurable on deployment, when antares backend is deployed along with the dispatcher?

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.

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.

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.

volodymyrss · 2021-05-04T13:29:30Z

values.yaml

+        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}


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.

What do you mean by "patches"?

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.

volodymyrss · 2021-05-04T14:34:05Z

values.yaml

+        data_server_port: 8893
+
+  antares:
+    enabled: true


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.

volodymyrss · 2021-05-04T14:38:01Z

values.yaml

+    instruments:
+      magic:
+        data_server_url: http://oda-magic:8000
+        dummy_cache: /data/dummy_prods


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.

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?

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.

dsavchenko · 2021-05-04T15:51:58Z

OK. In general, you convince me that configs should be left in configmap. I will restore this part for now.
Overriding backend URL should be useful, though. Why simply define with default in the configmap.yaml will not work? Could you describe such a scenario?

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)
So at least some flag to switch off unnecessary volumes must be there. To not be misleading, it may be named otherwise.

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

volodymyrss · 2021-05-04T17:17:00Z

OK. In general, you convince me that configs should be left in configmap. I will restore this part for now.
Overriding backend URL should be useful, though. Why simply define with default in the configmap.yaml will not work? Could you describe such a scenario?

It will work, sure. For this particular config and field.
You mean what is the advantage of this merging config business? It's because it's more generic, and would allow custom configuration of some plugins. But maybe it's too generic and flexible. For now, it is not required really.

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)
So at least some flag to switch off unnecessary volumes must be there. To not be misleading, it may be named otherwise.

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"

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

Absolutely! It can be done in several places at once for consistency. I add tentatively here: oda-hub/oda-chart#11

dsavchenko added 2 commits May 4, 2021 12:34

data_server_conf moved to values

086c723

turn off dda-related volumes if no dda

394ea4e

volodymyrss requested changes May 4, 2021

View reviewed changes

volodymyrss assigned dsavchenko May 4, 2021

configs back to configmap

4c808dd

dsavchenko requested a review from volodymyrss May 4, 2021 19:39

volodymyrss approved these changes May 5, 2021

View reviewed changes

volodymyrss merged commit 11e78a4 into oda-hub:master May 5, 2021

dsavchenko deleted the add-antares branch May 5, 2021 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugins configurable with values + decouple from dda #11

plugins configurable with values + decouple from dda #11

dsavchenko commented May 4, 2021

volodymyrss left a comment

volodymyrss May 4, 2021

volodymyrss May 4, 2021

volodymyrss May 4, 2021

volodymyrss May 4, 2021

dsavchenko May 4, 2021

volodymyrss May 4, 2021

volodymyrss May 4, 2021

volodymyrss May 4, 2021

dsavchenko May 4, 2021

volodymyrss May 4, 2021

volodymyrss May 4, 2021

volodymyrss May 4, 2021

dsavchenko May 4, 2021

volodymyrss May 4, 2021

dsavchenko commented May 4, 2021

volodymyrss commented May 4, 2021

plugins configurable with values + decouple from dda #11

plugins configurable with values + decouple from dda #11

Conversation

dsavchenko commented May 4, 2021

volodymyrss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsavchenko commented May 4, 2021

volodymyrss commented May 4, 2021