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

[Data-Plane Mgr] Add Local Cache Support to DPM #486

Merged
merged 4 commits into from
Dec 2, 2020
Merged

[Data-Plane Mgr] Add Local Cache Support to DPM #486

merged 4 commits into from
Dec 2, 2020

Conversation

chenpiaoping
Copy link
Contributor

@chenpiaoping chenpiaoping commented Nov 26, 2020 •
edited
Loading

  1. When creating a port, save the basic information of all port in all subnet to the local cache of dpm.
  2. When creating router configuration get the basic information of port from the local cache of dpm according to subnet id.
  3. Rename DataPlane to Dpm: DataPlaneController--->DpmController, DataPlaneService--->DpmService.
  4. Split DpmService into multiple files according to resource type.
  5. Modify the content of dpm processing result.

@chenpiaoping chenpiaoping requested a review from xieus November 26, 2020 11:12
xieus
xieus reviewed Nov 27, 2020
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.

@chenpiaoping Do we have the logics to use the cache in case of cache hit and cache miss?

Left a few minor comments. The rest looks pretty good to me.

...s/data_plane_manager/src/main/java/com/futurewei/alcor/dataplane/client/DataPlaneClient.java
Comment on lines -33 to -35
List<Map<String, List<GoalStateOperationStatus>>> updateGoalStates(List<UnicastGoalState> unicastGoalStates) throws Exception;

List<Map<String, List<GoalStateOperationStatus>>> deleteGoalStates(List<UnicastGoalState> unicastGoalStates) throws Exception;
Copy link
Contributor

Choose a reason for hiding this comment

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

A quick question: why do we remove these interfaces?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it is okay to keep the interfaces which reminds us that we need to implement in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about adding it when I needed it.

...a_plane_manager/src/main/java/com/futurewei/alcor/dataplane/service/ovs/ResourceService.java Outdated

import com.futurewei.alcor.schema.Common.OperationType;

public class ResourceService {
Copy link
Contributor

Choose a reason for hiding this comment

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

ResourceService class could be moved on layer up to com.futurewei.alcor.dataplane.service;

...ces/data_plane_manager/src/main/java/com/futurewei/alcor/dataplane/cache/LocalCacheImpl.java
for (InternalPortEntity portEntity: portEntities) {
List<PortEntity.FixedIp> fixedIps = portEntity.getFixedIps();
if (fixedIps == null) {
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to log this unusual portEntity (at least on the DEBUG level) as this is an unexpected scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

...ces/data_plane_manager/src/main/java/com/futurewei/alcor/dataplane/cache/LocalCacheImpl.java Outdated
subnetPorts.setGatewayPortMac(subnetEntity.getGatewayMacAddress());
subnetPorts.setGatewayPortIp(subnetEntity.getGatewayIp());

//FIXME: get the gateway port id
Copy link
Contributor

Choose a reason for hiding this comment

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

GW port could be retrieved from SubnetEntity I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll check it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@chenpiaoping
Copy link
Contributor Author

@chenpiaoping Do we have the logics to use the cache in case of cache hit and cache miss?
Not yet.

@xieus
Copy link
Contributor

xieus commented Dec 2, 2020

@chenpiaoping Let us get back on this PR. It has been a while.

@xieus
Copy link
Contributor

xieus commented Dec 2, 2020

BTW #468 is merged to master. After the cache is in, we can start testing E2E.

@chenpiaoping
Copy link
Contributor Author

chenpiaoping commented Dec 2, 2020 •
edited
Loading

@chenpiaoping Let us get back on this PR. It has been a while.

OK,there seems to be a lot of conflict.

@chenpiaoping
Copy link
Contributor Author

@chenpiaoping Do we have the logics to use the cache in case of cache hit and cache miss?
Not yet.

Under what circumstances will miss occur?

@xieus
Copy link
Contributor

xieus commented Dec 2, 2020

@chenpiaoping Let us get back on this PR. It has been a while.

OK,there seems to be a lot of conflict.

yeah, DPM is very popular these days :-)

@xieus
Copy link
Contributor

xieus commented Dec 2, 2020 •
edited
Loading

@chenpiaoping Do we have the logics to use the cache in case of cache hit and cache miss?
Not yet.

Under what circumstances will miss occur?

In an ideal situation, the port creation should always reach DPM earlier than RM reaches DPM for routing rule update. We can imagine that in some rare (but possible) race condition, the second call could reach DPM earlier than the first call.

@xieus xieus self-requested a review December 2, 2020 13:01
@xieus xieus added enhancement New feature or request feature feature development labels Dec 2, 2020
@xieus xieus added this to the Version 1.0.2020.11.30 milestone Dec 2, 2020
xieus
xieus approved these changes Dec 2, 2020
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.

Overall LGTM. Left a comment on one place that might have potential issue. Please take a look to see if we could further optimize shortly. @chenpiaoping

...e_manager/src/main/java/com/futurewei/alcor/dataplane/client/pulsar/DataPlaneClientImpl.java
public List<Map<String, List<Goalstateprovisioner.GoalStateOperationReply.GoalStateOperationStatus>>> createGoalStates(List<UnicastGoalState> unicastGoalStates) throws Exception {
public List<String> createGoalStates(List<UnicastGoalState> unicastGoalStates) throws Exception {
List<String> failedHosts = new ArrayList<>();

for (UnicastGoalState unicastGoalState: unicastGoalStates) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the case of large number of neighbors, this for-loop could slow down potentially. See if we can optimize this part. @chenpiaoping

@xieus xieus changed the title Add local cache for DPM [Data-Plane Mgr] Add Local Cache Support to DPM Dec 2, 2020
@xieus xieus merged commit 8ad0c2f into futurewei-cloud:master Dec 2, 2020
@xieus xieus mentioned this pull request Dec 3, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xieus xieus xieus approved these changes

Assignees

@chenpiaoping chenpiaoping

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

Successfully merging this pull request may close these issues.
2 participants
@chenpiaoping @xieus