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

1.10 generated files breaking changes #570

Open
jsafrane opened this issue Aug 21, 2024 · 5 comments · May be fixed by #572
Open

1.10 generated files breaking changes #570

jsafrane opened this issue Aug 21, 2024 · 5 comments · May be fixed by #572
Assignees

Comments

@jsafrane
Copy link
Contributor

jsafrane commented Aug 21, 2024

Update to #552 has broken quite some Kubernetes CSI sidecars.

So far I identified two issues:

  1. gomock cannot easily compare two messages now. Protobuf now stores some private stuff in state field, e.g. NodeGetInfoResponse:

    spec/lib/go/csi/csi.pb.go

    Lines 4827 to 4830 in 2f34062

    type NodeGetInfoResponse struct {
    state protoimpl.MessageState
    sizeCache protoimpl.SizeCache
    unknownFields protoimpl.UnknownFields
    And this private field has quite random values.

    It is possible to implement a custom gomock matcher that compares just the public fields, or use proto.Equal, as suggested in https://protobuf.dev/reference/go/faq/#deepequal.

  2. It's not possible to copy message content. For example, this code copies value of NodeGetInfoResponse:

    nodeInfo, err := ctrl.GetNodeInfo(grpcClient, *operationTimeout)
    if err != nil {
        klog.Fatalf("Failed to get node info from CSI driver: %v", err)
    }
    nodeDeployment.NodeInfo = *nodeInfo

    Such a copy will copy also the private fields and one of the fields is sync.Mutex, which should not be copied (govet complains hard about it).
    In this particular case, the Kubernetes external-provisioner can use a pointer, it does not need to copy the struct content.

Both issues are solvable using proper protobuf functions (WIP PR here), but they were quite surprising for a minor go package bump.

I am not asking for a revert, just for a better communication of the breaking changes and semantic versioning of go packages. This should not be a minor package bump. There should be probably a separate repo with the generated go code, so we can bump the versions independently on CSI spec. Kubernetes solved this by calling all its go packages 0.x, so no go API stability should be expected, and they break their API from time to time.

@gnufied
Copy link
Contributor

gnufied commented Aug 21, 2024

I think generated mock defaults to Eq matcher and moving to a better matcher for protobuf matches is indeed correct solution. So we were probably using incorrect code all along. :(

The protobuf messages embedding RWMutex is a unfortunate decision, but its been taken 5 years ago - protocolbuffers/protobuf-go@c37adef#diff-781ba79d06e543a67fe8ac84c0732cbcb8f27ed43123e3a3bb81bcb4e84c8a8b so... :/

@jsafrane jsafrane linked a pull request Aug 26, 2024 that will close this issue
@jsafrane
Copy link
Contributor Author

I created #572 to regenerate the structures in the old way. A proof of concept that it works is at kubernetes-csi/external-provisioner#1266.

@saad-ali
Copy link
Member

saad-ali commented Sep 12, 2024

Thanks a lot @jsafrane for catching this and for helping put together a rollback PR.

We discussed this with at the k8s Storage SIG meeting and the CSI Community Meeting and got input from a CSI driver author (AWS EBS team) who went through a 1.10 version bump.

Based on those discussions, and given that:

  1. It is important to keep to an "up to date" version of our dependencies.
  2. The "on the wire" CSI RPC is backwards compatible and has not had breaking changes.
  3. These breaking changes are in code generation and are immediately obvious to SPs when they bump the API (build fails, there is no silent failure).

We have decided that we will keep the changes as is, and document them as breaking changes in release notes (and send out an email to notify folks).

@saad-ali saad-ali self-assigned this Sep 12, 2024
@jdef
Copy link
Member

jdef commented Sep 12, 2024 via email

@saad-ali
Copy link
Member

Release notes for 1.10.0 have been updated to indicate breaking change: https://github.com/container-storage-interface/spec/releases/tag/v1.10.0

I will send a follow up email to broadcast the breaking change more broadly.

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

Successfully merging a pull request may close this issue.

4 participants