-
Notifications
You must be signed in to change notification settings - Fork 48
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
Helm chart: Add default minimal pod security #133
Conversation
Add default minimal pod security. Related to issue opea-project#129. Signed-off-by: Lianhao Lu <[email protected]>
Signed-off-by: Lianhao Lu <[email protected]>
@@ -10,24 +10,24 @@ IMAGE_REPO=${OPEA_IMAGE_REPO:-""} | |||
|
|||
function init_codegen() { | |||
# insert a prefix before opea/.*, the prefix is IMAGE_REPO | |||
find . -name '*values.yaml' -type f -exec sed -i "s#repository: opea/*#repository: ${IMAGE_REPO}opea/#g" {} \; | |||
find .. -name '*values.yaml' -type f -exec sed -i "s#repository: opea/*#repository: ${IMAGE_REPO}opea/#g" {} \; |
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 think current folder (.) is good enough. Why use the parent folder (..) instead?
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.
just to be consistent with chatqna. Because of the new helm chart dependency structure created by @yongfengdu , we need to change the dependency image too. Codegen happens to be correct with current folder(.) in this case because it lists all the dependency's default image in its parent values.yaml, which is a bit of redundant and may be changed in the future.
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
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
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.
That was a lot of detailed careful work! Thank you!
@@ -13,17 +13,17 @@ helm dependency update chatqna | |||
export HFTOKEN="insert-your-huggingface-token-here" | |||
export MODELDIR="/mnt" | |||
export MODELNAME="Intel/neural-chat-7b-v3-3" | |||
helm install chatqna chatqna --set global.HUGGINGFACEHUB_API_TOKEN=${HFTOKEN} --set global.volume=${MODELDIR} --set llm-uservice.tgi.LLM_MODEL_ID=${MODELNAME} | |||
helm install chatqna chatqna --set global.HUGGINGFACEHUB_API_TOKEN=${HFTOKEN} --set global.modelUseHostPath=${MODELDIR} --set llm-uservice.tgi.LLM_MODEL_ID=${MODELNAME} |
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.
Can someone specify a model dir that is absolute and on the host versus in the container root filesystem through the variable MODELDIR? Do we need to check its value? Like one does for cross site scripting attacks.
LANGCHAIN_TRACING_V2: false | ||
LANGCHAIN_API_KEY: "insert-your-langchain-key-here" | ||
# set modelUseHostPath to host directory if you want to use hostPath volume for model storage | ||
# comment out modeluseHostPath if you want to download the model from huggingface | ||
modelUseHostPath: /mnt |
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.
Could an end user accidentially or maliciously assing directory to a place where there is sensitive material like passwords?
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.
/mnt
is general mount point where other unrelated things can/will be mounted. Better to specify a subdirectory (e.g. /mnt/opea-models
).
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.
User should override this directory with the one contains cached models at deployment time.
The /mnt is used just to avoid any reporting errors like "directory not exist".
For a production ready deployment, PV/PVC should be used.
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.
If user is expected to override it, getting an error from the default is a good thing!
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 a production ready deployment, PV/PVC should be used.
Agree, but quick git grep PVC
for the OPEA projects did not find any mentions of that.
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.
PV/PVC will be tracked by another issue #128
@@ -13,7 +13,7 @@ | |||
{{- end }} | |||
|
|||
2. Use this command to verify teirerank service: | |||
curl ${teirerank_svc_ip}:6006/embed \ | |||
curl ${teirerank_svc_ip}/embed \ |
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.
elsewhere you have http protocol
Description
Add default minimal pod security to meet K8S restricted pod security standard.
Rename the helm-chart option
global.volume
toglobal.modelUseHostPath
. Set it to null or empty value will result the emptyDir type of volumes to be mounted into the container and apply the the pod security as much as possible to meet restricted pod security standard.Issues
issue #129
Type of change
List the type of change like below. Please delete options that are not relevant.
Dependencies
n/a
Tests
n/a