feat: add proto by FrankYang0529 · Pull Request #2 · longhorn/types

FrankYang0529 · 2024-02-20T09:20:48Z

Test Plan

Please build images from following PRs:

Case 1: New cluster

Clone longhorn/longhorn.

git clone https://github.com/longhorn/longhorn.git
cd longhorn/chart

Install the cluster with your images. For example:

helm install longhorn . --namespace longhorn-system --create-namespace \
    --set image.longhorn.engine.repository=frankyang/longhorn-engine \
    --set image.longhorn.engine.tag="arm64-latest" \
    --set image.longhorn.manager.repository=frankyang/longhorn-manager \
    --set image.longhorn.manager.tag="arm64-latest" \
    --set image.longhorn.instanceManager.repository=frankyang/longhorn-instance-manager \
    --set image.longhorn.instanceManager.tag="arm64-latest" \
    --set image.pullPolicy="Always" \
    --set image.longhorn.backingImageManager.repository=frankyang/backing-image-manager \
    --set image.longhorn.backingImageManager.tag="v3_20240417" \
    --set image.longhorn.shareManager.repository=frankyang/longhorn-share-manager \
    --set image.longhorn.shareManager.tag="arm64-latest" \
    --set defaultSettings.systemManagedPodsImagePullPolicy="always"

Create a volume or snapshot. Check basic operations can work.

Case 2: Upgrade from v1.6.1

Clone longhorn/longhorn and checkout to v1.6.1.

git clone https://github.com/longhorn/longhorn.git
cd longhorn/chart
git checkout v1.6.1

Install v1.6.1 cluster.

helm install longhorn . --namespace longhorn-system --create-namespace

Create a Volume, PVC, and try to write some data to the volume.
Upgrade the cluster to new images. For example.

git checkout master
helm upgrade longhorn . --namespace longhorn-system \
    --set image.longhorn.engine.repository=frankyang/longhorn-engine \
    --set image.longhorn.engine.tag="arm64-latest" \
    --set image.longhorn.manager.repository=frankyang/longhorn-manager \
    --set image.longhorn.manager.tag="arm64-latest" \
    --set image.longhorn.instanceManager.repository=frankyang/longhorn-instance-manager \
    --set image.longhorn.instanceManager.tag="arm64-latest" \
    --set image.pullPolicy="Always" \
    --set image.longhorn.backingImageManager.repository=frankyang/backing-image-manager \
    --set image.longhorn.backingImageManager.tag="v3_20240417" \
    --set image.longhorn.shareManager.repository=frankyang/longhorn-share-manager \
    --set image.longhorn.shareManager.tag="arm64-latest"

Before upgrading engine, delete a replica of the volume and make sure it can come back.
Detach the volume. Upgrade the engine to latest. Make sure it can be attached and data is still there.

FrankYang0529 · 2024-02-23T09:34:41Z

Hi @innobead, currently, different LH projects use different protobuf package naming.

longhorn-engine: ptypes
longhorn-instance-manager: imrpc
longhorn-spdk-engine: spdkrpc
backing-image-manager: longhorn.backingimagemanager.pkg.rpc
longhorn-share-manager: don't specify package name

Originally, I would like to fix the naming in this PR like pkg.<service name>. However, this introduces another problem. New generated code has different invoke path. Let's use ReplicaCreate for example:

longhorn-engine ptypes: /ptypes.ReplicaService/ReplicaCreate
longhorn-types enginerpc: /pkg.enginerpc.ReplicaService/ReplicaCreate

In a new LH cluster, this is not a problem. However, when we upgrade an old LH cluster, we may have both old and new longhorn-instance-manager and longhorn-engine. This will be an issue. I get an error like:

level=error msg="Failed to initialize im client to instance-manager-b3561682e04c14c53967b1711a0905e7 before monitoring" func="controller.(*InstanceManagerController).startMonitoring" file="instance_manager_controller.go:1373" controller=longhorn-instance-manager error="failed to check version of Instance Manager Instance Service Client for instance-manager-b3561682e04c14c53967b1711a0905e7 IP 10.244.133.199: failed to get version: rpc error: code = Unimplemented desc = unknown service pkg.imrpc.InstanceService" instance manager=instance-manager-b3561682e04c14c53967b1711a0905e7 node=worker-2

We have two ways to solve the problem:

We introduce an old grpc server to conversion the request to the new server.
Pros: we can unify protobuf package naming in longhorn/types.
Cons: The longhorn-manager architecture will be complex.
We keep the package name.
Pros: We don't need any change for longhorn-manager server.
Cons: The naming is different even if we have a repository to collect all protobuf.

innobead · 2024-02-23T15:20:59Z

As discussed with/ @FrankYang0529 , we will adopt option 2 to keep the package name as usual, as this is the original design principle of Protobuf (ex: backward compatibility) before we figure out a better way to mitigate this design debt.

innobead · 2024-03-15T06:18:48Z

@ejweber @shuo-wu @ChanYiLin Please review this.

innobead

LGTM. Just one suggestion about folder naming.

innobead · 2024-04-10T15:28:22Z

+
+package bimrpc;
+
+option go_package = "github.com/longhorn/types/pkg/bimrpc";


Suggested change

option go_package = "github.com/longhorn/types/pkg/bimrpc";

option go_package = "github.com/longhorn/types/pkg/generated/bimrpcpb";

For Python generated module, suggest generated-py/... instead of rpc-py. WDYT?

@innobead, If we want to update go_package in bimrpc, I will update all go_package with prefix github.com/longhorn/types/pkg/generated for consistency. The generated-py path also looks good to me.

Sounds good.

Updated it. Thank you.

innobead · 2024-04-10T15:33:52Z

@FrankYang0529 In addition, please use Github Action instead and we only need to run this on amd64.

shuo-wu · 2024-04-10T17:48:21Z

I am thinking we have the 3rd option:
Retain the old gRPC client code for existing longhorn-instance-manager and longhorn-engine. Then remove those codes in the next minor release.

FrankYang0529 · 2024-04-11T05:01:17Z

I am thinking we have the 3rd option: Retain the old gRPC client code for existing longhorn-instance-manager and longhorn-engine. Then remove those codes in the next minor release.

Hi @shuo-wu, when users upgrade LH, longhorn-instance-manager needs to connect old and new longhorn-engine. If we change protobuf package name, longhorn-instance-manager has to create two gRPC clients, one for old protobuf and another for new. There are 3 package names need to be changed: imrpc, ptypes, and smrpc. This may introduce some complexity in longhorn-manager and longhorn-instance-manager.

derekbit · 2024-04-11T05:47:25Z

I am thinking we have the 3rd option: Retain the old gRPC client code for existing longhorn-instance-manager and longhorn-engine. Then remove those codes in the next minor release.

Hi @shuo-wu, when users upgrade LH, longhorn-instance-manager needs to connect old and new longhorn-engine. If we change protobuf package name, longhorn-instance-manager has to create two gRPC clients, one for old protobuf and another for new. There are 3 package names need to be changed: imrpc, ptypes, and smrpc. This may introduce some complexity in longhorn-manager and longhorn-instance-manager.

@FrankYang0529 Have you tested the combability after upgrading Longhorn, for example, creating and deleting volumes in mixed old/new longhorn-manager, instance-instance-manager, and longhorn-engine cluster?

FrankYang0529 · 2024-04-12T02:12:04Z

I am thinking we have the 3rd option: Retain the old gRPC client code for existing longhorn-instance-manager and longhorn-engine. Then remove those codes in the next minor release.

Hi @shuo-wu, when users upgrade LH, longhorn-instance-manager needs to connect old and new longhorn-engine. If we change protobuf package name, longhorn-instance-manager has to create two gRPC clients, one for old protobuf and another for new. There are 3 package names need to be changed: imrpc, ptypes, and smrpc. This may introduce some complexity in longhorn-manager and longhorn-instance-manager.

@FrankYang0529 Have you tested the combability after upgrading Longhorn, for example, creating and deleting volumes in mixed old/new longhorn-manager, instance-instance-manager, and longhorn-engine cluster?

Yes, in my test steps, I think these two steps cover it.

5. Before upgrading engine, delete a replica of the volume and make sure it can come back.
6. Detach the volume. Upgrade the engine to latest. Make sure it can be attached and data is still there.

derekbit · 2024-04-12T02:42:02Z

+    rm -rf ./protobuf/vendor/protobuf/src/google/protobuf
+    DIR="./protobuf/vendor/protobuf/src/google/protobuf"
+    mkdir -p $DIR
+    wget https://raw.githubusercontent.com/protocolbuffers/protobuf/v24.3/src/google/protobuf/empty.proto -P $DIR


Make v24.3 as a global variable PROTOBUF_VER

Updated it. Thank you.

derekbit · 2024-04-12T02:45:33Z

+* protoc 24.3
+* protoc-gen-go 1.31.0
+* protoc-gen-go-grpc 1.3.0
+* python protobuf 4.24.3


nit: Where should developers make changes if they want to update?

Sorry, I add Dockerfile.dapper* to .gitignore, so we don't see it here. I will add it and force push again. We can change it in Dockerfile.dapper in the future.

I add Dockerfile.dapper. There are some variables can change the version. Thanks.

Signed-off-by: PoAn Yang <yangpoan@gmail.com>

FrankYang0529 · 2024-04-12T03:15:17Z

@FrankYang0529 In addition, please use Github Action instead and we only need to run this on amd64.

Added it. However, this is first PR to add GItHub Action, so it looks like it's not triggered in this PR.

innobead · 2024-04-17T06:29:09Z

No worries. You can update the action later if it doesn't work.

innobead

LGTM.

FrankYang0529 force-pushed the LH-6744 branch from 3748493 to 501fe5b Compare February 20, 2024 10:57

FrankYang0529 force-pushed the LH-6744 branch 6 times, most recently from fa4fbc6 to 25f1b5b Compare February 29, 2024 02:40

FrankYang0529 marked this pull request as ready for review February 29, 2024 05:34

FrankYang0529 force-pushed the LH-6744 branch from bc21201 to 43613e4 Compare February 29, 2024 05:40

FrankYang0529 mentioned this pull request Feb 29, 2024

[TASK] Collect all proto files in longhorn-types repo longhorn/longhorn#6744

Closed

FrankYang0529 requested a review from innobead February 29, 2024 05:53

innobead requested review from ChanYiLin, ejweber and shuo-wu March 15, 2024 06:18

FrankYang0529 force-pushed the LH-6744 branch 3 times, most recently from ee78f3e to 9ddb72a Compare April 8, 2024 02:34

innobead requested a review from derekbit April 10, 2024 15:21

innobead reviewed Apr 10, 2024

View reviewed changes

derekbit reviewed Apr 12, 2024

View reviewed changes

feat: add github workflows and license

0123178

Signed-off-by: PoAn Yang <yangpoan@gmail.com>

feat: add proto

3703d39

Signed-off-by: PoAn Yang <yangpoan@gmail.com>

FrankYang0529 force-pushed the LH-6744 branch from 9ddb72a to 3703d39 Compare April 12, 2024 03:13

innobead approved these changes Apr 17, 2024

View reviewed changes

innobead merged commit 43dea92 into longhorn:main Apr 17, 2024

FrankYang0529 deleted the LH-6744 branch April 17, 2024 07:19


		package bimrpc;

		option go_package = "github.com/longhorn/types/pkg/bimrpc";

Conversation

FrankYang0529 commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Plan

Case 1: New cluster

Case 2: Upgrade from v1.6.1

Uh oh!

FrankYang0529 commented Feb 23, 2024

Uh oh!

innobead commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

innobead commented Mar 15, 2024

Uh oh!

innobead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

innobead commented Apr 10, 2024

Uh oh!

shuo-wu commented Apr 10, 2024

Uh oh!

FrankYang0529 commented Apr 11, 2024

Uh oh!

derekbit commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FrankYang0529 commented Apr 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankYang0529 commented Apr 12, 2024

Uh oh!

innobead commented Apr 17, 2024

Uh oh!

innobead left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FrankYang0529 commented Feb 20, 2024 •

edited

Loading

innobead commented Feb 23, 2024 •

edited

Loading

derekbit commented Apr 11, 2024 •

edited

Loading