Skip to content

feat: [NPM] define transport shim layer using gRPC #1163

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Dec 14, 2021
Merged

feat: [NPM] define transport shim layer using gRPC #1163

merged 14 commits into from
Dec 14, 2021

Conversation

@nitishm
Copy link
Contributor

@nitishm nitishm commented Dec 7, 2021

User story AB#12707964

Reason for Change:
Define protobuf message and services for transport shim

Issue Fixed:
AB#12707965

Requirements:

Tasks:

  • Define protobuf file for all Datapath interface methods

Signed-off-by: Nitish Malhotra nitishm@microsoft.com

Notes:

@nitishm
Define transport protobuf model
86b3516 
Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
@rbtr
Copy link
Collaborator

rbtr commented Dec 7, 2021

how are you generating the pb.go? would be nice to define gen targets in the makefile for consistency 🙂
even better if you also don't assume protoc is installed and take care of that in a similar fashion as the existing go tools dependency targets.

@nitishm
Copy link
Contributor Author

nitishm commented Dec 7, 2021

how are you generating the pb.go? would be nice to define gen targets in the makefile for consistency 🙂 even better if you also don't assume protoc is installed and take care of that in a similar fashion as the existing go tools dependency targets.

Sure I can do that. Are we using it any place else today? I did see a few proto files across the code base.

@vakalapa
Copy link
Contributor

vakalapa commented Dec 8, 2021

how are you generating the pb.go? would be nice to define gen targets in the makefile for consistency 🙂 even better if you also don't assume protoc is installed and take care of that in a similar fashion as the existing go tools dependency targets.

Sure I can do that. Are we using it any place else today? I did see a few proto files across the code base.

@nitishm https://github.com/Azure/azure-container-networking/blob/master/npm/pkg/dataplane/Makefile Check this, Mat has created this MakeFile for auto-generating go mock. You can do something similar. My opinion is this does not have to be in global MakeFile, since we will only generate this once in a while.

@nitishm
Define transport protobuf model
d2f6ca1 
Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
@nitishm
Install protoc and add Makefile target protoc
c82df74 
Signed-off-by: Ubuntu <nitishm@microsoft.com>
rbtr
rbtr reviewed Dec 8, 2021
View reviewed changes
rbtr
rbtr reviewed Dec 8, 2021
View reviewed changes
@nitishm nitishm force-pushed the nitishm/feat/AB#12707965/transport-event-model-proto branch 2 times, most recently from a526847 to ffa077f Compare December 8, 2021 22:21
@nitishm nitishm force-pushed the nitishm/feat/AB#12707965/transport-event-model-proto branch from ffa077f to f414dcd Compare December 8, 2021 22:40
@nitishm
Copy link
Contributor Author

nitishm commented Dec 8, 2021

To test this out use the following instructions:

Download the protoc binary

./scripts/install-protoc.sh

Install the grpc go gen tool

make tools

Run generate target

cd npm
make generate

vakalapa
vakalapa reviewed Dec 8, 2021
View reviewed changes
vakalapa
vakalapa previously approved these changes Dec 8, 2021
View reviewed changes
Copy link
Contributor

@vakalapa vakalapa left a comment

Choose a reason for hiding this comment

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

Approving with minor comments.

@nitishm
switch from ID to Pod in proto message
ac59980 
Signed-off-by: Ubuntu <nitishm@microsoft.com>
@nitishm
Copy link
Contributor Author

nitishm commented Dec 9, 2021

@vakalapa can I get another approval. Also any idea why the CI doesn't like it?

ramiro-gamarra
ramiro-gamarra reviewed Dec 9, 2021
View reviewed changes
Copy link
Contributor

@ramiro-gamarra ramiro-gamarra left a comment

Choose a reason for hiding this comment

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

Got curious about this, so I'm leaving a few comments/questions :)

@nitishm
Implement registeration and dereg of clients
043dfb0 
- Implemented the reg and dereg of client on connection establishment
  and disconnection respectively
- Added sample server program to test this out

Signed-off-by: Ubuntu <nitishm@microsoft.com>
vakalapa
vakalapa reviewed Dec 10, 2021
View reviewed changes
Copy link
Contributor

@vakalapa vakalapa left a comment

Choose a reason for hiding this comment

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

Minor comments, please resolve all lints.

@nitishm
Address PR comments and add timestamp to reg/dereg events
c004bc3 
Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
@nitishm nitishm changed the title Define transport protobuf model feat: [NPM] define transport shim layer using gRPC Dec 10, 2021
@nitishm
Add API version enum and field to Events proto msg
a4cc884 
Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
nitishm
nitishm commented Dec 10, 2021
View reviewed changes
ramiro-gamarra
ramiro-gamarra reviewed Dec 10, 2021
View reviewed changes
@nitishm
start gRPC server as go routine
82e160f 
Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
nitishm
nitishm commented Dec 11, 2021
View reviewed changes
ramiro-gamarra
ramiro-gamarra reviewed Dec 13, 2021
View reviewed changes
@nitishm
Rename to appropriate filename/address review comments
684b4ed 
Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
@nitishm
Implement client and add test program to stream data
916e5e3 
Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
@nitishm
Addresses PR comments
bac0539 
- Move all proto files to npm/pkg/protos
- Rename pod and node fields to pod_name and node_name in
  transport.proto

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
@nitishm
Merge branch 'master' of https://github.com/azure/azure-container-net…
eb78fa9 
…working into nitishm/feat/AB#12707965/transport-event-model-proto
@nitishm
Fix lint issues
c65e7c1 
Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
@nitishm nitishm force-pushed the nitishm/feat/AB#12707965/transport-event-model-proto branch from 5f6d760 to c65e7c1 Compare December 14, 2021 19:23
@nitishm nitishm merged commit b6f04b6 into Azure:master Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rbtr rbtr rbtr left review comments

@ramiro-gamarra ramiro-gamarra ramiro-gamarra left review comments

@vakalapa vakalapa vakalapa approved these changes

@matmerr matmerr Awaiting requested review from matmerr

@huntergregory huntergregory Awaiting requested review from huntergregory

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nitishm @rbtr @vakalapa @ramiro-gamarra