[cloud] Auto-port forward during cloud shell #586
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gcurtis
approved these changes
Feb 9, 2023
internal/services/status.go
Outdated
| return status, errors.WithStack(json.Unmarshal(content, status)) | ||
| } | ||
|
|
||
| func hostname() (string, error) { |
| return errors.WithStack(err) | ||
| } | ||
| return cloud.Shell(cmd.ErrOrStderr(), box.ProjectDir(), flags.githubUsername) | ||
| return cloud.Shell(cmd.Context(), cmd.ErrOrStderr(), box.ProjectDir(), flags.githubUsername) |
There was a problem hiding this comment.
Where does this context get set?
There was a problem hiding this comment.
It gets set when we execute cobra root command https://github.com/jetpack-io/devbox/blob/main/internal/boxcli/midcobra/midcobra.go#L64-L64
internal/services/services.go
Outdated
| fmt.Fprintf(w, "Waiting for port forwarding to start/stop for service %q\n", serviceName) | ||
| waitGroup.Add(1) | ||
|
|
||
| ctx, cancel := context.WithCancel(ctx) |
There was a problem hiding this comment.
Can the cancel of this context be deferred? It ultimately needs to be called everywhere otherwise it'll leak resources.
internal/services/services.go
Outdated
|
|
||
| // After 5 seconds, if the context has not been canceled, go ahead and cancel it | ||
| // and also decrement the wait group so that the caller can continue. | ||
| time.AfterFunc(5*time.Second, func() { |
There was a problem hiding this comment.
Can this be done with context.WithTimeout?
There was a problem hiding this comment.
Yes, it would need a timeout and also a cancel. I'll change
internal/services/services.go
Outdated
|
|
||
| // Done function is thread safe and will cancel the context and decrement the wait group | ||
| // if context is already canceled, it will not do anything | ||
| var m sync.Mutex |
There was a problem hiding this comment.
Could this be a sync.Once instead of a mutex?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR enables automatic port forwarding when services are started in devbox cloud. It does this by maintaining synced "status" files in
.devbox.cloud/[host-id]How to review this PR:
cloud.AutoPortForwardis a background thread that listens to changes in.devbox.cloud/[host-id]and starts port forwarding as needed. It only runs locally.services.ListenToChangeslistens to changes in.devbox.cloud/[host-id]and calls a callback when an event occurs. The path of a service status file is.devbox.cloud/[host-id]/[service-name].json. It was modified to listen to statuses of individual services. I did this because having a singleservices.jsoncould lead to file edit race conditions when starting/stoping multiple services.services.listenToAutoPortForwardingChangesOnRemoteonly runs on the cloud. It's the complement tocloud.AutoPortForward. It listens for when port forwarding has already been set up so it can print out the local ports on the cloud shell. It waits for 5 seconds and then gives up.How was it tested?
devbox services start/stopin several combinations and observed that port forwarding started and stopped as expected.