Convert to class to reduce argument passing #9490

mads-hartmann · 2022-04-22T11:57:55Z

Description

This promotes installMonitoringSatellite to be a class to reduce the need for passing the same arguments around to the functions.

Related Issue(s)

Fixes #9059

How to test

in core-dev with observability enabled werft run github -f -a with-helm=true -a with-observability=true - job - was able to port-forward to Prometheus and Grafana.
with-vm werft run github -f -a with-vm=true - job - was able to port-forward to Prometheus and Grafana but had to use the following commands instead of our own script. I suspect there's a bug in our proxy. Will open a follow up issue.
```
kubectl --context=k3s-preview-environment port-forward svc/prometheus-k8s "9090:9090"
kubectl --context=k3s-preview-environment port-forward svc/grafana "3000:3000"
```

Release Notes

NONE

Documentation

N/A

ArthurSens

Noice! I'm really liking this pattern for using those "installer" classes.

I have a few questions but I don't know if they make sense 😅, so I'm approving with a hold. Please decide if we still need to make changes here, leave them another PR or not do them at all

/hold

ArthurSens · 2022-04-22T14:02:30Z

.werft/observability/monitoring-satellite.ts

-async function deployGitpodServiceMonitors(kubeconfig: string) {
-    const werft = getGlobalWerftInstance()
+    private async deployGitpodServiceMonitors() {
+        const { werft, kubeconfigPath } = this.options;


back when I implemented this I had no idea what was this async and was just adding because it was being used everywhere.

Does it make sense in this class? Almost every function is using async, although we're not using Promises anywhere. Would it make sense to just remove it?

ArthurSens · 2022-04-22T14:03:48Z

.werft/jobs/build/deploy-to-preview-environment.ts

-    installMonitoringSatelliteParams.stackdriverServiceAccount = stackdriverServiceAccount
-    installMonitoringSatelliteParams.withVM = withVM
-    installMonitoringSatellite(installMonitoringSatelliteParams);
+async function installMonitoring(werft: Werft, kubeconfig: string, namespace: string, nodeExporterPort: number, domain: string, stackdriverServiceAccount: any, withVM: boolean, observabilityBranch: string) {


same thing here, we declare an async function only to use await when calling it. Would it make sense to just remove it?

I'm keeping this async as it's my understand that we want to block in core-dev (when observability is enabled) but not block when installing it in VM-based preview environments ☺️

mads-hartmann · 2022-04-26T13:18:26Z

@ArthurSens Thanks for the pointer! I removed the async keyword from the methods and also the async option for the exec invocations - in effect making observability installation blocking. Previously we were blocking using await in core-dev but not in Harvester. As we generally want observability ready before we install Gitpod to avoid errors about Gitpod not being able to send telemetry I think it makes sense to always have the function be blocking. But I will test the code tomorrow and do another pass to see if it makes sense ☺️

ArthurSens · 2022-04-27T13:20:09Z

I believe this is good to go? Can I just remove the hold? 🙂

mads-hartmann · 2022-04-27T13:28:45Z

@ArthurSens When testing earlier it looked like it had broken tracing but I didn't have time to debug further. I will move this back to draft ☺️

mads-hartmann · 2022-04-28T12:31:38Z

Okay @ArthurSens this is finally ready for another review. In the future I think we want to separate the installation of monitoring satellite from the waiting for it to be ready. That way we'd avoid the problem of having a slice potentially spanning multiple phases, but I didn't want to make that change now as I've already spent too long on this 😅

This should PR should also fix the problem Chris has been experiencing with failed monitoring satellite deployments breaking the build as explained here.

The last job has some missing spans, but I think that's more likely to to be due to burst protection as you also called out here - that also explains why I saw issue sometime yesterday but not always.

mads-hartmann · 2022-04-28T12:56:43Z

@ArthurSens lol sorry, I have to get used to commits not dismissing previous reviews - I did want you to have a look at this before merging 😅 I don't want to revert it as I tested it pretty well, but if you have time please have a look and I can create a follow up PR with any changes you'd like to see

ArthurSens

No worries about the merge, nothing really important to change :)

ArthurSens · 2022-04-28T13:56:56Z

.werft/jobs/build/deploy-to-preview-environment.ts

+        // Deploying monitoring satellite to VM-based preview environments is currently best-effort.
+        // That means we currently don't wait for the promise here, and should the installation fail
+        // we'll simply log an error rather than failing the build.
+        //
+        // Note: Werft currently doesn't support slices spanning across multiple phases so running this
+        // can result in many 'observability' slices. Currently we close all the spans in a phase
+        // when we complete a phase. This means we can't currently measure the full duration or the
+        // success rate or installing monitoring satellite, but we can at least count and debug errors.
+        // In the future we can consider not closing spans when closing phases, or restructuring our phases
+        // based on parallelism boundaries


This is a golden comment 🤌, perfectly explains the code below!

ArthurSens · 2022-04-28T13:59:48Z

.werft/jobs/build/deploy-to-preview-environment.ts

@@ -462,7 +496,18 @@ async function deployToDevWithHelm(werft: Werft, jobConfig: JobConfig, deploymen
    werft.log(`observability`, "Installing monitoring-satellite...")
    if (deploymentConfig.withObservability) {
        try {
-            await installMonitoring(CORE_DEV_KUBECONFIG_PATH, namespace, nodeExporterPort, monitoringDomain, STACKDRIVER_SERVICEACCOUNT, false, jobConfig.observability.branch);
+            const installMonitoringSatellite = new MonitoringSatelliteInstaller({


Suggested change

const installMonitoringSatellite = new MonitoringSatelliteInstaller({

const monitoringSatelliteInstaller = new MonitoringSatelliteInstaller({

I'd change that just to be consistent with how you call it on other places

roboquat added release-note-none do-not-merge/work-in-progress size/XL labels Apr 22, 2022

mads-hartmann marked this pull request as ready for review April 22, 2022 12:57

mads-hartmann requested a review from a team April 22, 2022 12:57

roboquat removed the do-not-merge/work-in-progress label Apr 22, 2022

github-actions bot added the team: devx label Apr 22, 2022

ArthurSens approved these changes Apr 22, 2022

View reviewed changes

roboquat added the do-not-merge/hold label Apr 22, 2022

mads-hartmann force-pushed the mads-classify-monitoring-satellite branch from 193a8e7 to 794d226 Compare April 27, 2022 09:04

mads-hartmann marked this pull request as draft April 27, 2022 13:29

roboquat added the do-not-merge/work-in-progress label Apr 27, 2022

mads-hartmann added 3 commits April 28, 2022 08:58

Convert to class to reduce argument passing

1747e73

Dont use async in these methods

004384c

Make installMonitoring blocking

7913e4d

mads-hartmann force-pushed the mads-classify-monitoring-satellite branch from 794d226 to 7913e4d Compare April 28, 2022 08:58

mads-hartmann added 2 commits April 28, 2022 10:59

Install monitoring satellite async for VMs again

7be9e5b

Ensure install is actually non-blocking

c0eb857

mads-hartmann marked this pull request as ready for review April 28, 2022 12:30

roboquat removed the do-not-merge/work-in-progress label Apr 28, 2022

mads-hartmann removed the do-not-merge/hold label Apr 28, 2022

roboquat merged commit fc40211 into main Apr 28, 2022

roboquat deleted the mads-classify-monitoring-satellite branch April 28, 2022 12:32

ArthurSens reviewed Apr 28, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert to class to reduce argument passing #9490

Convert to class to reduce argument passing #9490

mads-hartmann commented Apr 22, 2022 •

edited

Loading

ArthurSens left a comment

ArthurSens Apr 22, 2022 •

edited

Loading

ArthurSens Apr 22, 2022

mads-hartmann Apr 28, 2022

mads-hartmann commented Apr 26, 2022 •

edited by werft-gitpod-dev-com bot

Loading

ArthurSens commented Apr 27, 2022 •

edited by werft-gitpod-dev-com bot

Loading

mads-hartmann commented Apr 27, 2022 •

edited by werft-gitpod-dev-com bot

Loading

mads-hartmann commented Apr 28, 2022 •

edited by werft-gitpod-dev-com bot

Loading

mads-hartmann commented Apr 28, 2022 •

edited by werft-gitpod-dev-com bot

Loading

ArthurSens left a comment

ArthurSens Apr 28, 2022

ArthurSens Apr 28, 2022

	const installMonitoringSatellite = new MonitoringSatelliteInstaller({
	const monitoringSatelliteInstaller = new MonitoringSatelliteInstaller({

Convert to class to reduce argument passing #9490

Convert to class to reduce argument passing #9490

Conversation

mads-hartmann commented Apr 22, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

ArthurSens left a comment

Choose a reason for hiding this comment

ArthurSens Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

ArthurSens Apr 22, 2022

Choose a reason for hiding this comment

mads-hartmann Apr 28, 2022

Choose a reason for hiding this comment

mads-hartmann commented Apr 26, 2022 • edited by werft-gitpod-dev-com bot Loading

ArthurSens commented Apr 27, 2022 • edited by werft-gitpod-dev-com bot Loading

mads-hartmann commented Apr 27, 2022 • edited by werft-gitpod-dev-com bot Loading

mads-hartmann commented Apr 28, 2022 • edited by werft-gitpod-dev-com bot Loading

mads-hartmann commented Apr 28, 2022 • edited by werft-gitpod-dev-com bot Loading

ArthurSens left a comment

Choose a reason for hiding this comment

ArthurSens Apr 28, 2022

Choose a reason for hiding this comment

ArthurSens Apr 28, 2022

Choose a reason for hiding this comment

mads-hartmann commented Apr 22, 2022 •

edited

Loading

ArthurSens Apr 22, 2022 •

edited

Loading

mads-hartmann commented Apr 26, 2022 •

edited by werft-gitpod-dev-com bot

Loading

ArthurSens commented Apr 27, 2022 •

edited by werft-gitpod-dev-com bot

Loading

mads-hartmann commented Apr 27, 2022 •

edited by werft-gitpod-dev-com bot

Loading

mads-hartmann commented Apr 28, 2022 •

edited by werft-gitpod-dev-com bot

Loading

mads-hartmann commented Apr 28, 2022 •

edited by werft-gitpod-dev-com bot

Loading