-
Notifications
You must be signed in to change notification settings - Fork 183
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(setup): check if pvcs can be created automatically #3316
Conversation
|
||
{{- if .Values.sumologic.setup.dashboards.enabled }} | ||
can_create_pvc | ||
{{- end }} |
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.
Maybe because of waiting, this should be run as a separate process in the background.
9dce4ba
to
6187cab
Compare
pvc_test_create_statefulset | ||
sleep 30 | ||
pvc_test_check |
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.
Not sure what constant should be used here.
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.
do we need to wait 30 seconds? How do we know pvc cannot be created automatically?
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.
Afaik the only way to do it is to try to create a pvc and see if it gets created - and it doesn't happen automatically. Although it's not perfect, I think it's fine as long as we don't get false negatives (which we might, if creating a pvc takes longer than 30 seconds).
We can lower the impact of waiting by moving this to a separate process, run at the beginning of the script.
I'll fix helm template tests once |
@@ -2,6 +2,57 @@ | |||
|
|||
readonly DEBUG_MODE=${DEBUG_MODE:="false"} | |||
readonly DEBUG_MODE_ENABLED_FLAG="true" | |||
readonly TEST_PVC_FILE="test-pvc-ss.yaml" | |||
readonly TEST_PVC_STATEFULSET="pvc-nginx" | |||
readonly TEST_PVC_STATEFULSET_CONFIG="apiVersion: v1 |
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 move it to config map?
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.
Most env variables are in job.yaml
, wouldn't it be better to move it there?
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 was referring to TEST_PVC_STATEFULSET_CONFIG
😅
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'll have to check how to access it in the setup image.
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 can't find out which files are copied onto the image: https://github.com/SumoLogic/sumologic-kubernetes-setup/blob/main/Dockerfile
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 don't think we need it in image, we can simply use configmap to add it in runtime: https://github.com/SumoLogic/sumologic-kubernetes-collection/blob/main/deploy/helm/sumologic/templates/setup/configmap.yaml
In dockerfile, we only get files required for terraform init
to have dependencies already in image
function install_kubectl() { | ||
curl -LO "https://dl.k8s.io/release/$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/linux/amd64/kubectl" | ||
chmod +x kubectl | ||
mkdir -p ~/.local/bin | ||
mv ./kubectl ~/.local/bin/kubectl | ||
export PATH=$PATH:~/.local/bin | ||
} |
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.
It should be added to setup image
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 there a reason we don't try to create a PVC directly instead of the StatefulSet?
When reproducing, I wasn't sure if just creating a pvc will imply whether it can or not be creating automatically - with statefulset I had that guarantee, because it is directly the case we have in the Helm Chart. |
You've seen a situation where a plain PVC worked, but not one created by a StatefulSet? |
@swiatekm-sumo It worked when I created PV manually. I had some problems with creating it automatically on EKS (even after adding the plugin), so I went for a statefulset. |
I managed to reproduce the problem without using statefulsets: I will remake this then |
2ae311e
to
dbde761
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.
This pull request contains invalid labels. Please remove all of the following labels: ['do-not-merge/hold']
This PR adds to setup script a functionality to check if pvcs can be created automatically.
Checklist