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

Feat: beam shell #817

Merged
merged 45 commits into from
Jan 2, 2025
Merged

Feat: beam shell #817

merged 45 commits into from
Jan 2, 2025

Conversation

luke-lombardi
Copy link
Contributor

@luke-lombardi luke-lombardi commented Dec 31, 2024

Adds a shell command to let users connect to containers with their stub config. It can be used with any decorator that inherits from the DeployableMixin.

Screenshot 2025-01-01 at 6 52 13 PM

@luke-lombardi luke-lombardi marked this pull request as draft December 31, 2024 14:02
@luke-lombardi luke-lombardi force-pushed the ll/shell branch 2 times, most recently from ea51e20 to e39639c Compare December 31, 2024 14:08
@luke-lombardi luke-lombardi marked this pull request as ready for review December 31, 2024 20:42
pkg/abstractions/shell/http.go Show resolved Hide resolved
pkg/abstractions/shell/http.go Show resolved Hide resolved
pkg/abstractions/shell/shell.go Show resolved Hide resolved
pkg/abstractions/shell/shell.go Show resolved Hide resolved
pkg/abstractions/shell/shell.go Show resolved Hide resolved
Comment on lines +161 to +168
// Don't allow negative and 0-valued compute requests
if stubConfig.Runtime.Cpu <= 0 {
stubConfig.Runtime.Cpu = defaultContainerCpu
}

if stubConfig.Runtime.Memory <= 0 {
stubConfig.Runtime.Memory = defaultContainerMemory
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe make a validateContainerRequestParams for these checks and some of the others below? It would keep this section leaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes sense. I copied this from one of the other places we spawn containers

Copy link
Contributor

Choose a reason for hiding this comment

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

I second this

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a step up from this is to also have a function to configure mounts, secrets, cpu, memory, and gpu stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I just think it should be a separate PR

Comment on lines +222 to +231
echo "if [ -f /root/.bashrc ]; then . /root/.bashrc; fi" >> "/root/.profile";
echo "cd /mnt/code" >> "/root/.profile";
echo "export TERM=xterm-256color" >> "/root/.bashrc";
echo "alias ls='ls --color=auto'" >> "/root/.bashrc";
echo "alias ll='ls -lart --color=auto'" >> "/root/.bashrc";
sed -i 's/^#PasswordAuthentication.*/PasswordAuthentication yes/' /etc/ssh/sshd_config;
sed -i 's/^#PermitRootLogin.*/PermitRootLogin yes/' /etc/ssh/sshd_config;
sed -i 's/^#PubkeyAuthentication.*/PubkeyAuthentication no/' /etc/ssh/sshd_config;
echo "AllowUsers $USERNAME" >> /etc/ssh/sshd_config;
echo "PermitUserEnvironment yes" >> /etc/ssh/sshd_config;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we read-only mounted the sshd_config and .bashrc and then just copy the contents over in this command. My thinking is that it would clean this up a bit and also let us change bashrc without releasing.

pkg/gateway/services/stub.go Outdated Show resolved Hide resolved
pkg/abstractions/shell/shell.go Show resolved Hide resolved
pkg/abstractions/shell/http.go Show resolved Hide resolved
pkg/abstractions/shell/shell.go Show resolved Hide resolved
Copy link
Contributor

@nickpetrovic nickpetrovic left a comment

Choose a reason for hiding this comment

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

Left a few comments about the global contexts.

@luke-lombardi luke-lombardi merged commit 535d153 into main Jan 2, 2025
3 checks passed
luke-lombardi added a commit that referenced this pull request Jan 2, 2025
Adds a shell command to let users connect to containers with their stub
config. It can be used with any decorator that inherits from the
`DeployableMixin`.

<img width="609" alt="Screenshot 2025-01-01 at 6 52 13 PM"
src="https://github.com/user-attachments/assets/0d8232fc-6215-40d7-bfdd-1f717e550aef"
/>
@luke-lombardi luke-lombardi deleted the ll/shell branch January 2, 2025 21:20
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.

4 participants