-
Notifications
You must be signed in to change notification settings - Fork 34
Alcor DVR Host Design and Protobuf Schema Change #339
Conversation
…into design/routing
…into design/routing
…into design/routing
…into design/routing
moving from er1cthe0ne branch design/routing to er1cthe0ne master
xieus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some early feedback to the proto change.
| PORT = 2; | ||
| SECURITYGROUP = 3; | ||
| DHCP = 4; | ||
| NEIGHBOR = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resource type is a type for customer-defined resources. Could you elaborate why neighbor is a type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GoalStateOperationStatus includes ResourceType for GoalStateOperationReply. Since we introduced NeighborState in the schema, we need to add NEIGHBOR as ResourceType for our GoalStateOperationReply.
| int64 tunnel_id = 8; | ||
| string request_id = 3; | ||
| string id = 4; | ||
| NetworkType network_type = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move network_type from port directly to network, instead of subnet? Do you plan to support two subnets in the same network with different values of network_type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
putting it in subnet level will allow us to support:
- two subnets in the same network with different network_type
- each network with n subnet will support one network_type only
@cj-chung can you help to find out what openstack neutron supports?
Note that we have the notion to look up subnet info, but we don't have the notion to look up vpc info (yet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neutron by default supports one network, one network type, and one segmentation id.
Ref: https://docs.openstack.org/api-ref/network/v2/?expanded=create-network-detail#create-network
Regarding two subnets with different network_type, DP will be pretty challenging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it supports one network with multiple segments, each with different network type and segmentation id.
"network": {
"segments": [
{
"provider:segmentation_id": 2,
"provider:physical_network": "public",
"provider:network_type": "vlan"
},
{
"provider:physical_network": "default",
"provider:network_type": "flat"
}
],
"name": "net1",
"admin_state_up": true,
"qos_policy_id": "6a8454ade84346f59e8d40665f878b2e"
}
}
xieus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Signed off and merged to unblock the ACA PR futurewei-cloud/alcor-control-agent#119.
haboy52581
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comments about neighbor and dhcp
| // TODO: change to uint32 but that would require change in DPM | ||
| uint64 tunnel_id = 10; | ||
|
|
||
| message Gateway { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not find Gateway definition in subnet proto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I have updated DPM's input about subnet Internal Entity part here, let me know if you have any questions:
https://github.com/haboy52581/alcor/blob/master/docs/modules/ROOT/pages/infra_services/data_plane_manager.adoc#api-specification
| limitations under the License. | ||
| */ | ||
|
|
||
| syntax = "proto3"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we put the neighbor to port as one field which is more naturally?
| string ipv4_address = 4; | ||
| string ipv6_address = 5; | ||
| string port_host_name = 6; // for local DNS response | ||
| string request_id = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need one called Id?
| FINALIZE = 9; // For Mizar only | ||
| CREATE_UPDATE_SWITCH = 10; // For Mizar only | ||
| CREATE_UPDATE_ROUTER = 11; // For Mizar only | ||
| NEIGHBOR_CREATE_UPDATE = 5; // To be removed after using NeighborState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we put neighbor into port as field, then we do NOT need NEIGHBORXXX type and logic to handle that
This PR includes: