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

Adding note about verification of the existance of kube-cache-volume PVC #166

Closed
wants to merge 2 commits into from

Conversation

tovar-rodrigo
Copy link

No description provided.

@tovar-rodrigo tovar-rodrigo self-assigned this Aug 3, 2023
Copy link
Collaborator

@iteaguy iteaguy left a comment

Choose a reason for hiding this comment

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

Let's add the sample manifest here. Just add snippet of what you removed from go-clouddriver manifest.

@billiford
Copy link
Collaborator

The YAML in this repo is already setting the kube-cache-volume to be an emptyDir so it shouldn't be referencing some PVC: https://github.com/homedepot/go-clouddriver/blob/master/deployments/spin-go-clouddriver.yaml#L102

Not sure if this PR is necessary.

Snippet to show the deployment YML manifest modifications to avoid the usage of "kube-cache-volume" PVC.
@tovar-rodrigo
Copy link
Author

Let's add the sample manifest here. Just add snippet of what you removed from go-clouddriver manifest.
Snippet added on commit ee8ab58

@iteaguy
Copy link
Collaborator

iteaguy commented Aug 3, 2023

Valid point @billiford - As background to why we still want to go ahead and do this - We got audited and were told to get rid of some PVs/PVCs from our project since they are not in use. kube-cache-volume was one of them. Since we are not using disk with go-cd by default, we are cleaning up what we can. We found references of the one we are deleting in this manifest and would like to remove to avoid confusion. It doesn't hurt to leave it but it doesn't hurt to remove it either. Hence, we are also adding DOCs for what to do if someone does intend to use disk. Hope that clarifies matters.

@tovar-rodrigo
Copy link
Author

These are the internal PR's where that reference is being removed.

@billiford
Copy link
Collaborator

I'm wondering what happened on your end. You shouldn't need to have this documented since your YAMLs aren't referencing a PVC. There shouldn't be any issue starting up Go CD with disk cache in use. Maybe try deploying the YAML with name spin-go-clouddriver2 and posting the error here and opening an issue if necessary, but this doesn't seem like something that needs to be resolved in this repo, but at the Kubernetes level.

@iteaguy
Copy link
Collaborator

iteaguy commented Aug 3, 2023

@billiford - I believe you have a good point. From what I can see now, the names are the same (kube-cache-volume) but the config in go-clouddriver is an emptyDir Volume (not a PV) and the PVC we deleted in this TFE PR has nothing to do with it.

So, for now, we will not take any action on this PR. Once we are done with the PV/PVC cleanup, we will come back here and close this PR.

@billiford
Copy link
Collaborator

Yeah that sounds correct. Go CD doesn't care if it's some pod-specific disk or PV. We were never able to get the PV working anyway in GKE because GKE didn't allow ReadWriteMany for nodes. Deleting those resources from your TF script is definitely the correct approach. If you're able to recreate the error you saw then we should address that, but I didn't want to add a bunch of info if it's unnecessary and would just confuse users.

@billiford
Copy link
Collaborator

@iteaguy can we close this PR?

@iteaguy iteaguy closed this Jan 16, 2024
@iteaguy iteaguy deleted the readme-update-kube-cache branch January 16, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants