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

1427 - Add DNS and cert cleanup into the preview env cron #9525

Merged
merged 7 commits into from
Apr 27, 2022

Conversation

liam-j-bennett
Copy link
Contributor

@liam-j-bennett liam-j-bennett commented Apr 25, 2022

Description

Added two functions to cleanup DNS records and certificates

Related Issue(s)

Fixes https://github.com/gitpod-io/ops/issues/1427

Release note

NONE

How to test

Create a new branch and then delete after all resources are created
Check the DNS and cert resources are correctly removed

Copy link
Contributor

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Would it make sense to implement DNS deletion using the same nodejs library we use to create them? 🙂

https://github.com/gitpod-io/gitpod/blob/main/.werft/util/gcloud.ts

@liam-j-bennett liam-j-bennett force-pushed the lb_1427_add_dns_cert_cleanup branch from 6dfb471 to e6cae92 Compare April 25, 2022 11:13
@roboquat roboquat added size/M and removed size/S labels Apr 25, 2022
@liam-j-bennett
Copy link
Contributor Author

liam-j-bennett commented Apr 25, 2022

Would it make sense to implement DNS deletion using the same nodejs library we use to create them? 🙂

https://github.com/gitpod-io/gitpod/blob/main/.werft/util/gcloud.ts

Changed to use a util function

@ArthurSens
Copy link
Contributor

ArthurSens commented Apr 26, 2022

Changed to use a util function

Nice, thanks Liam! 🙂

Another question: when implementing the creation, I had to add some checks to guarantee we don't try to recreate a DNS record that already exists. Do you know what would happen if we try to delete a DNS that does not exist?

@liam-j-bennett
Copy link
Contributor Author

liam-j-bennett commented Apr 26, 2022

Changed to use a util function

Nice, thanks Liam! 🙂

Another question: when implementing the creation, I had to add some checks to guarantee we don't try to recreate a DNS record that already exists. Do you know what would happen if we try to delete a DNS that does not exist?

It returns a 404 (I hate delete APIs that do this). I'll add an ignore to the callback, as I don't think we would ever care if the delete returns this, as the record is gone either way.

@liam-j-bennett liam-j-bennett force-pushed the lb_1427_add_dns_cert_cleanup branch 2 times, most recently from dd732a5 to 8034c40 Compare April 26, 2022 14:05
@liam-j-bennett
Copy link
Contributor Author

liam-j-bennett commented Apr 26, 2022

So earlier today, I tested this and it worked, with several branches completedly cleaned up. However, after some extra changes, I have not been able to test again, along with werft being unstable and therefore hard to test.

Since rebasing on main, it seems the branches are not being picked up:

  1. New branch and preview env created https://werft.gitpod-dev.com/job/gitpod-build-lb-test-dns-cert-cleanup3.0
  2. Branch deleted
  3. Run job to clean up https://werft.gitpod-dev.com/job/gitpod-custom-lb-1427-add-dns-cert-cleanup.7/logs

The branch is in the preview envs log, but does not get cleaned up, even though the branch doesn't exist.

@mads-hartmann mads-hartmann force-pushed the lb_1427_add_dns_cert_cleanup branch from 8034c40 to b1a1f3a Compare April 27, 2022 09:05
@roboquat roboquat added size/L and removed size/M labels Apr 27, 2022
@mads-hartmann mads-hartmann force-pushed the lb_1427_add_dns_cert_cleanup branch from 450ea42 to 2d2d129 Compare April 27, 2022 12:22
@mads-hartmann mads-hartmann force-pushed the lb_1427_add_dns_cert_cleanup branch from 2d2d129 to 38a0503 Compare April 27, 2022 12:31
@mads-hartmann
Copy link
Contributor

@ArthurSens this is ready for another review. This has since been rebased on main which included the changes from #9225 which it appears unfortunately had some issues when checking for DB activity in the preview environments where the Werft job would complete prematurely without trying to clean up preview environments - to debug this I had to improve the Werft logging as well as simply the logic for deciding if a preview environment is stale due to DB inactivity.

I apologise for the messy diff. I was able to verify it now deletes certificates and dns records in this job

Copy link
Contributor

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Looks pretty nice!

The only thing that worried me a bit was the number of slices 😅. If I understand honeycomb correctly, we pay for every new span, which is created for every new slice. So this amount of new slices sounds like a bit overkill. I'm approving anyways because when comparing to traces coming from production, I believe this is almost nothing 😛

Another thing I've noticed is that some of those slices' span is not being closed correctly 🤔
image

Also not a blocker for this PR, but I think it could be worth adding an issue for technical debt?

@mads-hartmann
Copy link
Contributor

mads-hartmann commented Apr 27, 2022

@ArthurSens Good catch with the span not being closed 🙏 I just pushed a commit that I believe will fix it. Given it's such a small change I don't think I have to manually test again - I'm happy to merge so it can run over night and I can check the output tomorrow.

@mads-hartmann
Copy link
Contributor

mads-hartmann commented Apr 27, 2022

@ArthurSens Just realised I didn't need to request re-review based on the new rules. Sorry about that ☺️

@roboquat roboquat merged commit 978805e into main Apr 27, 2022
@roboquat roboquat deleted the lb_1427_add_dns_cert_cleanup branch April 27, 2022 15:11
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.

4 participants