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.

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

feat: add ElasticSearch support in tutor-discovery #89

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- [Feature] Add Elasticsearch support in tutor-discovery. As Tutor and Open edX have shifted to Meilisearch, and course-discovery still depends on Elasticsearch, running the Elasticsearch container with tutor-discovery will facilitate smoother operation for the course-discovery service. (by @Faraz32123)
- It's related PR from tutor: https://github.com/overhangio/tutor/pull/1141.
1 change: 1 addition & 0 deletions tutordiscovery/patches/discovery-common-settings
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DISCOVERY_RUN_ELASTICSEARCH = {{ DISCOVERY_RUN_ELASTICSEARCH }}
51 changes: 51 additions & 0 deletions tutordiscovery/patches/k8s-deployments
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,54 @@ spec:
- name: settings
configMap:
name: discovery-settings

{% if DISCOVERY_RUN_ELASTICSEARCH %}
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: elasticsearch
labels:
app.kubernetes.io/name: elasticsearch
spec:
selector:
matchLabels:
app.kubernetes.io/name: elasticsearch
strategy:
type: Recreate
template:
metadata:
labels:
app.kubernetes.io/name: elasticsearch
spec:
securityContext:
runAsUser: 1000
runAsGroup: 1000
fsGroup: 1000
fsGroupChangePolicy: "OnRootMismatch"
containers:
- name: elasticsearch
image: {{ DISCOVERY_DOCKER_IMAGE_ELASTICSEARCH }}
env:
- name: cluster.name
value: "openedx"
- name: bootstrap.memory_lock
value: "true"
- name: discovery.type
value: "single-node"
- name: ES_JAVA_OPTS
value: "-Xms{{ DISCOVERY_ELASTICSEARCH_HEAP_SIZE }} -Xmx{{ DISCOVERY_ELASTICSEARCH_HEAP_SIZE }}"
- name: TAKE_FILE_OWNERSHIP
value: "1"
ports:
- containerPort: 9200
securityContext:
allowPrivilegeEscalation: false
volumeMounts:
- mountPath: /usr/share/elasticsearch/data
name: data
volumes:
- name: data
persistentVolumeClaim:
claimName: elasticsearch
{% endif %}
1 change: 0 additions & 1 deletion tutordiscovery/patches/k8s-jobs
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,3 @@ spec:
- name: settings
configMap:
name: discovery-settings

16 changes: 16 additions & 0 deletions tutordiscovery/patches/k8s-services
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,19 @@ spec:
protocol: TCP
selector:
app.kubernetes.io/name: discovery
{% if DISCOVERY_RUN_ELASTICSEARCH %}
---
apiVersion: v1
kind: Service
metadata:
name: elasticsearch
labels:
app.kubernetes.io/name: elasticsearch
spec:
type: ClusterIP
ports:
- port: 9200
protocol: TCP
selector:
app.kubernetes.io/name: elasticsearch
{% endif %}
16 changes: 16 additions & 0 deletions tutordiscovery/patches/k8s-volumes
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{% if DISCOVERY_RUN_ELASTICSEARCH %}
---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
name: elasticsearch
labels:
app.kubernetes.io/component: volume
app.kubernetes.io/name: elasticsearch
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 2Gi
{% endif %}
9 changes: 9 additions & 0 deletions tutordiscovery/patches/local-docker-compose-dev-services
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,12 @@ discovery:
default:
aliases:
- "{{ DISCOVERY_HOST }}"

{% if DISCOVERY_RUN_ELASTICSEARCH and is_docker_rootless() %}
elasticsearch:
ulimits:
memlock:
# Fixes error setting rlimits for ready process in rootless docker
soft: 0 # zero means "unset" in the memlock context
hard: 0
{% endif %}
Comment on lines +19 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these settings are necessary for elasticsearch i think, when we are running docker in rootless mode to avoid memory issues for elasticsearch.

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{% if DISCOVERY_RUN_ELASTICSEARCH %}setowner 1000 /mounts/elasticsearch{% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am seeing stat: can't stat '/mounts/elasticsearch': No such file or directory error in permissions container. Can you verify this, please?

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{% if DISCOVERY_RUN_ELASTICSEARCH %}- ../../data/elasticsearch:/mounts/elasticsearch{% endif %}
23 changes: 22 additions & 1 deletion tutordiscovery/patches/local-docker-compose-services
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,28 @@ discovery:
volumes:
- ../plugins/discovery/apps/settings/tutor:/openedx/discovery/course_discovery/settings/tutor:ro
- ../../data/discovery/media:/openedx/discovery/course_discovery/media
{% if DISCOVERY_RUN_ELASTICSEARCH %}- ../../data/elasticsearch:/mounts/elasticsearch{% endif %}
depends_on:
- lms
{% if RUN_MYSQL %}- mysql{% endif %}
{% if RUN_ELASTICSEARCH %}- elasticsearch{% endif %}
{% if DISCOVERY_RUN_ELASTICSEARCH %}- elasticsearch{% endif %}

{% if DISCOVERY_RUN_ELASTICSEARCH -%}
elasticsearch:
image: {{ DISCOVERY_DOCKER_IMAGE_ELASTICSEARCH }}
environment:
- cluster.name=openedx
- bootstrap.memory_lock=true
- discovery.type=single-node
- "ES_JAVA_OPTS=-Xms{{ DISCOVERY_ELASTICSEARCH_HEAP_SIZE }} -Xmx{{ DISCOVERY_ELASTICSEARCH_HEAP_SIZE }}"
ulimits:
memlock:
soft: -1
hard: -1
restart: unless-stopped
user: "1000:1000"
volumes:
- ../../data/elasticsearch:/usr/share/elasticsearch/data
depends_on:
- permissions
{%- endif %}
6 changes: 6 additions & 0 deletions tutordiscovery/plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@
"EXTRA_PIP_REQUIREMENTS": [],
"REPOSITORY": "https://github.com/openedx/course-discovery.git",
"REPOSITORY_VERSION": "{{ OPENEDX_COMMON_VERSION }}",
"RUN_ELASTICSEARCH": True,
"DOCKER_IMAGE_ELASTICSEARCH": "docker.io/elasticsearch:7.17.13",
"ELASTICSEARCH_HOST": "elasticsearch",
"ELASTICSEARCH_PORT": 9200,
"ELASTICSEARCH_SCHEME": "http",
"ELASTICSEARCH_HEAP_SIZE": "1g",
},
"unique": {
"MYSQL_PASSWORD": "{{ 8|random_string }}",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,14 @@
}
}

DISCOVERY_DOCKER_IMAGE_ELASTICSEARCH = "{{ DISCOVERY_DOCKER_IMAGE_ELASTICSEARCH }}"
DISCOVERY_ELASTICSEARCH_HOST = "{{ DISCOVERY_ELASTICSEARCH_HOST }}"
DISCOVERY_ELASTICSEARCH_PORT = "{{ DISCOVERY_ELASTICSEARCH_PORT }}"
DISCOVERY_ELASTICSEARCH_SCHEME = "{{ DISCOVERY_ELASTICSEARCH_SCHEME }}"
DISCOVERY_ELASTICSEARCH_HEAP_SIZE = "{{ DISCOVERY_ELASTICSEARCH_HEAP_SIZE }}"
Comment on lines +23 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these needed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we are running elasticsearch container for/from discovery plugin now, these settings will be need in various patches to run elasticsearch same as it was running through tutor.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if the patches were expecting the config with DISCOVERY_ prefix, won't those patches need to an update as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, these settings are already being used in all patches in this PR with DISCOVERY_ prefix.


ELASTICSEARCH_DSL['default'].update({
'hosts': "{{ ELASTICSEARCH_SCHEME }}://{{ ELASTICSEARCH_HOST }}:{{ ELASTICSEARCH_PORT }}/"
'hosts': "{{ DISCOVERY_ELASTICSEARCH_SCHEME }}://{{ DISCOVERY_ELASTICSEARCH_HOST }}:{{ DISCOVERY_ELASTICSEARCH_PORT }}/"
})

{% for name, index in DISCOVERY_INDEX_OVERRIDES.items() %}
Expand Down