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

[ws-manager-bridge] Support forced cluster dereg #6682

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

csweichel
Copy link
Contributor

Description

This PR adds support for forced workspace cluster deregistration.
When de-registering a cluster, ws-manager-bridge checks if there are still any workspaces which belong to this cluster and are marked as running. If we removed the cluster in this case, those instances would never again receive a status update - also the reconciliation in ws-manager-bridge would no longer act on those workspaces (the cluster is gone after all, what should we reconcile with).

With the newly added --force flag on gpctl, we can override this safety mechanism in exceptional cases, e.g. when introducing new workspace cluster types.

How to test

  1. Register the same ws-manager as dynamic cluster and give it a very high score
  2. Start a workspace on that cluster
  3. Deregister the newly added cluster - should not work without --force, but should work with it.

Release Notes

[gpctl] Support forceful cluster de-registration

@roboquat roboquat added release-note team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team size/L labels Nov 12, 2021
@csweichel
Copy link
Contributor Author

/approve no-issue

@aledbf
Copy link
Member

aledbf commented Nov 12, 2021

/lgtm

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: cc1fc47b4fd6f3698a4cf56f2764cbf94e4dee5d

@csweichel
Copy link
Contributor Author

csweichel commented Nov 15, 2021

/werft run

👍 started the job as gitpod-build-cw-cluster-force-dereg.1

@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #6682 (822cf4f) into main (43f041d) will decrease coverage by 1.72%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##            main   #6682      +/-   ##
========================================
- Coverage   7.89%   6.16%   -1.73%     
========================================
  Files         14      12       -2     
  Lines       1254    1086     -168     
========================================
- Hits          99      67      -32     
+ Misses      1152    1018     -134     
+ Partials       3       1       -2     
Flag Coverage Δ
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 ?
installer-raw-app 6.16% <ø> (ø)

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

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43f041d...822cf4f. Read the comment docs.

@geropl
Copy link
Member

geropl commented Nov 18, 2021

👍 for the PR. We just need to be aware that leaving dangling workspaces around currently breaks UX for people that started those (bc they might run into an unresolvable "max parallel workspaces" situation). We had this incident end of last week with a temporary k3-eu102 cluster, for instance.

So fine with merging this as long as we:

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 , but keep #6682 (comment) in mind.

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, csweichel, geropl

Associated issue requirement bypassed by: csweichel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit e1f1183 into main Nov 18, 2021
@roboquat roboquat deleted the cw/cluster-force-dereg branch November 18, 2021 12:00
@roboquat roboquat added the deployed: workspace Workspace team change is running in production label Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved deployed: workspace Workspace team change is running in production release-note size/L team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants