Skip to content

[ws-proxy] Wait for workspace info until the request is canceled#2305

Merged
csweichel merged 1 commit intomasterfrom
cw/ws-proxy-wait-for-info
Nov 27, 2020
Merged

[ws-proxy] Wait for workspace info until the request is canceled#2305
csweichel merged 1 commit intomasterfrom
cw/ws-proxy-wait-for-info

Conversation

@csweichel
Copy link
Copy Markdown
Contributor

@csweichel csweichel commented Nov 26, 2020

This PR makes ws-proxy wait for workspace info.
We wait for workspace info to arrive until the request context is canceled.

fixes #2299

How to test

Place a lot of load on the system, or artificially delay the ws-man subscription e.g. using blowtorch.

Also, there's a unit test :)

@csweichel csweichel force-pushed the cw/ws-proxy-wait-for-info branch from 1f1159d to 366d300 Compare November 26, 2020 18:17
@csweichel csweichel marked this pull request as ready for review November 26, 2020 18:18
@akosyakov
Copy link
Copy Markdown
Member

akosyakov commented Nov 27, 2020

/werft run

👍 started the job as gitpod-build-cw-ws-proxy-wait-for-info.2

@geropl
Copy link
Copy Markdown
Member

geropl commented Nov 27, 2020

Started reviewing the code, will then have a closer look with blowtorch 👷‍♂️

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not have the c.cond.Broadcast() here instead of in doInsert? It would help separate concerns (locking/notification) from insertion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've moved here - and into Reinit - because then

  • we don't signal that much during reinit (one broadcast after all insertions)
  • the cond.Broadcast() is closer to the locking.

Comment thread components/ws-proxy/pkg/proxy/infoprovider.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A comment on what the cond signal means. Here it's used as "some new info available" if I'm not mistaken (as opposed to "data has changed (added/removed)" as one might tempted to think).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A short comment would be ❤️

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do I understand correctly that we need this goroutine because we want to wait for - select - on a condition variable which is not selectable? And we use cond because we don't have the buffering problems and the cond code is nicer in the other places?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exactly, we have two things we want to wait for: context cancelation or info becoming available. We cannot select on the cond, hence need to "turn it into a channel". We use a cond because we have potentially multiple WaitFors waiting, and we cannot use a single channel directly (except with a much more complicated locked map).

@geropl
Copy link
Copy Markdown
Member

geropl commented Nov 27, 2020

I delayed ws-manager -> ws-proxy for several seconds. It always worked when the delay was <10s. Above, the workspace pod would start fine but the UI did hang in Starting.... This is expected as we 10s as the connection timeout between ws-proxy and ws-manager.

@csweichel csweichel force-pushed the cw/ws-proxy-wait-for-info branch from 366d300 to 0fefa20 Compare November 27, 2020 10:39
Copy link
Copy Markdown
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.

Cool! 🚀

(build fails bc gofmt only)

@csweichel csweichel force-pushed the cw/ws-proxy-wait-for-info branch from 0fefa20 to 89b3c19 Compare November 27, 2020 10:59
@csweichel csweichel merged commit c2607e5 into master Nov 27, 2020
@csweichel csweichel deleted the cw/ws-proxy-wait-for-info branch November 27, 2020 11:04
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.

[ws-proxy] Access races ws-manager updates

3 participants