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: tart macOS vm's as job container #2434

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

feat: tart macOS vm's as job container #2434

wants to merge 17 commits into from

Conversation

ChristopherHX
Copy link
Contributor

@ChristopherHX ChristopherHX commented Aug 24, 2024

adds the tart:// protocol to platform mapping

e.g. -P macos-14=tart://ghcr.io/cirruslabs/macos-sonoma-base:latest if you have a mac.

I have written the code in a payed dedicated host machine that is now deallocated, expenses satisfied by sponsoring of my other act based project while I don't see anything here, as I don't own a mac.

add-path is probably broken, but I ignore the fact as of now.

@ChristopherHX ChristopherHX requested a review from a team as a code owner August 24, 2024 12:00
@ChristopherHX ChristopherHX marked this pull request as draft August 24, 2024 12:00
Copy link
Contributor

github-actions bot commented Aug 24, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 10 0 0.03s
✅ REPOSITORY gitleaks yes no 2.35s
✅ REPOSITORY git_diff yes no 0.01s
✅ REPOSITORY grype yes no 8.51s
✅ REPOSITORY secretlint yes no 1.09s
✅ REPOSITORY trivy-sbom yes no 0.11s
✅ REPOSITORY trufflehog yes no 4.46s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

Copy link

codecov bot commented Aug 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.07%. Comparing base (5a80a04) to head (706746e).
Report is 122 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2434       +/-   ##
===========================================
+ Coverage   61.56%   75.07%   +13.51%     
===========================================
  Files          53       63       +10     
  Lines        9002    10019     +1017     
===========================================
+ Hits         5542     7522     +1980     
+ Misses       3020     1935     -1085     
- Partials      440      562      +122     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChristopherHX ChristopherHX marked this pull request as ready for review August 24, 2024 13:23
Comment on lines +200 to +211
func (e *Environment) Copy(destPath string, files ...*container.FileEntry) common.Executor {
return e.HostEnvironment.Copy(e.ToHostPath(destPath), files...)
}
func (e *Environment) CopyTarStream(ctx context.Context, destPath string, tarStream io.Reader) error {
return e.HostEnvironment.CopyTarStream(ctx, e.ToHostPath(destPath), tarStream)
}
func (e *Environment) CopyDir(destPath string, srcPath string, useGitIgnore bool) common.Executor {
return e.HostEnvironment.CopyDir(e.ToHostPath(destPath), srcPath, useGitIgnore)
}
func (e *Environment) GetContainerArchive(ctx context.Context, srcPath string) (io.ReadCloser, error) {
return e.HostEnvironment.GetContainerArchive(ctx, e.ToHostPath(srcPath))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I'm overriding GetActPath with a relocated vm path and I'm copying into the bind mount those change needs to be undone by ToHostPath before calling the super implementation

Comment on lines +213 to +218
func (e *Environment) GetRunnerContext(ctx context.Context) map[string]interface{} {
rctx := e.HostEnvironment.GetRunnerContext(ctx)
rctx["temp"] = e.ToContainerPath(e.TmpDir)
rctx["tool_cache"] = e.ToContainerPath(e.ToolCache)
return rctx
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed so RUNNER_TEMP are valid inside the vm, when used by actions cache

Comment on lines +196 to +198
func (e *Environment) GetActPath() string {
return e.ToContainerPath(e.HostEnvironment.GetActPath())
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed so script paths passed to exec are valid inside the vm

return err
}

return e.execRaw(ctx, "ln -sf '/Volumes/My Shared Files/act' /private/tmp/act")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed for working around bugs with paths using spaces, e.g. the shell templates of run steps

}

func (e Env) VirtualMachineID() string {
return e.JobID
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'm reusing the docker container names used by containers, so no prefix and so on


func (rc *RunContext) startTartEnvironment() common.Executor {
return func(_ context.Context) error {
return fmt.Errorf("You need macOS for tart")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone thinks to run this mode on another OS, abort as tart is not available and some syscall part doesn't compile anyway.

toolCache := filepath.Join(cacheDir, "tool_cache")
platImage := rc.runsOnImage(ctx)
platURI, _ := url.Parse(platImage)
query := platURI.Query()
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 thought that I'm smart, but no providing options in the -P <label>=tart://test?... query doesn't actually work so only defaults for now

"github.com/nektos/act/pkg/tart"
)

func (rc *RunContext) startTartEnvironment() common.Executor {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This started as a copy of hostenvironment, but is now different

func (rc *RunContext) IsTartEnv(ctx context.Context) bool {
platform := rc.runsOnImage(ctx)
image := rc.containerImage(ctx)
return image == "" && strings.HasPrefix(platform, "tart://")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

like -self-hosted, but with tart://. I forget this variant has only been used in act_runner as of now, but looks better.

@ChristopherHX ChristopherHX changed the title Act tart feat: tart macOS vm's as job ~container~ Aug 24, 2024
@ChristopherHX ChristopherHX changed the title feat: tart macOS vm's as job ~container~ feat: tart macOS vm's as job ~~container~~ Aug 24, 2024
@ChristopherHX ChristopherHX changed the title feat: tart macOS vm's as job ~~container~~ feat: tart macOS vm's as job container Aug 24, 2024
@ChristopherHX ChristopherHX added the area/image Relating to issues in the Docker image label Aug 24, 2024
@YOU54F
Copy link

YOU54F commented Sep 5, 2024

Just starting to test this out and it looks good on first pass. testing on a mac m1 mbp

if the gh action step fails, the container is left in a running state.

if the container fails to start (say 2 vm limit is reached), the container is left in a stopped state.

I've been using ssh, with tart / parallels / qemu to run jobs with -P ${PLATFORM}=-self-hosted on windows/macos/linux vms for the last year (https://gist.github.com/YOU54F/3ac099e54e48a31a69ac2d671aa878f6 / https://gist.github.com/YOU54F/789c53ae8380a1f90325952615635f8a )

I had a play with your runner.server & github-act-runner to build gh actions in my homelab with a set of vm's on a few diff machines. Cheers for your work

@ChristopherHX
Copy link
Contributor Author

if the gh action step fails, the container is left in a running state.

Does --rm change anything here? Act has a feature for docker containers to keep them running if a step fails so you can debug, but reusing the vm has not been working in my testing..

I had a play with your runner.server & github-act-runner to build gh actions in my homelab with a set of vm's on a few diff machines.

Yeah I have written and use my runner.server prog exactly for such a use case, but mostly with actions/runner as the actual runner.

pkg/runner/expression.go Outdated Show resolved Hide resolved
_, _ = rand.Read(randBytes)
miscpath := filepath.Join(cacheDir, hex.EncodeToString(randBytes))
actPath := filepath.Join(miscpath, "act")
if err := os.MkdirAll(actPath, 0o777); err != nil {
Copy link

@allexiusw allexiusw Oct 19, 2024

Choose a reason for hiding this comment

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

Should 0o777 be added as a constant? Maybe FULL_RWX or FULL_PERMS


func (rc *RunContext) startTartEnvironment() common.Executor {
return func(_ context.Context) error {
return fmt.Errorf("You need macOS for tart")

This comment was marked as outdated.

This comment was marked as outdated.

Copy link

@allexiusw allexiusw left a comment

Choose a reason for hiding this comment

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

I'm seeing not test coverage, do we require them in a PR in order to get merged in this repo?

@ChristopherHX
Copy link
Contributor Author

ChristopherHX commented Oct 19, 2024

I'm seeing not test coverage

This requires a self-hosted runner, macos-14 github runner cannot be used due to running already in a Virtual Machine.

Or stubbing tart, but then do we really know if tart works?
I wanted in the past to have a tart emulator that fakes creating VM, but didn't find anything,

This is not like intel macOS with nested virtualization... :(.

Copy link
Contributor

mergify bot commented Oct 20, 2024

@ChristopherHX this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Oct 20, 2024
Copy link
Contributor

mergify bot commented Oct 20, 2024

@ChristopherHX this pull request has failed checks 🛠

@ChristopherHX ChristopherHX marked this pull request as draft October 26, 2024 09:32
@ChristopherHX ChristopherHX marked this pull request as ready for review October 26, 2024 09:40
@mergify mergify bot removed the needs-work Extra attention is needed label Oct 26, 2024
@ChristopherHX
Copy link
Contributor Author

Added a test for the error caused by using the tart mapping in linux where it is not supported, now coverage is green.

We have a coverage collection problem for platforms outside of linux anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/image Relating to issues in the Docker image size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request for Support: Integration of Cirruslabs/Tart with Nektos/Act
3 participants