Keep preview environment context in sync #11202

mads-hartmann · 2022-07-07T13:06:57Z

Description

This PR implements the --watch option and uses it in .gitpod.yaml. I considered the following requirements:

It should deal with users changing branches
It should deal with users using "with-clean-slate-deployment"; that is, it should re-install the kubectx if the underlying VM is recreated.

Implementation

This changes the behaviour of "--watch" so that it runs forever and polls the current branch and VM periodically to see if a new preview environment context should be installed.

Alternative implementation

If we wanted to avoid polling could've created a channel for changes to the preview environment. For changes to the VM we could (I think) have used an Informer. Branch changes would be a bit more tricky though but might be achievable using a git hook that sends a signal to the process. Or if the current branch is stored in the filesystem we might be able to be notified on changes to that file.

I went with polling as that seemed like a smaller 🛹

Related Issue(s)

Fixes #11108

How to test

# Run manually during development (or start a workspace off this branch as gitpod.yaml runs --watch
go run main.go install-context --watch

# To test switching branches:
# In another terminal
# change between a few different branches. See that it installs the context
git checkout main
# wait
kubectx
git checkout - 
# wait
kubectx

# To test re-creating the VM
werft run github -a with-clean-slate-deployment=true

Release Notes

NONE

Documentation

Werft options:

/werft with-preview
N/A

ArthurSens

Hey Mads, thanks for picking this up!

I like the decision of watching the VMI creation timestamp as the information we use to decide if a preview should be re-installed!

What I didn't understand was the need to implement the watch loops 🤔.

@vulkoingim could probably elaborate more here, but my assumption is that when using informers (which we are), we're subscribing to specific events from kubernetes.

The events we're subscribed to are ADD and UPDATE from virtualmachines, but we decide to quit once we successfully install a context. Couldn't we just not quit, but keep watching for UPDATE instead?

dev/preview/previewctl/pkg/k8s/vm.go

dev/preview/previewctl/pkg/preview/preview.go

Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>

github-actions · 2022-07-11T10:34:33Z

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yml was changed and it might be harmful.

mads-hartmann · 2022-07-11T10:55:38Z

What I didn't understand was the need to implement the watch loops

The "outer" watch loop serve two purposes

Detecting if the VM has changed (e.g. was deleted and a new one created)
Detecting if the branch has been changed

We could most likely fix (1) by changing preview.InstallContext so that it keeps listening for changes to the VM and proxy. But we can't add (2) without a larger refactoring. A "preview" (the struct) is currently associated with a single branch (which I think makes sense) so if we wanted to use that loop to install contexts for VMs from other branches it would get weird rather quickly.

The implementation in this branch has the following separation of concerns:

preview.InstallContext is responsbile for installing a single context for a single VM. It implements a wait behaviour so it can be called even if the VM is not ready when you invoke it.
The install context command implements a watch loop that monitors for changes to the current branch or VM and invokes preview.InstallContext if anything changed

We could improve it slightly by moving the "VM was changed" logic into "preview.InstallContext" so that the outer loop only looks for changes to the branch. I can do a quick time-boxed attempt at that and see if the result is nicer ☺️

ArthurSens

As agreed in our team sync, let's get this merged since the PR solves the problem!

I just found a new problem here, but feel free to leave this to another PR if you prefer. We should be including any logic in the cmd package, all it is supposed to do is initialize commands and call functions 😬

Adding the /hold label so you can decide on tackling this now or in another PR

ArthurSens · 2022-07-12T12:15:09Z

dev/preview/previewctl/cmd/install_context.go

+	// Used to ensure that we only install contexts
+	var lastSuccessfulPreviewEnvironment *preview.Preview = nil
+
+	install := func(timeout time.Duration) error {
+		p, err := preview.New("", logger)
+
+		if err != nil {
+			return err
+		}
+
+		if lastSuccessfulPreviewEnvironment != nil && lastSuccessfulPreviewEnvironment.Same(p) {
+			logger.Infof("The preview envrionment hasn't changed")
+			return nil
+		}
+
+		err = p.InstallContext(true, timeout)
+		if err == nil {
+			lastSuccessfulPreviewEnvironment = p
+		}
+		return err
+	}
+


This part looks a bit like an anti-pattern for the pattern we established 😬

To keep every logic testable, I believe we that code inside the cmd package should do nothing more than declare the CLI commands and calling functions from other packages.

If we move this part to a separate function the preview package, we can create tests for it 🙂

ArthurSens · 2022-07-12T12:20:22Z

dev/preview/previewctl/cmd/install_context.go

+			if watch {
+				for range time.Tick(15 * time.Second) {
+					// We're using a short timeout here to handle the scenario where someone switches
+					// to a branch that doens't have a preview envrionment. In that case the default
+					// timeout would mean that we would block for 10 minutes, potentially missing
+					// if the user changes to a new branch that does that a preview.
+					err := install(30 * time.Second)
+					if err != nil {
+						logger.WithFields(logrus.Fields{"err": err}).Info("Failed to install context. Trying again soon.")
+					}
+				}
+			} else {
+				return install(timeout)


Same as above 😅

ArthurSens · 2022-07-12T12:26:49Z

/hold

mads-hartmann · 2022-07-12T12:44:46Z

@ArthurSens Thanks! I'll merge this and follow-up on this in another PR

Move logic away from cmd package
See if it makes sense to only care about git in the "outer loop" and extend the inner-loop to deal with VMs being recreated

If I don't manage to get a PR open before I leave for the week I'll create an issue about this instead ☺️

ArthurSens · 2022-07-12T12:46:51Z

If I don't manage to get a PR open before I leave for the week I'll create an issue about this instead ☺️

Sounds great!

Move logic to cmd package

from* cmd 😛

mads-hartmann · 2022-07-12T12:49:29Z

@ArthurSens haha, updated the comment to avoid confusion

mads-hartmann · 2022-07-15T09:39:44Z

Follow-up issue created as I didn't get to it this week https://github.com/gitpod-io/ops/issues/3357

roboquat added release-note-none do-not-merge/work-in-progress size/M size/L and removed size/M labels Jul 7, 2022

mads-hartmann force-pushed the mads/watch-forever branch from a876d21 to 37ccb49 Compare July 8, 2022 08:48

Implement --watch mode for previewctl install-context

750bc1a

mads-hartmann force-pushed the mads/watch-forever branch from d931d56 to 750bc1a Compare July 8, 2022 09:59

Ignore port 8022

89d763f

mads-hartmann marked this pull request as ready for review July 8, 2022 10:19

mads-hartmann requested a review from a team July 8, 2022 10:19

roboquat removed the do-not-merge/work-in-progress label Jul 8, 2022

github-actions bot added the team: devx label Jul 8, 2022

ArthurSens reviewed Jul 8, 2022

View reviewed changes

dev/preview/previewctl/pkg/k8s/vm.go Show resolved Hide resolved

dev/preview/previewctl/pkg/preview/preview.go Show resolved Hide resolved

dev/preview/previewctl/pkg/preview/preview.go Outdated Show resolved Hide resolved

Apply suggestions from code review

f84f11f

Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>

ArthurSens approved these changes Jul 12, 2022

View reviewed changes

roboquat added the do-not-merge/hold label Jul 12, 2022

mads-hartmann removed the do-not-merge/hold label Jul 12, 2022

roboquat merged commit 8338b6b into main Jul 12, 2022

roboquat deleted the mads/watch-forever branch July 12, 2022 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep preview environment context in sync #11202

Keep preview environment context in sync #11202

mads-hartmann commented Jul 7, 2022 •

edited

ArthurSens left a comment

github-actions bot commented Jul 11, 2022

mads-hartmann commented Jul 11, 2022 •

edited by werft-gitpod-dev-com bot

ArthurSens left a comment

ArthurSens Jul 12, 2022

ArthurSens Jul 12, 2022

ArthurSens commented Jul 12, 2022

mads-hartmann commented Jul 12, 2022 •

edited

ArthurSens commented Jul 12, 2022 •

edited by werft-gitpod-dev-com bot

mads-hartmann commented Jul 12, 2022

mads-hartmann commented Jul 15, 2022

Keep preview environment context in sync #11202

Keep preview environment context in sync #11202

Conversation

mads-hartmann commented Jul 7, 2022 • edited

Description

Implementation

Alternative implementation

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

ArthurSens left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 11, 2022

mads-hartmann commented Jul 11, 2022 • edited by werft-gitpod-dev-com bot

ArthurSens left a comment

Choose a reason for hiding this comment

ArthurSens Jul 12, 2022

Choose a reason for hiding this comment

ArthurSens Jul 12, 2022

Choose a reason for hiding this comment

ArthurSens commented Jul 12, 2022

mads-hartmann commented Jul 12, 2022 • edited

ArthurSens commented Jul 12, 2022 • edited by werft-gitpod-dev-com bot

mads-hartmann commented Jul 12, 2022

mads-hartmann commented Jul 15, 2022

mads-hartmann commented Jul 7, 2022 •

edited

mads-hartmann commented Jul 11, 2022 •

edited by werft-gitpod-dev-com bot

mads-hartmann commented Jul 12, 2022 •

edited

ArthurSens commented Jul 12, 2022 •

edited by werft-gitpod-dev-com bot