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(setup): check if pvcs can be created automatically #3316

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aboguszewski-sumo
Copy link
Collaborator

@aboguszewski-sumo aboguszewski-sumo commented Oct 10, 2023

This PR adds to setup script a functionality to check if pvcs can be created automatically.

Checklist

  • Changelog updated or skip changelog label added
  • Documentation updated
  • Template tests added for new features
  • Integration tests added or modified for major features

Comment on lines 275 to 282

{{- if .Values.sumologic.setup.dashboards.enabled }}
can_create_pvc
{{- end }}
Copy link
Collaborator Author

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.

@aboguszewski-sumo aboguszewski-sumo force-pushed the ab-setup-check branch 2 times, most recently from 9dce4ba to 6187cab Compare October 10, 2023 11:53
pvc_test_create_statefulset
sleep 30
pvc_test_check
Copy link
Collaborator Author

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.

Copy link
Contributor

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?

Copy link
Collaborator Author

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.

@aboguszewski-sumo aboguszewski-sumo marked this pull request as ready for review October 13, 2023 10:35
@aboguszewski-sumo aboguszewski-sumo requested a review from a team as a code owner October 13, 2023 10:35
@aboguszewski-sumo
Copy link
Collaborator Author

I'll fix helm template tests once setup.sh will be stable after the review.

@@ -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
Copy link
Contributor

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?

Copy link
Collaborator Author

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?

Copy link
Contributor

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 😅

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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

Copy link
Contributor

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

Comment on lines +182 to +188
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
}
Copy link
Contributor

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

Copy link
Contributor

@swiatekm-sumo swiatekm-sumo left a 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?

@aboguszewski-sumo
Copy link
Collaborator Author

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.
In general, creating anything manually seemed fishy for me, because in some cases the pvcs seem to be working fine if created manually.

@swiatekm-sumo
Copy link
Contributor

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. In general, creating anything manually seemed fishy for me, because in some cases the pvcs seem to be working fine if created manually.

You've seen a situation where a plain PVC worked, but not one created by a StatefulSet?

@aboguszewski-sumo
Copy link
Collaborator Author

@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.

@aboguszewski-sumo
Copy link
Collaborator Author

aboguszewski-sumo commented Nov 20, 2023

I managed to reproduce the problem without using statefulsets: I will remake this then

Copy link

@github-actions github-actions bot left a 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']

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants