search

weaveworks / ignite

Ignite a Firecracker microVM
https://ignite.readthedocs.org
Apache License 2.0
3.49k stars 224 forks source link

Bump CNI Plugins to v1.0.1 #863

Closed gaby closed 2 years ago

gaby commented 3 years ago

CNI Plugins have officially graduated to stable with version 1.0.1 released yesterday. The biggest breaking change is the removal of the flannel plugin.

Release notes here: https://github.com/containernetworking/plugins/releases/tag/v1.0.1

gaby commented 3 years ago

Doing a search for flannel on this repo shows several mentions. Does this mean that now flannel has to be included as part of ignite or should the change not affect ignite?

stealthybox commented 3 years ago

Thanks @gaby ! Good catch Ignite doesn't directly depend on the flannel CNI plugin, so we are safe there.

We do have a guide for setting up networking with Flannel -- this needs to be updated to note that the flannel CNI plugin should be installed before this section: https://ignite.readthedocs.io/en/stable/networking/#configuring-the-nodes

The script for that tutorial depends on it here: https://github.com/weaveworks/ignite/blob/b5214a0/tools/ignite-flannel.sh#L20

stealthybox commented 3 years ago

I see I personally forgot to bump the go.mod in #805 and #731, but luxas did in #561. Looking at updating go.mod now to v1.0.1, @darkowlzz noticed there may be a dependency conflict with firecracker-go-sdk.

The most current fc-go-sdk commit is @ cni plugins v0.9.0 (https://github.com/firecracker-microvm/firecracker-go-sdk/blob/90688a4/go.mod#L8) but we are many commits behind on that dep as well.

stealthybox commented 3 years ago

@darkowlzz is going to look at PR'ing firecracker-microvm/firecracker-go-sdk to update containernetworking/plugins. They depend on a package that has moved.

gaby commented 3 years ago

The flannel cni plugin now lives here: https://github.com/flannel-io/cni-plugin

Sadly they haven't done a release yet, so no binaries available.

darkowlzz commented 3 years ago

Hi, I've created https://github.com/firecracker-microvm/firecracker-go-sdk/pull/351 to update CNI dependencies in firecracker-go-sdk and also another PR to tc-redirect-tap repo which fc-go-sdk depends on. I was able to locally run the tests in those repos with those changes successfully. I hope they get accepted and we can go ahead with this PR.

We should also update the default CNI config in https://github.com/weaveworks/ignite/blob/ca96ad43e0ea753ae3cda1fef8ce5c57991a024c/pkg/network/cni/cni.go#L57 to 1.0.0.

gaby commented 3 years ago

Seems like changing the default config version breaks CNI.

time="2021-08-27T04:21:26Z" level=error msg="failed to setup network for namespace \"ignite-1ad06c2be2bbbd49\": unsupported CNI result version \"1.0.0\""
darkowlzz commented 3 years ago

Seems like changing the default config version breaks CNI.

@gaby that's expected for now because we are still using a different version of CNI go dependencies, not v1.0.0. It'll be fine once we have updated the go dependencies.

gaby commented 3 years ago

It's related to CNI plugins itself, there's a PR for fixing this.

https://github.com/containernetworking/plugins/pull/652

gaby commented 3 years ago

CNI plugins 1.0.1 was just released which fixes the config file version!

https://github.com/containernetworking/plugins/releases/tag/v1.0.1

jhult commented 2 years ago

@stealthybox and @darkowlzz, any news on getting this merged?

gaby commented 2 years ago

@jhult Thanks for the reminder. I just pushed changes for v1.0.1 which official bumps the SPEC to 1.0.0 as stable.

I think that https://github.com/firecracker-microvm/firecracker-go-sdk/pull/351 needs to get merge before this PR can be merge.

The firecracker PR needs https://github.com/awslabs/tc-redirect-tap/pull/9 to be merge

gaby commented 2 years ago

Submitted https://github.com/awslabs/tc-redirect-tap/pull/11 to address the breaking bug in v1.0.0

jhult commented 2 years ago

@gaby and @stealthybox,

Can this be merged now that these 2 PRs have been merged?

gaby commented 2 years ago

@jhult The CI needs to be re-triggered.

darkowlzz commented 2 years ago

@gaby looking at the CI failure, https://weaveworks-foss.semaphoreci.com/jobs/6eb41f54-1bcd-4e6b-a94a-d08c49eaa109, it could be that the go dependencies need to be updated as well.

gaby commented 2 years ago

Ahhh! I'd take a look tomorrow

gaby commented 2 years ago

I really have no idea how to update these, Go makes this way too complicated 😆

gaby commented 2 years ago

If someone can point me out on how to update Go dependencies, I'd really appreciate it. Everything I try locally/github fails to build.

darkowlzz commented 2 years ago

@gaby The changes in https://github.com/weaveworks/ignite/pull/863/commits/d367a2e7552426c3d5a50fc1baba4dc37e13146b looks right. If go modules removes things from go.sum, no need to add it back manually. Maybe you just need to update the vendored dependencies with make tidy-in-docker. That'll update all the relevant files in the vendor directory.

gaby commented 2 years ago

Current output of running make tidy-in-docker 

go: finding module for package github.com/containernetworking/cni/pkg/types/current
github.com/weaveworks/ignite/pkg/container imports
    github.com/firecracker-microvm/firecracker-go-sdk imports
    github.com/containernetworking/cni/pkg/types/current: module github.com/containernetworking/cni@latest found (v1.0.1), but does not contain package github.com/containernetworking/cni/pkg/types/current

I remember @darkowlzz doing changes regarding that package in https://github.com/firecracker-microvm/firecracker-go-sdk/pull/351

gaby commented 2 years ago

It seems like https://github.com/firecracker-microvm/firecracker-go-sdk may need another PR.

Output:

ubuntu@ubuntu-vb:~/Desktop/ignite$ go get github.com/firecracker-microvm/firecracker-go-sdk
github.com/firecracker-microvm/firecracker-go-sdk imports
    github.com/containernetworking/cni/pkg/types/current: cannot find module providing package github.com/containernetworking/cni/pkg/types/current
darkowlzz commented 2 years ago

@gaby looking at the go.mod file, the version of firecracker-go-sdk is v0.22.0, which is more than a year old. To update it, you can edit go.mod file's firecracker-go-sdk require line to be:

...
github.com/firecracker-microvm/firecracker-go-sdk 3de2ae41e930c8ff1d726da9f513df11957f4de3
...

or the equivalent revision from the main branch of that repo. And then run tidy again. That should update everything accordingly. If that doesn't help, let me know. I can give it a try at resolving this.