-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Disable PVC creation for che-multiuser master #9236
Conversation
96baae0
to
d5526bd
Compare
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.
LGTM
deploy/openshift/deploy_che.sh
Outdated
@@ -292,6 +292,10 @@ CHE_CONFIG_FILE_PATH=${CHE_CONFIG_FILE_PATH:-${DEFAULT_CHE_CONFIG_FILE_PATH}} | |||
cat "${CHE_DEPLOYMENT_FILE_PATH}" | \ | |||
sed "s/ image:.*/ image: \"${CHE_IMAGE_SANITIZED}\"/" | \ | |||
sed "s/ imagePullPolicy:.*/ imagePullPolicy: \"${IMAGE_PULL_POLICY}\"/" | \ | |||
if [[ "${CHE_MULTIUSER}" != "true" ]]; then | |||
sed "s|#CHE_MASTER_PVC|- apiVersion: v1\n kind: PersistentVolumeClaim\n metadata:\n labels:\n app: che\n name: che-data-volume\n spec:\n accessModes:\n - ReadWriteOnce\n resources:\n requests:\n storage: 1Gi|" | \ |
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.
Is it possible to rewrite it in the following way?
CHE_MASTER_PVC="- apiVersion: v1\n
kind: PersistentVolumeClaim\n
metadata:\n
labels:\n
app: che\n
name: che-data-volume\n
spec:\n
accessModes:\n
- ReadWriteOnce\n
resources:\n
requests:\n
storage: 1Gi"
...
cat "${CHE_DEPLOYMENT_FILE_PATH}" | \
...
if [[ "${CHE_MULTIUSER}" != "true" ]]; then
sed "s|#CHE_MASTER_PVC|${CHE_MASTER_PVC}|" | \
sed "s| #CHE_MASTER_VOLUME_MOUNTS.*|${CHE_MASTER_VOLUME_MOUNTS}|" | \
sed "s| #CHE_MASTER_VOLUMES.*|${CHE_MASTER_VOLUMES}|"; \
else cat -; fi | \
inject_che_config "#CHE_MASTER_CONFIG" "${CHE_CONFIG_FILE_PATH}"
}
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.
VERY GOOD
dockerfiles/che/entrypoint.sh
Outdated
rm -rf ${CHE_DATA}/lib/* | ||
mkdir -p ${CHE_DATA}/lib | ||
cp -rf ${CHE_HOME}/lib/* "${CHE_DATA}"/lib | ||
rm -rf ${CHE_DATA}/lib |
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.
Please, create an issue to remove this line after several releases. And update comment since it is out of date now.
@guydaichs @perspectivus1 please, review changes in helm deployment files |
ci-test |
Looks ok. @akorneta Can you test this on minikube? |
ci-test build report: |
@guydaichs sure. |
What does this PR do?
Removes backup of che-home/lib folder.
Disables che-data PVC creation for che-muliuser deployed on Kubernetes/OpenShift.
The motivation for these changes is that we want to be able to run simultaneously two Che masters and more. To achieve that we need to avoid usage of shared PV between these instances, because in the cases when PVs does not support RWX mode we will get an issue with deploying of Che on different nodes.
What issues does this PR fix or reference?
#9040