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

Allow custom images on iterative_cml_runner resource #266

Merged
merged 1 commit into from
Jan 21, 2022

Conversation

0x2b3bfa0
Copy link
Member

Could close #231 if merged

@0x2b3bfa0 0x2b3bfa0 requested a review from a team November 23, 2021 22:53
@0x2b3bfa0 0x2b3bfa0 self-assigned this Nov 23, 2021
@0x2b3bfa0 0x2b3bfa0 added discussion Waiting for team decision enhancement New feature or request resource-runner iterative_runner TF resource labels Nov 23, 2021
@dacbd
Copy link
Contributor

dacbd commented Nov 24, 2021

Are there non-systemd images that could be selected(are there any popular non-systemd distros anymore)? That would definitely break, or consider that too much of archaic possibility to be concerned about?

@0x2b3bfa0
Copy link
Member Author

Are there non-systemd images that could be selected [...] ?

Of course! Custom images may use any other init system.

[...] are there any popular non-systemd distros anymore.

Alpine, which uses OpenRC. 🤷🏼 Although its popularity for containers is — by far — greater than for virtual machines.

That would definitely break, or consider that too much of archaic possibility to be concerned about?

We consider systemd as a hard dependency. 🙉 It's included on all the popular virtual machine images, so relying on it is relatively safe.

@dacbd
Copy link
Contributor

dacbd commented Nov 24, 2021

Are there non-systemd images that could be selected [...] ?

Of course! Custom images may use any other init system.

Yeah I realized this was a silly question 🦒

@0x2b3bfa0
Copy link
Member Author

Not really; that's an excellent addition to the documentation. I believe that users with custom images will also wonder what the minimum requirements are.

@casperdcl
Copy link
Contributor

casperdcl commented Nov 24, 2021

users with custom images will also wonder what the minimum requirements are.

we could add another FROM line midway through https://github.com/iterative/cml/blob/master/Dockerfile to distinguish optional deps from essential ones?

@0x2b3bfa0
Copy link
Member Author

Container images only require tar and coreutils. This is about machine images.

@casperdcl
Copy link
Contributor

don't we need terraform, node & cml too?

@0x2b3bfa0
Copy link
Member Author

Yes, for the iterative_cml_runner resource

@casperdcl casperdcl mentioned this pull request Nov 24, 2021
@0x2b3bfa0 0x2b3bfa0 requested review from a team and removed request for a team November 30, 2021 15:16
@DavidGOrtega
Copy link
Contributor

Im missing a check of terraform in the setup script to fail in case its not present.
If not the runner resource is going to destroy itself after timeout without an apparent reason.

@0x2b3bfa0
Copy link
Member Author

Despite not being set as computed in the schema, the startup_script argument is being overridden always by the runner installation script.

"startup_script": &schema.Schema{
Type: schema.TypeString,
ForceNew: true,
Optional: true,
Default: "",
Sensitive: true,
},

d.Set("startup_script", startupScript)

While the current startup_script behavior might not be as sensible as one could desire, it doesn't seem to be related to this pull request. 🤔

@DavidGOrtega
Copy link
Contributor

Despite not being set as computed in the schema, the startup_script argument is being overridden always by the runner installation script.

Thats weird. startup_script argument should be executed within the runner's script

{- if not .container}}
{{- if .setup}}{{.setup}}{{- end}}
sudo npm config set user 0 && sudo npm install --global @dvcorg/cml
{{- end}}

{{- if .runner_startup_script}}
{{.runner_startup_script}}
{{- end}}

{{- if not .container}}
sudo tee /usr/bin/cml.sh << 'EOF'
#!/bin/sh
{{- end}}

@0x2b3bfa0
Copy link
Member Author

@iterative/cml, can we merge this?

@0x2b3bfa0 0x2b3bfa0 merged commit 5063eea into master Jan 21, 2022
@0x2b3bfa0 0x2b3bfa0 deleted the cml-runner-custom-images branch January 21, 2022 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Waiting for team decision enhancement New feature or request resource-runner iterative_runner TF resource
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using a custom AMI doesn't work
4 participants