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

[sshproxy] Add heartbeating #7760

Merged
merged 5 commits into from
Jan 24, 2022
Merged

[sshproxy] Add heartbeating #7760

merged 5 commits into from
Jan 24, 2022

Conversation

csweichel
Copy link
Contributor

Description

Adds heartbeating to SSH PTY channels. This way, input from an SSH session counts towards a workspace's activity and prevents its timeout.

Related Issue(s)

Fixes #4779

How to test

  • start a workspace
  • SSH into that workspace using the owner token
  • produce input on the SSH session
  • observe that the workspace does not time out

ways to speed this up:

  • use a custom timeout annotation
  • check the debug logs of ws-proxy which will show when heartbeats are being sent

Release Notes

Support heartbeats from SSH sessions

@roboquat roboquat added release-note team: workspace Issue belongs to the Workspace team size/L labels Jan 21, 2022
@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #7760 (15577e0) into main (c96aaee) will decrease coverage by 20.16%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #7760       +/-   ##
===========================================
- Coverage   30.45%   10.28%   -20.17%     
===========================================
  Files         115       18       -97     
  Lines       18732     1001    -17731     
===========================================
- Hits         5705      103     -5602     
+ Misses      12538      897    -11641     
+ Partials      489        1      -488     
Flag Coverage Δ
components-content-service-app ?
components-content-service-lib ?
components-ee-agent-smith-app ?
components-ee-agent-smith-lib ?
components-gitpod-cli-app 10.28% <ø> (ø)
components-gitpod-protocol-go-lib ?
components-image-builder-mk3-app ?
components-installation-telemetry-app ?
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 ?
components-ws-daemon-app ?
components-ws-daemon-lib ?
components-ws-manager-app ?
components-ws-proxy-app ?
installer-raw-app ?

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

Impacted Files Coverage Δ
components/ws-daemon/pkg/content/service.go
components/ws-manager/pkg/manager/manager_ee.go
installer/pkg/components/ws-manager/tlssecret.go
components/supervisor/pkg/terminal/service.go
components/ws-daemon/pkg/content/archive.go
components/supervisor/pkg/ports/served-ports.go
...onents/content-service/pkg/service/blob-service.go
components/ws-daemon/pkg/content/hooks.go
...omponents/ws-manager/pkg/manager/pod_controller.go
...ents/image-builder-mk3/pkg/orchestrator/metrics.go
... and 87 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 c96aaee...15577e0. Read the comment docs.

Copy link
Contributor

@iQQBot iQQBot left a comment

Choose a reason for hiding this comment

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

Hi @csweichel I'm currently refactoring the SSH gateway, and I've made some fixes for the issues mentioned above #7772, to avoid conflicts I set the base branch to this PR

components/ws-proxy/pkg/sshproxy/server.go Show resolved Hide resolved
components/ws-proxy/pkg/sshproxy/forward.go Show resolved Hide resolved
@roboquat roboquat added size/XL and removed size/L labels Jan 24, 2022
@csweichel
Copy link
Contributor Author

Note for reviewers: re-introducing the ws-proxy <> ws-manager connection is not ideal. Down the road we could introduce a CRD/annotation-based mechanism.

@iQQBot
Copy link
Contributor

iQQBot commented Jan 24, 2022

Note for reviewers: re-introducing the ws-proxy <> ws-manager connection is not ideal. Down the road we could introduce a CRD/annotation-based mechanism.

Did you mean we update the lastActiveTime on pod annotation instead of db?

@csweichel
Copy link
Contributor Author

Note for reviewers: re-introducing the ws-proxy <> ws-manager connection is not ideal. Down the road we could introduce a CRD/annotation-based mechanism.

Did you mean we update the lastActiveTime on pod annotation instead of db?

Something like that - we don't have that annotation yet because of the load this would impose on the Kubernetes control plane. If we were to introduce something like that, we would drastically reduce the update frequency, close to the theoretical maximum of 1/2 * timeout.

@iQQBot
Copy link
Contributor

iQQBot commented Jan 24, 2022

/approve
/lgtm

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 282b72230d97a63deb90e91f901f91c50b950446

}, nil
},
PublicKeyCallback: func(conn ssh.ConnMetadata, key ssh.PublicKey) (*ssh.Permissions, error) {
args := strings.Split(conn.User(), "#")
Copy link
Contributor

Choose a reason for hiding this comment

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

change it because it broken for scp, scp use : to split host and path

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we haven't officially released it to the public yet, I don't think such a change would cause much trouble

@aledbf
Copy link
Member

aledbf commented Jan 24, 2022

/approve

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, iQQBot

Associated issue: #4779

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 f566dc0 into main Jan 24, 2022
@roboquat roboquat deleted the cw/fix-4779 branch January 24, 2022 23:00
@roboquat roboquat added deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Jan 25, 2022
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 deployed Change is completely running in production release-note size/XL team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[local-app] heartbeating (keep workspace alive via SSH / whilst using non-browser IDE's)
4 participants