Skip to content
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.

Sign up for GitHub

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

Jump to bottom

plugins configurable with values + decouple from dda #11

Merged
merged 3 commits into from
May 5, 2021
Merged

plugins configurable with values + decouple from dda #11

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
086c723
data_server_conf moved to values
dsavchenko May 4, 2021
394ea4e
turn off dda-related volumes if no dda
dsavchenko May 4, 2021
4c808dd
configs back to configmap
dsavchenko May 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 6 additions & 28 deletions templates/configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,32 +4,10 @@ metadata:
name: dispatcher-config
namespace: {{ .Values.environment }}
data:
osa_data_server_conf.yml: |
{{- range $plugin, $conf := .Values.plugins -}}
{{- if $conf.enabled }}
{{$plugin}}_data_server_conf.yml: |
instruments:
isgri:
dispatcher_mnt_point: /data
data_server_cache: reduced/ddcache
dummy_cache: /data/dummy_prods
data_server_url: http://oda-dda-interface:8000

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


magic_data_server_conf.yml: |
instruments:
magic:
data_server_url: http://oda-magic:8000


polar_data_server_conf.yml: |
instruments:
polar:
dispatcher_mnt_point:
data_server_cache:
dummy_cache: dummy_prods
data_server_url: polar-worker
data_server_port: 8893
{{- toYaml $conf.instruments | nindent 6 }}
{{- end }}
{{- end -}}
37 changes: 19 additions & 18 deletions templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,18 +53,20 @@ spec:
hostPath:
path: {{ .Values.oda.ddcache }}
{{ else }}
- name: scratch
persistentVolumeClaim:
claimName: dda-interface-scratch
- name: dispatcher-scratch-root
persistentVolumeClaim:
claimName: dispatcher-scratch-root
- name: filelogdir
persistentVolumeClaim:
claimName: dda-filelogdir
claimName: dispatcher-filelogdir
{{- if .Values.plugins.osa.enabled }}
- name: scratch
persistentVolumeClaim:
claimName: dda-interface-scratch
- name: data-reduced-ddcache
persistentVolumeClaim:
claimName: data-reduced-ddcache
{{- end }}
#- name: isdc-arc-rev3
#- name: isdc-pvphase-nrt-ops
{{ end }}
Expand Down Expand Up @@ -122,31 +124,30 @@ spec:
value: "no"
{{- end }}
volumeMounts:
- mountPath: /scratch
name: scratch
- mountPath: /var/log/containers
name: filelogdir
- mountPath: /data/reduced/ddcache
name: data-reduced-ddcache
- mountPath: /data/dispatcher_scratch
name: dispatcher-scratch-root
{{- if .Values.plugins.osa.enabled }}
Copy link
Member

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.

- mountPath: /data/reduced/ddcache
name: data-reduced-ddcache
- mountPath: /scratch
name: scratch
{{- end }}

- mountPath: /dispatcher/conf/conf_env.yml
name: dispatcher-conf-secret
subPath: conf_env.yml
readOnly: true

{{- range $plugin, $conf := .Values.plugins -}}
{{- if $conf.enabled }}
- name: dispatcher-config-volume
mountPath: /dispatcher/conf/conf.d/osa_data_server_conf.yml
subPath: osa_data_server_conf.yml
readOnly: true
- name: dispatcher-config-volume
mountPath: /dispatcher/conf/conf.d/polar_data_server_conf.yml
subPath: polar_data_server_conf.yml
readOnly: true
- name: dispatcher-config-volume
mountPath: /dispatcher/conf/conf.d/magic_data_server_conf.yml
subPath: magic_data_server_conf.yml
mountPath: /dispatcher/conf/conf.d/{{$plugin}}_data_server_conf.yml
subPath: {{$plugin}}_data_server_conf.yml
readOnly: true
{{- end }}
{{- end }}

- name: celery
securityContext:
Expand Down
49 changes: 49 additions & 0 deletions values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,3 +104,52 @@ oda:
use_hostpath: true
ddaQueue: queue-osa11
ddcache: "/net/cdcicn02/scratch/shared/savchenk/data/reduced/ddcache"

plugins:
osa:
ebabled: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ebabled?

instruments:
isgri:
dispatcher_mnt_point: /data
data_server_cache: reduced/ddcache
dummy_cache: /data/dummy_prods
data_server_url: http://oda-dda-interface:8000
Copy link
Member

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.


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

magic:
enabled: true
instruments:
magic:
data_server_url: http://oda-magic:8000
dummy_cache: /data/dummy_prods
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.


polar:
enabled: true
instruments:
polar:
dispatcher_mnt_point:
data_server_cache:
dummy_cache: /data/dummy_prods
data_server_url: polar-worker
data_server_port: 8893

antares:
enabled: true
Copy link
Member

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.

instruments:
antares:
data_server_url: http://oda-antares:8000
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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.

dummy_cache: /data/dummy_prods

spiacs:
enabled: true
instruments:
spi_acs:
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}
Copy link
Member

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.

Copy link
Member Author

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"?

Copy link
Member

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.