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

[Vpc/Subnet Manager] API Standardization, and Segment APIs #181

Merged
merged 128 commits into from
May 13, 2020

Conversation

kevin-zhonghao
Copy link
Contributor

@kevin-zhonghao kevin-zhonghao commented Apr 30, 2020

Code refactor and new features included in this PR:

  • Standardized VPC Manager and Subnet Manager with their properties and APIs
  • Add Segment module and new UTs
  • Add SegmentRange module and new UTs
  • Add NetworkVlanType, NetworkVxlanType, NetworkGreType entities in db
  • Implement Vlan , Vxlan and GRE module in Segment
  • Add design doc draft for VPC and Subnet manager
  • Add new APIs in route manager

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.

Some early feedback for the new iteration.

@JsonProperty("mtu")
private String mtu;

@JsonProperty("vlan_id")
Copy link
Contributor

Choose a reason for hiding this comment

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

This class is for VxLan, so the id should be "vxlan_id", instead of "vlan_id" :-)

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 think this property is used to store the vlan_id related to vxlan ~ let us talk about it

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we discussed this before.

@xieus
Copy link
Contributor

xieus commented May 4, 2020

Some API gateway UTs failed. See detailed here: https://jenkins.alkaidcloud.io/job/alcor-controller-pr-test/228/console

[�[1;34mINFO�[m] AlcorApiGateway .................................... �[1;31mFAILURE�[m [ 2.268 s]

We still need to fix the UT failure.

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.

Left a few more comments. Vlan allocation looks better!

docs/design/vpc_manager.adoc Show resolved Hide resolved
lib/pom.xml Outdated Show resolved Hide resolved
if (NetworkTypeEnum.VXLAN.getNetworkType().equals(networkType)) {
key = segmentService.addVxlanEntity(segmentWebRequestObject.getId());
} else if (NetworkTypeEnum.VLAN.getNetworkType().equals(networkType)) {
key = segmentService.addVlanEntity(segmentWebRequestObject.getId(), networkTypeId, networkType);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the parameter of networkType in the addVlanEntity method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

public VlanKeyRequest allocateVlan(VlanKeyRequest request) throws Exception;
public VlanKeyRequest releaseVlan(String networkType, String rangeId, Long key) throws Exception;
public VlanKeyRequest getVlan(String networkType, String rangeId, Long key) throws Exception;
public NetworkVlanRangeRequest createRange(NetworkVlanRangeRequest request) 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.

Ideally, the Segment Service could have a generic method name like addEntity(segmentId, segmentationId, networkType) so that it could have different implementation for various network types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it looks better in structure ~ but if we have that in SegmentService, we maybe not implement three different implementations in SegmentServiceImpl

Copy link
Contributor

Choose a reason for hiding this comment

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

Three implementation would look fantastic. why not? :-)

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 more comments, most of them are minor. There is one issue we need to pay attention to: NetworkVlanRange/NetworkVxlanRange shouldn't include segment id. The schema appears not right.

lib/pom.xml Outdated Show resolved Hide resolved

import java.util.Map;

public interface SegmentDatabaseService {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is another service interface called "SegmentService" but it also deals with database access, right? In this case, we should use names that match what it is exactly?

A renaming suggestion:
(1) SegmentDatabaseService => SegmentManagementService
(2) SegmentService => SegmentKeyManagementService

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's talk about it

@xieus xieus changed the title [Vpc/Subnet Manager] Networking API Standardization [Vpc/Subnet Manager] Networking API Standardization, and Segment APIs May 13, 2020
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. Thanks for the point-to-point response to the comments.

@xieus xieus changed the title [Vpc/Subnet Manager] Networking API Standardization, and Segment APIs [Vpc/Subnet Manager] API Standardization, and Segment APIs May 13, 2020
@xieus xieus merged commit f88a67f into futurewei-cloud:master May 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants