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

support sidecar containers and provide cloudsqlproxy example #107

Merged

Conversation

mnichols
Copy link
Contributor

@mnichols mnichols commented Nov 7, 2020

This PR extends the helm templates to support sidecar containers. An example for adding a Google cloud sql proxy sidecar is provided.

@underrun
Copy link
Contributor

underrun commented Nov 7, 2020

Thanks for the PR!

My initial thoughts are that it would be really nice if users could apply values files without editing. Maybe just an example in the docs would be useful?

@mnichols
Copy link
Contributor Author

mnichols commented Nov 8, 2020

Hi @underrun! Helm isn't really my wheelhouse and I wasn't sure of a way to apply a sidecar to the existing templates without the approach taken here.
How does one extend the deployment without doing the -f <valuesfile> approach used so far in this repo?
Or were you just saying to not include the cloudsqlproxy values file and make it purely a form of documentation?

Personally I don't mind the approach in the values.postgres.yaml and others but could see how they should be maintained outside. Only question I'd have is how best to keep it maintained in sync with the helm templates they are extending.

@underrun
Copy link
Contributor

underrun commented Nov 8, 2020

I meant the documentation angle... But let me think about this a bit and chat with some folks next week.

Thanks again for working on this!

@mnichols
Copy link
Contributor Author

mnichols commented Nov 8, 2020

Sure thing..lemme know what y'all need. I was just pleased to be able to simply support cloudsqlproxy for our prod stuff in GCP. Your work here made it a snap to extend...nice job!

@jpearll
Copy link

jpearll commented Dec 15, 2020

Hi,
I am new with Helm. I am trying to deploy temporal on GCP (GKE).

I have tried to use the PR for getting Cloud Proxy support, with the following command:
helm install -f ./values/values.cloudsqlproxy.yaml workflow . --timeout 15m

But I have systematically a status 'Does not have minimum availability' on GCP for the temporal-frontend, temporal-worker, temporal-history, temporal-matching and temporal-worker workloads. Even if I tried to add nodes (6 nodes, auto-scaling).

I am not sure about how to tackle this. Also, what is the recommended GKE configuration for the cluster/node pool/machine types?

Thanks !

@mnichols
Copy link
Contributor Author

mnichols commented Jan 7, 2021

Hi,
I am new with Helm. I am trying to deploy temporal on GCP (GKE).

I have tried to use the PR for getting Cloud Proxy support, with the following command:
helm install -f ./values/values.cloudsqlproxy.yaml workflow . --timeout 15m

But I have systematically a status 'Does not have minimum availability' on GCP for the temporal-frontend, temporal-worker, temporal-history, temporal-matching and temporal-worker workloads. Even if I tried to add nodes (6 nodes, auto-scaling).

I am not sure about how to tackle this. Also, what is the recommended GKE configuration for the cluster/node pool/machine types?

Thanks !

@jpearll I cant comment on the recommended GKE config you need, but it sounds like if your pods arent starting up with this cloudsqlproxy config in place your attempt to connect to your db might be hanging. This is a sidecar and I've found they tend to fail badly (eg silently). You might want to check a direct db connection to your cloudsql instance first before trying with this proxy in place?

Copy link
Contributor

@underrun underrun left a comment

Choose a reason for hiding this comment

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

My apologies for the much-longer-than-a-week it took me to come back to this. We'd like to try and merge this in with a little cleanup. Thanks again for putting this together!

values/values.cloudsqlproxy.yaml Outdated Show resolved Hide resolved
@underrun underrun self-assigned this Feb 2, 2021
@jasonzoladz
Copy link

@underrun, what's the ETA for merging this? We need this as well. :)

@underrun
Copy link
Contributor

@mnichols would you like to make the requested change? If not I can open a parallel PR to get this merged and close this one? Either way is totally fine. Thanks again for your work on this!

@CLAassistant
Copy link

CLAassistant commented Mar 1, 2021

CLA assistant check
All committers have signed the CLA.

@mnichols mnichols force-pushed the mn-sidecar-cloudsqlproxy-support branch from 40b0232 to 21ca6aa Compare March 29, 2021 19:25
@mnichols
Copy link
Contributor Author

@underrun I made the changes I think you were after and squashed it after rebasing on latest master

@JDWardle
Copy link

JDWardle commented Aug 3, 2021

@underrun Is there anything blocking this from being merged in? This would be very helpful to have not just for cloudsql but support for adding in any other sidecars as well.

@hazcod
Copy link

hazcod commented Sep 14, 2021

@mnichols @underrun any news here?

@mnichols
Copy link
Contributor Author

🤷
@underrun ?

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.

7 participants