Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Implement the containerd runtime for Ignite #337

Merged
merged 9 commits into from
Aug 23, 2019

Conversation

twelho
Copy link
Contributor

@twelho twelho commented Aug 15, 2019

This PR implements the alternative containerd runtime for Ignite. containerd v1.2.7, the latest stable version, was automatically vendored, but this bumps that to v1.3.0-beta.1 for multiple reasons:

  • Image exporting is not possible with v1.2.7, the old extractor was already dropped in that release before transitioning to the new implementation for v1.3.0
  • attach is natively implemented, this also enables logs
  • There is a new fully featured CLI for containerd I can use for implementation reference

The runtime interface has also received some changes to better adapt to both container runtimes.

Fixes #209.

cc @luxas

@twelho twelho added kind/feature Categorizes issue or PR as related to a new feature. kind/design Categorizes issue or PR as related to the design of the project. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. do-not-merge/wip The PR is still work in progress labels Aug 15, 2019
@twelho twelho added this to the v0.6.0 milestone Aug 15, 2019
@twelho twelho self-assigned this Aug 15, 2019
Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

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

Thanks for this initial piece of work!
Can you please split this up however, to do the containerd vendor bump in a totally different PR so this PR can be pretty small?

)

const (
ctdSocket = constants.DATA_DIR + "/containerd.sock"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this looks strange? The default containerd socket is /run/containerd/containerd.sock, and it should point there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed, it seems that there's no way to ask containerd where the socket is, so we might want to make this configurable in the future.


func (cc *ctdClient) AttachContainer(container string) error {
// TODO: Implement attach for containerd
return unsupported("logs")
Copy link
Contributor

Choose a reason for hiding this comment

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

attach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks

func (cc *ctdClient) ExportImage(image string) (io.ReadCloser, string, error) {
r, w := io.Pipe()
err := cc.client.Export(context.Background(), w)
return r, "", err // We don't need a temporary container, leave the ID string blank
Copy link
Contributor

Choose a reason for hiding this comment

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

Update our code to support a blank string here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does already support a blank string there. I might refactor the interface to handle a specific ContainerID type which can be nil so we can avoid confusion.

@@ -49,7 +49,7 @@ type ContainerConfig struct {

type Interface interface {
InspectImage(image string) (*ImageInspectResult, error)
PullImage(image string) (io.ReadCloser, error)
PullImage(image string) (io.ReadCloser, error) // TODO: This doesn't need to return the io.ReadCloser
Copy link
Contributor

Choose a reason for hiding this comment

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

please update the existing ignite code to support this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

}

func (cc *ctdClient) RunContainer(image string, config *runtime.ContainerConfig, name string) (s string, err error) {
img, err := cc.client.GetImage(context.Background(), image)
Copy link
Contributor

Choose a reason for hiding this comment

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

after this, add: https://gist.github.com/luxas/38fb8a5d3edca73414b6ef9c40300962#file-main-go-L61-L69

	unpacked, err := image.IsUnpacked(ctx, containerd.DefaultSnapshotter)
	if err != nil {
		return err
	}
	if !unpacked {
		if err := image.Unpack(ctx, containerd.DefaultSnapshotter); err != nil {
			return err
		}
	}

Copy link
Contributor Author

@twelho twelho Aug 20, 2019

Choose a reason for hiding this comment

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

Solved using containerd.WithPullUnpack when pulling

@luxas
Copy link
Contributor

luxas commented Aug 19, 2019

@twelho
Copy link
Contributor Author

twelho commented Aug 20, 2019

The containerd vendoring has been separated in #348, this PR has now been rebased on top of it.

@twelho
Copy link
Contributor Author

twelho commented Aug 20, 2019

Rebased on top of #356.

@luxas
Copy link
Contributor

luxas commented Aug 20, 2019

I implemented RunContainer now based off previous PoC code I had created

@twelho twelho changed the title [WIP] Implement the containerd runtime for Ignite Implement the containerd runtime for Ignite Aug 23, 2019
@twelho twelho removed the do-not-merge/wip The PR is still work in progress label Aug 23, 2019
@twelho
Copy link
Contributor Author

twelho commented Aug 23, 2019

The containerd runtime now has basic features complete and supports a full ignite run from scratch. It still needs work to be able to stop/remove VMs and to support the rest of the runtime features, but those improvements shall be implemented in separate PRs, this one is already quite massive. The containerd runtime is not enabled by default (as it's still incomplete).

@luxas luxas self-assigned this Aug 23, 2019
@luxas
Copy link
Contributor

luxas commented Aug 23, 2019

Awesome work 🎉 ✨!!
Let's get this going

@luxas luxas merged commit d1d97db into weaveworks:master Aug 23, 2019
@twelho twelho deleted the containerd branch August 23, 2019 18:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/design Categorizes issue or PR as related to the design of the project. kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add containerd support as the container runtime
2 participants