Fixing the clean-slate-deployment of new VM base preview-environments

[fullmetalrooster]

Description

This fixes a problem where VMs are not removed during a clean-slate-deployment.

UPDATE: Thanks for the support. I dug deeper and refactored a bit. The prepareVM function is now only used when we really want a VM. Then it checks in prepareVM if the the VM is present. If it is not present createVM otherwise the VM exists and we need to check if a clean-slate-deployment is triggered. Then the VM which existence has been verified in the previous step will be deleted and recreated.

Related Issue(s)

Fixes #8747

How to test

Start a workspace and run werft run github -a with-vm=true. After the deployment of the VM run werft run github -a with-vm=true -a with-clean-slate-deployment. The VM will be removed during the prepare phase and watch out for the string "Cleaning previously created VM" in the logs. An additional test would be to run werft run github -a with-vm=true and make sure that the VM will not be removed.

Release Notes

None

Documentation

[codecov]

Codecov Report

Merging #8748 (8acb739) into main (5e3897d) will decrease coverage by 1.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #8748      +/-   ##
==========================================
- Coverage   12.31%   11.17%   -1.14%     
==========================================
  Files          20       18       -2     
  Lines        1161      993     -168     
==========================================
- Hits          143      111      -32     
+ Misses       1014      880     -134     
+ Partials        4        2       -2     

Flag	Coverage Δ
components-gitpod-cli-app	11.17% <ø> (ø)
components-local-app-app-darwin-amd64	?
components-local-app-app-darwin-arm64	?
components-local-app-app-linux-amd64	?
components-local-app-app-linux-arm64	?
components-local-app-app-windows-386	?
components-local-app-app-windows-amd64	?
components-local-app-app-windows-arm64	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
components/local-app/pkg/auth/pkce.go
components/local-app/pkg/auth/auth.go

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[mads-hartmann]

The changes don't look right to me. You're passing in the namespace and then also appending preview in the -n argument to kubectl.

The code looks right before though, so I suspect whatever issue you're looking for is hiding somewhere else ☺️

[mads-hartmann]

@wulfthimm I suspect the real problem is that we only invoke prepareVM if the VM doesn't exist. And the logic to delete the VM is inside prepareVM ☺️

[fullmetalrooster]

The changes don't look right to me. You're passing in the namespace and then also appending preview in the -n argument to kubectl.

The code looks right before though, so I suspect whatever issue you're looking for is hiding somewhere else relaxed

Thanks, that is what I also suspect. I started with this approach and the job did what I expected to do. Therefore this premature PR to unblock the beta release and take a deeper look afterwards. But I will take a deeper look to ensure that the solution is sustainable now.

[fullmetalrooster]

@wulfthimm I suspect the real problem is that we only invoke prepareVM if the VM doesn't exist. And the logic to delete the VM is inside prepareVM relaxed

Having the deletion of the previous VM inside prepareVM is valid as we check for a cleanSlateDeployment. The problem is the function existsVM that return a false when it should return a true.

[mads-hartmann]

The problem is the function existsVM that return a false when it should return a true.

Maybe we're talking past each other. Your changes has introduced a bug which means VM.vmExists will always return false, but was that also the case on main?

If we're looking at main there's a bug even if VM.vmExists does the right thing. If VM.vmExists return true we will never invoke prepareVM because of this conditional. Because we never invoke prepareVM, we will never try to delete the VM.

I think the best way forward is to

1. Revert your changes
2. Ensure VM.vmExists does the right thing
3. Fix the issue I described above

[ArthurSens]

I believe you want to revert this :)

[fullmetalrooster]

Thanks for the support. I dug deeper and refactored a bit. The prepareVM function is now only used when we really want a VM. Then it checks in prepareVM if the the VM is present. If it is not present createVM otherwise the VM exists and we need to check if a clean-slate-deployment is triggered. Then the VM which existence has been verified in the previous step will be deleted and recreated.

[ArthurSens]

Thanks for tackling this bug introduced by me wulf! 🤗

I feel like your approach is overcomplicating things a bit though... Could we stick with the original implementation, while adding your check following the Single Responsibility Principle?

Does this approach looks better to read?

function decideHarvesterVMCreation(werft: Werft, config: JobConfig) {
    if (shouldCreateVM(config)) {
        prepareVM(werft, config)
    } else {
        werft.currentPhaseSpan.setAttribute("werft.harvester.created_vm", false)
    }
    werft.done(prepareSlices.BOOT_VM)
}

function shouldCreateVM(config: JobConfig) {
    return config.withVM && (
          !VM.vmExists({ name: config.previewEnvironment.destname }) || 
          config.cleanSlateDeployment
    )
}

[fullmetalrooster]

@ArthurSens I find my approach much easier to read because it does not use as much functions and is still relatively compact. But I am not a TS developer and do not have any preferences.

[ArthurSens]

Using conditionals is nice because it's easy to implement things when in a hurry, but they add complexity as we need to keep track of how all those conditionals correlate between different parts of the code. When reading the code, with this approach, we need to scroll up and down, we need to look at different levels of nested conditionals and it does add cognitive noise 😵

Functions, on the other hand, are a powerful thing that can reduce complexity 🙂. We can read code only once in a top-to-bottom approach, while not entering into nested conditionals.

[ArthurSens]

@wulfthimm and I just did a pair-programming session to align coding style 🙂

[mads-hartmann]

Looks good to me, thanks!

[ArthurSens]

/hold

Looks like the job failed due to core-dev flakyness?

/werft run

👍 started the job as gitpod-build-wth-fix-clean-vm.44

[ArthurSens]

/werft run

👍 started the job as gitpod-build-wth-fix-clean-vm.45

[ArthurSens]

/unhold