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

'genproto' workaround blocks Google Cloud imports #1212

Closed
cfircohen opened this issue Jul 18, 2023 · 10 comments
Closed

'genproto' workaround blocks Google Cloud imports #1212

cfircohen opened this issue Jul 18, 2023 · 10 comments

Comments

@cfircohen
Copy link

Hello, I'm adding Peer-pod support on GCP, see https://github.com/cfircohen/cloud-api-adaptor/tree/peerpod_gcp/gcp.

I'm currently blocked by issue 62 ("panic: protobuf tag not enough fields in Status.state") which has a workaround that replaces genproto with an older 2020 version. Workaround was removed in issue 175 ("go.mod contains unnecesary replace directive") and reintroduced in issue 180 ("Removal of the TTRPC workaround causes panic").

Google cloud GO SDK (cloud.google.com/go/compute) requires the latest genproto package: the older 2020 version doesn't have the necessary bindings, and downgrading GCP client SDK also doesn't work due to missing packages in the old 2020 genproto.

Please advise how to proceed.

@bpradipt
Copy link
Member

@yoheiueda ptal

@yoheiueda
Copy link
Member

I don't have a quick answer to this problem...

We could work around the issue, if golang could allow coexistance of multiple module versions in a single binary, but I guess it is not allowed in the go module system. https://go.dev/ref/mod#minimal-version-selection

The best solution is to eliminate the dependency in the Kata Containers.

I removed the replace line from Kata's go.mod file, and I was able to build the kata shim.

https://github.com/kata-containers/kata-containers/blob/7153b51578e8b24cf34a53134fcd0cb43060f1de/src/runtime/go.mod#L116

git clone -b CCv0 https://github.com/kata-containers/kata-containers.git
cd kata-containers/src/runtime
go mod edit -dropreplace google.golang.org/genproto
go mod tidy
go mod vendor
make $PWD/containerd-shim-kata-v2

I haven't tested its functionality yet.

If we can confirm that peer pods work with the modified kata shim, we can raise a PR to Kata Containers.

@yoheiueda
Copy link
Member

yoheiueda commented Jul 19, 2023

The protobuf API for agent protocol defined in Kata is referenced in Cloud API Adaptor, so I think removing dependency to Kata is difficult for now.

pbHypervisor "github.com/kata-containers/kata-containers/src/runtime/protocols/hypervisor"

@cfircohen
Copy link
Author

I don't have a quick answer to this problem...

We could work around the issue, if golang could allow coexistance of multiple module versions in a single binary, but I guess it is not allowed in the go module system. https://go.dev/ref/mod#minimal-version-selection

The best solution is to eliminate the dependency in the Kata Containers.

I removed the replace line from Kata's go.mod file, and I was able to build the kata shim.

https://github.com/kata-containers/kata-containers/blob/7153b51578e8b24cf34a53134fcd0cb43060f1de/src/runtime/go.mod#L116

git clone -b CCv0 https://github.com/kata-containers/kata-containers.git
cd kata-containers/src/runtime
go mod edit -dropreplace google.golang.org/genproto
go mod tidy
go mod vendor
make $PWD/containerd-shim-kata-v2

I haven't tested its functionality yet.

If we can confirm that peer pods work with the modified kata shim, we can raise a PR to Kata Containers.

How do you normally test these changes? I assume Kata has an automatic e2e testing environment? Did all the unit-tests pass? Were you able to build a new podvm image and run "confidential-containers" e2e tests against it?

@yoheiueda
Copy link
Member

How do you normally test these changes? I assume Kata has an automatic e2e testing environment? Did all the unit-tests pass? Were you able to build a new podvm image and run "confidential-containers" e2e tests against it?

I didn't tested the functionality when I posted my previous comment.

I just tested the updated go.mod using cloud-api-adaptor, and got the the following error.

2023/07/20 07:49:21 [adaptor/proxy] StartContainer: containerID:70d7e9067494ad82e66f684baf7f9917430cf2194b8823024f76e43b250a8422
panic: protobuf tag not enough fields in Status.state:

goroutine 71 [running]:
github.com/gogo/protobuf/proto.(*unmarshalInfo).computeUnmarshalInfo(0xc000616fa0)
	/root/go/pkg/mod/github.com/gogo/[email protected]/proto/table_unmarshal.go:341 +0x139f
github.com/gogo/protobuf/proto.(*unmarshalInfo).unmarshal(0xc000616fa0, {0xc0002e0ba0?}, {0xc000940582, 0x11, 0x558})
	/root/go/pkg/mod/github.com/gogo/[email protected]/proto/table_unmarshal.go:138 +0x67
github.com/gogo/protobuf/proto.makeUnmarshalMessagePtr.func1({0xc000940581, 0x12, 0x559}, {0x0?}, 0x0?)
	/root/go/pkg/mod/github.com/gogo/[email protected]/proto/table_unmarshal.go:1826 +0x151
github.com/gogo/protobuf/proto.(*unmarshalInfo).unmarshal(0xc000616f00, {0x1ce9700?}, {0xc000940580?, 0xc00062ada0?, 0x40d907?})
	/root/go/pkg/mod/github.com/gogo/[email protected]/proto/table_unmarshal.go:175 +0x35f
github.com/gogo/protobuf/proto.(*InternalMessageInfo).Unmarshal(0xc0002cc820?, {0x21ec1c0, 0xc0002cc1c0}, {0xc000940580?, 0x13?, 0x55a?})
	/root/go/pkg/mod/github.com/gogo/[email protected]/proto/table_unmarshal.go:63 +0xd0
github.com/gogo/protobuf/proto.(*Buffer).Unmarshal(0xc000565e18, {0x21ec1c0, 0xc0002cc1c0})
	/root/go/pkg/mod/github.com/gogo/[email protected]/proto/decode.go:424 +0x153
github.com/gogo/protobuf/proto.Unmarshal({0xc000940580, 0x13, 0x55a}, {0x21ec1c0, 0xc0002cc1c0})
	/root/go/pkg/mod/github.com/gogo/[email protected]/proto/decode.go:342 +0xe6
github.com/containerd/ttrpc.(*Client).recv(0xc0003d6220?, 0x0?, 0x0?)
	/root/go/pkg/mod/github.com/containerd/[email protected]/client.go:378 +0xf5
github.com/containerd/ttrpc.(*Client).run.func2()
	/root/go/pkg/mod/github.com/containerd/[email protected]/client.go:318 +0x24b
created by github.com/containerd/ttrpc.(*Client).run
	/root/go/pkg/mod/github.com/containerd/[email protected]/client.go:286 +0x17b
exit status 2

I think Kata needs to migrate to the google/protobuf from gogo/protobuf, since goto/protobuf has been deprecated.

gogo/protobuf is widely used in Kata, so replacing all of them will require some work...
https://github.com/search?q=repo%3Akata-containers%2Fkata-containers+gogo%2Fprotobuf&type=code

@cfircohen
Copy link
Author

Thanks for testing, @yoheiueda.
I see that @littlejawa had a comment here about Containerd moving away from gogo/protobuf in their main branch, and how Kata can now explore removing that dependency as well. Should we ping Julien and ask for status?

@yoheiueda
Copy link
Member

@cfircohen That would be great.

I am trying to update the protobuf code in Kata Containers, but haven't succeeded yet. I am currently facing this issue containerd/containerd#8850.

@yoheiueda
Copy link
Member

To fix this issue, I believe we need to remove the dependency to gogo in Kata. I raised an issue about it kata-containers/kata-containers#7420.

I think we need some volunteers to fix the issue in Kata, since it is complicated, and needs knowledge of both Kata and protobuf.

If fixing the issue in Kata needs some time, we may need to develop some work around in the cloud-api-adaptor side.

@bpradipt @stevenhorsman @littlejawa any comments?

@beraldoleal
Copy link
Member

beraldoleal commented Sep 12, 2023

Hi @yoheiueda and @cfircohen , sorry for the late reply here.

Just to give an update: we are working to remove gogoprotobuff from Kata and make this move as soon as possible. It is a high-priority for us right now.

In meanwhile I will check with others if workarounds are possible here.

@beraldoleal
Copy link
Member

genproto workaround is not in place since #1754. Closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants