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] fix error handling for register & update, show all admission-constraints for list #8330

Merged
merged 3 commits into from
Feb 22, 2022

Conversation

kylos101
Copy link
Contributor

@kylos101 kylos101 commented Feb 19, 2022

Description

The following changes are necessary to improve the experience for managing workspace clusters when using gpctl.

  1. Cluster list was not showing certain admission constraints, like has-more-resources. As an administrator, we want to see all admission constraints, so we can understand how access is controlled to workspace clusters.
  2. It was possible to register duplicate workspace cluster names. As an administrator, we want to prevent overwriting existing configured clusters and prevent unwanted behavior, so we can maximize system up-time and have predictability.
  3. The error message for update when a workspace cluster name does not exist was incorrect. As an administrator, we want to clearly see what is wrong when updating, so we can effectively manage configuration.

Related Issue(s)

These issues were found while building and and testing faster clusters for #8054.

How to test

The error message when a name does not exist on update:

gitpod /workspace/gitpod (kyleb/faster-clusters) $ gpctl clusters update score 0 --name defaul2
FATA[0001] rpc error: code = Unknown desc = a WorkspaceCluster with name defaul2 does not exist in the DB! 

update still works:

gitpod /workspace/gitpod (kyleb/faster-clusters) $ gpctl clusters update admission-constraint add has-more-resources --name workworkwork
cluster 'workworkwork' updated with admission constraint add:true  constraint:{has_more_resources:true}
gitpod /workspace/gitpod (kyleb/faster-clusters) $ gpctl clusters list
NAME                URL                           STATIC        STATE            SCORE        GOVERNED        ADMISSION CONSTRAINTS
workworkwork        5.6.7.8:9090           false         AVAILABLE        50           true            [has_more_resources:true]
default             dns:///ws-manager:8080        true          AVAILABLE        50           true            []

The output for list when a workspace cluster includes has-more-resources admission constraint:

#
gitpod /workspace/gitpod (kyleb/faster-clusters) $ gpctl clusters list
NAME                URL                           STATIC        STATE            SCORE        GOVERNED        ADMISSION CONSTRAINTS
workworkwork        5.6.7.8:9090           false         AVAILABLE        10000        true            [has_more_resources:true]
default             dns:///ws-manager:8080        true          AVAILABLE        50           true            []

The error when you try to register a workspace cluster with a name that already exists:

gitpod /workspace/gitpod (kyleb/faster-clusters) $ gpctl clusters list
NAME                URL                           STATIC        STATE            SCORE        GOVERNED        ADMISSION CONSTRAINTS
workworkwork        5.6.7.8:9090           false         CORDONED         50           true            []
default             dns:///ws-manager:8080        true          AVAILABLE        50           true            []
gitpod /workspace/gitpod (kyleb/faster-clusters) $ gpctl clusters register --name workworkwork --hint-cordoned --hint-govern --url 5.6.7.8:9090 --tls-path ./wsman-tls/
FATA[0001] rpc error: code = Unknown desc = a WorkspaceCluster with name workworkwork already exists in the DB 

Register still works:

gitpod /workspace/gitpod (kyleb/faster-clusters) $ gpctl clusters list
NAME           URL                           STATIC        STATE            SCORE        GOVERNED        ADMISSION CONSTRAINTS
default        dns:///ws-manager:8080        true          AVAILABLE        50           true            []
gitpod /workspace/gitpod (kyleb/faster-clusters) $ gpctl clusters register --name workworkwork --hint-cordoned --hint-govern --url 5.6.7.8:9090 --tls-path ./wsman-tls/
cluster registered: name:"workworkwork"  url:"5.6.7.8:9090"  tls:{redacted}  hints:{cordoned:true  govern:true}

Release Notes

Improve error handling for workspace cluster register & update
Show all admission constraints for workspace cluster list

Documentation

@codecov
Copy link

codecov bot commented Feb 19, 2022

Codecov Report

Merging #8330 (5031e01) into main (6497329) will decrease coverage by 20.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #8330       +/-   ##
===========================================
- Coverage   31.23%   11.17%   -20.06%     
===========================================
  Files          39       18       -21     
  Lines        5910      993     -4917     
===========================================
- Hits         1846      111     -1735     
+ Misses       3923      880     -3043     
+ Partials      141        2      -139     
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 ?
components-supervisor-app ?

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

Impacted Files Coverage Δ
...mponents/supervisor/pkg/supervisor/notification.go
components/supervisor/pkg/supervisor/services.go
components/supervisor/pkg/supervisor/supervisor.go
components/supervisor/pkg/supervisor/user.go
components/supervisor/pkg/dropwriter/dropwriter.go
components/supervisor/pkg/ports/exposed-ports.go
components/local-app/pkg/auth/auth.go
components/supervisor/pkg/ports/tunnel.go
components/supervisor/pkg/supervisor/tasks.go
components/supervisor/pkg/supervisor/config.go
... and 11 more

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 6497329...5031e01. Read the comment docs.

@kylos101 kylos101 force-pushed the kyleb/faster-clusters branch 3 times, most recently from d9eb681 to 03b29fb Compare February 20, 2022 00:05
@kylos101 kylos101 marked this pull request as ready for review February 20, 2022 00:11
@kylos101 kylos101 requested review from a team February 20, 2022 00:11
@github-actions github-actions bot added team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team labels Feb 20, 2022
if (!!clusterByName) {
throw new GRPCError(grpc.status.ALREADY_EXISTS, `a WorkspaceCluster with name ${req.name} already exists in the DB`);
}
if (Array.isArray(clusterByUrl) && clusterByUrl.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

hm. what if findFiltered will change in the future and will return something other than array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, since it's TypeScript this is overkill, null check and length check should be good. Thanks!

components/ws-manager-bridge/src/cluster-service-server.ts Outdated Show resolved Hide resolved
…stration and update

Prevent duplicate workspace cluster registration & improve error message for update when workspace cluster doesn't exist
@kylos101 kylos101 force-pushed the kyleb/faster-clusters branch from 03b29fb to 5031e01 Compare February 20, 2022 17:36
@kylos101 kylos101 self-assigned this Feb 20, 2022
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.

Thx for finding and fixing this @kylos101 🙏

@roboquat roboquat merged commit aa52b3e into main Feb 22, 2022
@roboquat roboquat deleted the kyleb/faster-clusters branch February 22, 2022 04:07
@roboquat roboquat added the deployed: workspace Workspace team change is running in production label Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: workspace Workspace team change is running in production release-note size/M team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants