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

[werft] Add sweeper cleanup logic for k3s ws cluster #4746

Merged
merged 5 commits into from
Jul 13, 2021

Conversation

princerachit
Copy link
Contributor

@princerachit princerachit commented Jul 8, 2021

What?

When running wipe job do the following:

  • determine is k3s-external.yaml exists, if it does then delete this preview env with the same logic as that of dev. And also delete the external IP that was created by werft for the ws-proxy in this cluster. Otherwise go to next step
  • Delete preview env in the dev cluster

Testing

To test this out I manually edited the sweeper deploy and added arg--timeout=5m to trigger wipe job instantly. The cleanup succeeded

Reference job in a different branch

  • /werft k3s-ws
  • /werft with-clean-slate-deployment

@princerachit princerachit force-pushed the prs/sweeper-n-meta-ws-disable branch from cba6362 to 60412e9 Compare July 8, 2021 10:32
@princerachit princerachit changed the title [WIP][werft] Add sweeper cleanup logic [werft] Add sweeper cleanup logic for k3s ws cluster Jul 8, 2021
@princerachit princerachit requested review from geropl and meysholdt July 8, 2021 10:40
@princerachit princerachit marked this pull request as ready for review July 8, 2021 10:41
@princerachit princerachit requested a review from a team as a code owner July 8, 2021 10:41


async function wipeDevstaging() {
async function wipeDevstaging(pathToKubeConfig: string) {
Copy link
Member

Choose a reason for hiding this comment

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

I won't find time for a full review today, but: please let's try to find another way to switch between kubectl contexts. This really decreases the signal to noise ratio a lot.

I have not coherent view on this or what might be better, but will try to come up with sth tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One way I can think of is to merge all the kubeconfigs we have into the default one and then use context switch. That would involve downloading a kubeconfig -> updating the context name in that and then adding it to the default kubeconfig.

Since pathToKubeConfig is how I have done for all other places I would let this PR merge and raise a separate PR to use a new approach for all the files.

Copy link
Member

Choose a reason for hiding this comment

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

Since pathToKubeConfig is how I have done for all other places I would let this PR merge and raise a separate PR to use a new approach for all the files.

Ok, don't want to block this PR as it fixes sweeper. But let's not forget this.

One way I can think of is to merge all the kubeconfigs we have into the default one and then use context switch.

This was the way we used to do it in the past albeit it was simpler back then because clusters were static. But we can also leave the separate kubectl configs, but pass a more generic env around which exec supports (we already have that in some places).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the way we used to do it in the past albeit it was simpler back then because clusters were static. But we can also leave the separate kubectl configs, but pass a more generic env around which exec supports (we already have that in some places).

This seems like a much better solution! I will try this out and raise a PR

@princerachit
Copy link
Contributor Author

princerachit commented Jul 9, 2021

/werft run

👍 started the job as gitpod-build-prs-sweeper-n-meta-ws-disable.3

async function deleteExternalIp(k3sWsProxyIP: string, namespace: string) {
Copy link
Member

Choose a reason for hiding this comment

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

Note: In general it would be good to find a common place for code like this, like a k3s-cluster.ts or deployment.ts or so. wipe-staging.ts should be just calling that stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense. Let me move this to another file.

@princerachit
Copy link
Contributor Author

princerachit commented Jul 9, 2021

/werft run

👍 started the job as gitpod-build-prs-sweeper-n-meta-ws-disable.7

@princerachit
Copy link
Contributor Author

princerachit commented Jul 9, 2021

/werft run

👍 started the job as gitpod-build-prs-sweeper-n-meta-ws-disable.8

@princerachit princerachit force-pushed the prs/sweeper-n-meta-ws-disable branch from 58d69da to 73c5811 Compare July 9, 2021 10:50
@princerachit
Copy link
Contributor Author

princerachit commented Jul 9, 2021

/werft run

👍 started the job as gitpod-build-prs-sweeper-n-meta-ws-disable.10

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

LGTM!

@princerachit princerachit merged commit 6f0f9b2 into main Jul 13, 2021
@princerachit princerachit deleted the prs/sweeper-n-meta-ws-disable branch July 13, 2021 09:09
MatthewFagan pushed a commit to trilogy-group/gitpod that referenced this pull request Dec 6, 2021
* Update sweeper logic to delete k3s ws preview env and refactor methods and files
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.

2 participants