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

Add Workbench session init container image #862

Merged
merged 10 commits into from
Oct 30, 2024
Merged

Conversation

skyeturriff
Copy link
Contributor

@skyeturriff skyeturriff commented Oct 29, 2024

Part of https://github.com/rstudio/rstudio-pro/issues/6920

Hoping to get some early(ish) feedback on adding a new image. I know this might be a bit awkward given this repo is in a bit of transition, so want to help mitigate any headaches/future pains where I can and make sure I'm doing things properly!

This image will be the new init container as part of Workbench's efforts to improve session components. I used connect-content-init as my example for how to set this up.

So far I've only added a new bake target for a daily build. I added this to the build-manual and build-bake-preview github workflows.

NOTE This also updates the rstudio_workbench_daily branch to kousa dogwood in get-version.py

I've tested this locally via

just preview-bake workbench-session-init-daily
mkdir init
docker run --rm -v $(pwd)/init:/mnt/init rstudio/workbench-session-init-preview:workbench-session-init-jammy-2024.11.0-daily-328.pro3

I also included a basic goss test.

@CLAassistant
Copy link

CLAassistant commented Oct 29, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@zachhannum zachhannum left a comment

Choose a reason for hiding this comment

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

Should also get approval from someone on platform, but I think it would be good to go ahead and get this into dailies for QA and testing! IMO we can push up additional PRs to add the release build and swap out the run script for something more elegant.

workbench-session-init/Dockerfile.ubuntu2204 Outdated Show resolved Hide resolved
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@bschwedler
Copy link
Contributor

Will this also help us reduce what is included in the session image?

Copy link
Contributor

@bschwedler bschwedler left a comment

Choose a reason for hiding this comment

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

This looks good overall. Thanks for adding these!

Comment on lines +434 to +436
PACKAGE_MANAGER_DAILY_VERSION: ${{ needs.versions.outputs.PACKAGE_MANAGER_DAILY_VERSION }}
PACKAGE_MANAGER_PREVIEW_VERSION: ${{ needs.versions.outputs.PACKAGE_MANAGER_PREVIEW_VERSION }}
CONNECT_DAILY_VERSION: ${{ needs.versions.outputs.CONNECT_DAILY_VERSION }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to specify the version for products we aren't building in the workflow?

I don't yet fully understand if all bake targets require all these env vars.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ianpittwood This is really a question for you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah. It's a bake limitation. It doesn't handle null values on build variables even if they're not part of the specified targets.

Comment on lines +6 to +9
/opt/session-components/:
exists: true
mode: "0755"
filetype: directory
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are specific session component files, we should probably check for them here.

If we want to check the indivdual contents of a tarfile that was added to the container, we could use a command block, running tar with the -t flag to ensure that the tarball has the expected contents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, I think the issue here is the tarball gets cleaned up in the last line of the Dockerfile RUN command, so it won't be around for goss to test against. But I think it's good practice to verify this, so I could check against the extracted directory structure. I'm uncertain how this would behave across versions though, if say we removed or added contents to the archive in a future release. I think in that case, we'd need to update the test, which would then only run successfully against the latest version?

workbench-session-init/run.sh Outdated Show resolved Hide resolved
Co-authored-by: Benjamin R. J. Schwedler <[email protected]>
@skyeturriff
Copy link
Contributor Author

Will this also help us reduce what is included in the session image?

@bschwedler That's the goal, yes! We'll use the init container to load all the session-specific components( vscode, rsession, rserver-launcher, etc.) into the session image, so the session image itself only needs to include the basics (system dependencies, R, python, etc.). Zach's prototype includes an example "base" session image here

Copy link
Collaborator

@ianpittwood ianpittwood left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +434 to +436
PACKAGE_MANAGER_DAILY_VERSION: ${{ needs.versions.outputs.PACKAGE_MANAGER_DAILY_VERSION }}
PACKAGE_MANAGER_PREVIEW_VERSION: ${{ needs.versions.outputs.PACKAGE_MANAGER_PREVIEW_VERSION }}
CONNECT_DAILY_VERSION: ${{ needs.versions.outputs.CONNECT_DAILY_VERSION }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah. It's a bake limitation. It doesn't handle null values on build variables even if they're not part of the specified targets.

@skyeturriff
Copy link
Contributor Author

I'm going to merge this in as-is so we can start to get dailies for testing, and address what's remaining (adding the release build, swapping out the run script, a more robust goss test) in follow-up PR(s)

@skyeturriff skyeturriff merged commit 02bfa6f into dev Oct 30, 2024
40 checks passed
@skyeturriff skyeturriff deleted the workbench-session-init branch October 30, 2024 19:58
@zachhannum
Copy link
Contributor

@ianpittwood @bschwedler does the repo need to be created manually on the dockerhub side of things? https://github.com/rstudio/rstudio-docker-products/actions/runs/11611641762/job/32333438273#step:4:1398

@ianpittwood
Copy link
Collaborator

@ianpittwood @bschwedler does the repo need to be created manually on the dockerhub side of things? https://github.com/rstudio/rstudio-docker-products/actions/runs/11611641762/job/32333438273#step:4:1398

Oh yes, let me go do that for you guys.

@ianpittwood
Copy link
Collaborator

@zachhannum @skyeturriff it had a successful build now to Dockerhub.

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.

5 participants