Skip to content
This repository has been archived by the owner on Mar 31, 2023. It is now read-only.

[Tracing] Support Jaeger tracing in NCM and TC #707

Merged
merged 136 commits into from
Dec 10, 2021
Merged

[Tracing] Support Jaeger tracing in NCM and TC #707

merged 136 commits into from
Dec 10, 2021

Conversation

zzxgzgz
Copy link
Contributor

@zzxgzgz zzxgzgz commented Dec 1, 2021 •
edited
Loading

What does this PR do:

  1. Added Jaeger tracing to NCM's gRPC server/client, so that we can use Jaeger to trace NCM's gRPC activities.
  2. Added Jaeger tracing to the Test Controller, to make the tracing more complete.
  3. Closes [NCM] NCM Not Putting VPC States into Cache during PushGoalStatesStream #708

Please refer to the following screenshot to get an idea of what to expect, when running Test Controller:
GoalState Pushdown process:
image

On-demand process:
image
The on-demand tracing was captured before merging #704 , after merging this PR, on-demand doesn't get triggered, as neigbhor states all push down before the pings.

Future improvements:

  1. Improve how the spans are organized when on-demand is triggered, the RequestGoalStates spans show be at the same level, but right now they are chained together. For the reason behind it and a possible solution, please refer to this issue for a possible solution.
  2. Jaeger tracing is also implemented in ACA, but the spans between ACA and NCM doesn't get connected, so the ACA's spans look like this:
    image
    we suspect that this is because we are using different libraries here. In ACA, jaeger tracing was adding using the opentelemetry-cpp library, however, NCM uses the opentracing library. We suspect this mismatch is causing the ACA spans not connected with the NCM spans. We may want to change to opentelemetry library in Alcor later.

@zzxgzgz
Tried to make TC's create container process concurrent
47caabf
@zzxgzgz
Added back background ping
6a29451
@zzxgzgz
Changed fixed time sleep to a library(await) that waits until GSs are…
6adc8e0 
… sent and programmed.
@zzxgzgz
Try to add code to send GSV1 to ACA for testing
fbe57f1
@zzxgzgz
Try to add code to send GSV1 to ACA for testing
62992b3
@zzxgzgz
Sending only gsv1, to see if it works
f4df134
@zzxgzgz
Sending only gsv1, to see if it works
3402d92
@zzxgzgz
Sending only gsv1, to see if it works
1378f7a
@zzxgzgz
Enable gsv2 sending for testing.
25507e5
@zzxgzgz
Enable gsv2 sending for testing.
8918995
@zzxgzgz
Enable gsv2 sending for testing.
5fdc054
@zzxgzgz
Enable gsv2 sending for testing.
1d4d1c0
@zzxgzgz
Test sending v1 and v2 together
88d18bc
@zzxgzgz
Comment out sending GSV1 code in TC for on-demand performance testing
210ccda
@zzxgzgz
Start to change TC, and try to figure out why two hosts have differen…
c146da5 
…t amount of ports
@zzxgzgz
Try to make port amount balanced
c2aaab5
@zzxgzgz
Try to add support for different number of ports on two hosts
32be97d
@zzxgzgz
Try to add support for different number of ports on two hosts
d2c5b50
@zzxgzgz
Try to add support for different number of ports on two hosts
556217a
@zzxgzgz
Try to add support for different number of ports on two hosts
e51bc76
@zzxgzgz
Try to add support for different number of ports on two hosts
50a13b4
@zzxgzgz
Disabling container creating and pings, to focus on testing ACA's abi…
4afbbe3 
…lity to handle neighbor states
@zzxgzgz
Disabling gsv2 printout, also added time for waiting response of gRPC…
cf4f34f 
… calls
@zzxgzgz
Tried to add router states in TC
cb97d94
@zzxgzgz
Fixed set/add function mismatches
f959c07
@zzxgzgz
Added setRevisionNumber for router configuration
a17ae63
@zzxgzgz
Added gateway info in the subnet state
05c17ad
@zzxgzgz
Make host_vdr_mac_address consistent for routers on the same host
5358b73
@zzxgzgz
Try to make neighbors L3, and added routers to both hosts' GSs
53f2d9f
@zzxgzgz
Added another subnet, so there are now two subnets
3b82784
@zzxgzgz zzxgzgz self-assigned this Dec 1, 2021
@zzxgzgz zzxgzgz added the enhancement New feature or request label Dec 1, 2021
@lgtm-com
Copy link

lgtm-com bot commented Dec 1, 2021

This pull request introduces 2 alerts when merging 4c99f65 into a843622 - view on LGTM.com

new alerts:

  • 2 for Dereferenced variable may be null

@zzxgzgz zzxgzgz changed the title Added Jaeger tracing to NCM's gRPC server/client and to the Test Controller Added Jaeger tracing to NCM's gRPC server/client and to the Test Controller; Put VPC States into Cache during pushGoalStatesStream Dec 4, 2021
@xieus xieus added this to the Version 1.0.2021.12.30 milestone Dec 4, 2021
This was referenced Dec 6, 2021
Open
Open
@xieus
Copy link
Contributor

xieus commented Dec 7, 2021

@cj-chung Could you review this PR?

@cj-chung
Copy link
Contributor

cj-chung commented Dec 7, 2021

@cj-chung Could you review this PR?

sure

xieus
xieus reviewed Dec 10, 2021
View reviewed changes
Copy link
Contributor

@xieus xieus left a comment

Choose a reason for hiding this comment

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

A few comments.

services/network_config_manager/pom.xml Show resolved Hide resolved
...ger/src/main/java/com/futurewei/alcor/netwconfigmanager/client/gRPC/GoalStateClientImpl.java Show resolved Hide resolved
.../main/java/com/futurewei/alcor/netwconfigmanager/server/grpc/GoalStateProvisionerServer.java Show resolved Hide resolved
.../main/java/com/futurewei/alcor/netwconfigmanager/server/grpc/GoalStateProvisionerServer.java Outdated Show resolved Hide resolved
xieus
xieus approved these changes Dec 10, 2021
View reviewed changes
Copy link
Contributor

@xieus xieus left a comment

Choose a reason for hiding this comment

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

LGTM.

@xieus xieus changed the title Added Jaeger tracing to NCM's gRPC server/client and to the Test Controller; Put VPC States into Cache during pushGoalStatesStream [Tracing] Support Jaeger tracing in NCM and TC Dec 10, 2021
@xieus xieus merged commit 42a6edc into futurewei-cloud:master Dec 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xieus xieus xieus approved these changes

@cj-chung cj-chung Awaiting requested review from cj-chung

@DavidLiu506 DavidLiu506 Awaiting requested review from DavidLiu506

Assignees

@zzxgzgz zzxgzgz

Labels
enhancement New feature or request
Projects
None yet
Milestone
Version 1.0.2022.01.30
Development

Successfully merging this pull request may close these issues.

[NCM] NCM Not Putting VPC States into Cache during PushGoalStatesStream
3 participants
@zzxgzgz @xieus @cj-chung