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

ignition: add support for ignition config file #209

Merged
merged 3 commits into from
Oct 23, 2024

Conversation

lstocchi
Copy link
Contributor

it adds support to pass an ignition config file to vfkit that handles the provisioning process.

it resolves #124

Copy link

openshift-ci bot commented Oct 17, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

if err != nil {
return err
}
defer file.Close()
Copy link
Collaborator

Choose a reason for hiding this comment

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

i dont know what the policy is on this project, but recently we have starting capturing the error here and logrus it

}

socketPath := filepath.Join(os.TempDir(), "ignition.sock")
dev, err := VirtioVsockNew(1024, socketPath, true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

pop to const ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

and maybe line above?

@@ -86,6 +91,10 @@ func newVMConfiguration(opts *cmdline.Options) (*config.VirtualMachine, error) {
return nil, err
}

if err := vmConfig.AddIgnitionFileFromCmdLine(opts.IgnitionPath); err != nil {
return nil, err
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps consider wrapping this here to make it meaningful

@@ -198,3 +216,37 @@ func runVirtualMachine(vmConfig *config.VirtualMachine, vm *vf.VirtualMachine) e

return <-errCh
}

func startServer(ignitionFile string, ignitionSocketPath string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider passing ignitionFile as a reader

@baude
Copy link
Collaborator

baude commented Oct 17, 2024

flyby review, nice work work @lstocchi ... all my comments are largely things to think about, neat idea too.

@lstocchi lstocchi force-pushed the i124 branch 2 times, most recently from fec258c to 0e8abdd Compare October 17, 2024 21:43
@lstocchi
Copy link
Contributor Author

@baude thanks for the suggestions. Hopefully I got all of them. 👍

Gonna add some tests tomorrow morning so that it can be reviewed again

@baude
Copy link
Collaborator

baude commented Oct 18, 2024

@lstocchi being that ignition is on the rarer side of things, I wonder you should create contrib/ignition and drop something of an example in there ? I assume it is in JSON so not something you could heavily comment on but a README.md in that dir could explain it ? This is extra credit of course :)

Copy link
Collaborator

@baude baude left a comment

Choose a reason for hiding this comment

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

Just the proposal to add an example file comment ... lgtm otherwise

Copy link
Collaborator

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for working on this new feature!

pkg/config/json_test.go Show resolved Hide resolved
it adds support to pass an ignition config file to vfkit that handles the provisioning process.
Vfkit start a small server, listening to a unix socket to feed the config file, and create a virtio-vsock device using port 1024. This will be used by Ignition that read its configuration using an HTTP GET over a vsock connection on port 1024.
Server code taken from https://github.com/containers/podman/blob/6487940534c1065d6c7753e3b6dfbe253666537d/pkg/machine/applehv/ignition.go

Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
it updates the doc to explain how the ignition flag is used. It also adds a new section under contrib where to store an example of configuration and link to the ignition website for more details about specification, mime type and other examples.

Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
Copy link
Collaborator

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

func startIgnitionProvisionerServer(ignitionReader io.Reader, ignitionSocketPath string) error {
mux := http.NewServeMux()
mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) {
_, err := io.Copy(w, ignitionReader)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could use https://pkg.go.dev/net/http#ServeContent (no need to make this change right, this can be a follow-up)

Copy link

openshift-ci bot commented Oct 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, cfergeau

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 1351d5a into crc-org:main Oct 23, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add --ignition config.ign support
3 participants