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

Add a new configmap for variables from the Lagoon API only #2348

Closed

Conversation

shreddedbacon
Copy link
Member

@shreddedbacon shreddedbacon commented Nov 30, 2020

Checklist

  • Affected Issues have been mentioned in the Closing issues section
  • Documentation has been written/updated
  • PR title is ready for changelog and subsystem label(s) applied

This adds a new configmap called lagoon-api-vars that will store any variables from the Lagoon API only.

This will allow for variables to be added and removed from the Lagoon API and actually be removed from the configmap, and since it is added after lagoon-env anything inside of it will override anything else once loaded in a running pod/container.

Any variables that are added to the normal lagoon-env will persist like normal.

Closing issues

closes uselagoon/build-deploy-tool#136

…ment variables when remove from the lagoon-api
@shreddedbacon shreddedbacon added the 2-build-deploy Build & Deploy subsystem label Nov 30, 2020
@shreddedbacon
Copy link
Member Author

This will still leave API variables that are currently defined in the lagoon-env though, so will really only benefit new environments/deployments rather than existing

@smlx
Copy link
Member

smlx commented Nov 30, 2020

This approach seems good, I just wanted to note that I think the envFrom precedence is "last one wins" rather than alphabetical, so the order of the configmaps in the template is important and it might be worth adding a comment explaining that.

@shreddedbacon
Copy link
Member Author

This approach seems good, I just wanted to note that I think the envFrom precedence is "last one wins" rather than alphabetical, so the order of the configmaps in the template is important and it might be worth adding a comment explaining that.

Updated my comment. I wasn't sure if it was alphabetical or order. Good to know.

@Schnitzel
Copy link
Contributor

I like the overall idea but I think we have to improve the way we do this, I see one risk here:
assuming the deployment stops for any reason in between when it empties the lagoon-api-vars config-map and then fill it again with the correct variables, we then have a half filled lagoon-api-vars which also applies to running pods, so if the running pods are restarted (assuming this happens because of a node restart) then the pods are loaded with a not complete env variable and will break the running application because of a failed lagoon deployment, something we want to prevent in any way.

So a first idea I had was to create a new configmap every time (add a hash suffix), which basically means we don't touch the old configmap and create a new one, if the creation of the new one fails for any reason, it will not cause any issues with the running pods.
The only "issue" is see with this is that the new configmap will basically mean we restart all pods every single time, which is not the case right now (for example a redis pod does not restart unless there is a new redis image). So maybe we should do something like:
Create a new ConfigMap with the new api env variables, after the configmap has been created we compare the md5 hash of it with the already existing configmap. If it's exactly the same, we still use the old configmap to inject into the pods, if it changes we use the new one and delete the old configmap at the end of the lagoon deployment.

I know this is a bit more complex than the current solution but I'm really worried we could end up in a situation where running pods are suddenly broken.

…hat shouldnt exist in the configmap instead
@shreddedbacon
Copy link
Member Author

I came up with a different way. Using op remove patches to remove keys from the configmap once we've patched it.

This way the configmap doesn't get nuked entirely unless there are no environment variables in the API.

@tobybellwood
Copy link
Member

I like this approach - it's currently difficult knowing which variables are being brought in from where, and having a configmap for them would aid in debugging the current state (along with #2375)

@tobybellwood
Copy link
Member

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.

Deleted variables from API remain in configmap
5 participants