Saksham Please respond to review comments with updated patch
Animesh > -----Original Message----- > From: Murali Reddy [mailto:nore...@reviews.apache.org] On Behalf Of Murali > Reddy > Sent: Sunday, February 10, 2013 11:22 PM > To: Murali Reddy; Chiradeep Vittal > Cc: Saksham Srivastava; cloudstack > Subject: Re: Review Request: IP Address Reservation within a Network > > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9180/#review16414 > ----------------------------------------------------------- > > > > api/src/com/cloud/network/Network.java > <https://reviews.apache.org/r/9180/#comment34904> > > Please add comment that explaining what is getGuestCidr is for. > > Keep it along with getCidr() and put a comment for getCidr, explaining > what > it return in case of IP reservation is in place > > > > api/src/com/cloud/network/NetworkProfile.java > <https://reviews.apache.org/r/9180/#comment34911> > > put related fields, setters, initialisation code together > > i see same issue in multiple places > > > > api/src/org/apache/cloudstack/api/response/NetworkResponse.java > <https://reviews.apache.org/r/9180/#comment34905> > > keep guestCidr, cidr, reservedIprange to gether > > > > api/src/org/apache/cloudstack/api/response/NetworkResponse.java > <https://reviews.apache.org/r/9180/#comment34906> > > comment for both guestCidr, cidr are not good enough for one to understand > the difference and relation between them > > > > server/src/com/cloud/network/NetworkServiceImpl.java > <https://reviews.apache.org/r/9180/#comment34907> > > add comment explaining logic for both cases updating already configured > value and new value bing configured > > > > server/src/com/cloud/network/NetworkServiceImpl.java > <https://reviews.apache.org/r/9180/#comment34908> > > exception message is wrong > > you can mention it as guestVmcidr, need to be subset of gestCidr > > > > server/src/com/cloud/network/NetworkServiceImpl.java > <https://reviews.apache.org/r/9180/#comment34898> > > indentation is not correct > > > > server/src/com/cloud/network/NetworkServiceImpl.java > <https://reviews.apache.org/r/9180/#comment34909> > > same here, exception message is not correct > > > > server/src/com/cloud/network/NetworkServiceImpl.java > <https://reviews.apache.org/r/9180/#comment34899> > > put this block under, previous if (guestVmCidr!= null ) {} block > > > > setup/db/create-schema.sql > <https://reviews.apache.org/r/9180/#comment34901> > > this no longer acting as 'network cidr'? this you are using it as cidr > from > which VM's get the IP right? Add right comment, also mention that this will be > subset or equal to 'guest_cidr' > > > > setup/db/create-schema.sql > <https://reviews.apache.org/r/9180/#comment34902> > > terminology is confusing, cidr, and guest_cidr which does what. Please add > better comments or give intuitive names. > > Can we use 'guest_vm_cidr' to represent the CIDR from which VM's get the > IP allocated. > > and 'network_cidr' to represent the CIDR of the network, that is > inclusive of > guest_vm_cidr and IP address that can be allocated out side cloudstack > > > > setup/db/create-schema.sql > <https://reviews.apache.org/r/9180/#comment34903> > > terminology is confusing. cidr, and guest_cidr which does what? Please add > better comments or give intuitive names. > > Can we use 'guest_vm_cidr' to represent the CIDR from which VM's get the > IP allocated. > > and 'network_cidr' to represent the CIDR of the network, that is > inclusive of > guest_vm_cidr and IP address that can be allocated out side cloudstack > > > > setup/db/db/schema-40to410.sql > <https://reviews.apache.org/r/9180/#comment34900> > > you should be using 4.1 to 4.2 upgrade script > > > - Murali Reddy > > > On Feb. 8, 2013, 9:39 a.m., Saksham Srivastava wrote: > > > > ----------------------------------------------------------- > > This is an automatically generated e-mail. To reply, visit: > > https://reviews.apache.org/r/9180/ > > ----------------------------------------------------------- > > > > (Updated Feb. 8, 2013, 9:39 a.m.) > > > > > > Review request for cloudstack, Murali Reddy and Chiradeep Vittal. > > > > > > Description > > ------- > > > > CLOUDSTACK-705 IP Address reservation for Isolated Guest Networks > > > > > > CloudStack uses Guest CIDR for dhcp-range for the Guest VMs. The entire > CIDR is used by CloudStack for assigning IPs to Guest VMs. > > IP Address Reservation will allow part of address space to be used for non > CloudStack hosts/physical servers also, by restricting the address space of > CloudStack Guest VMs. > > Reservation can be configured using update Network API by specifying > guestvmCidr as an additional parameter. > > Reservation will be applicable for Isolated Guest Networks including VPC. > > reservediprange in the response will return the IP range that can be used > > for > non Cloudstack hosts. > > > > > > This addresses bug CLOUDSTACK-705. > > > > > > Diffs > > ----- > > > > api/src/com/cloud/network/Network.java 2bf7b7f > > api/src/com/cloud/network/NetworkProfile.java 37d46ac > > api/src/com/cloud/network/NetworkService.java ace1bb6 > > api/src/com/cloud/network/vpc/VpcService.java cc66b58 > > api/src/org/apache/cloudstack/api/ApiConstants.java d29408e > > > api/src/org/apache/cloudstack/api/command/user/network/UpdateNetworkC > md.java 6777407 > > api/src/org/apache/cloudstack/api/response/NetworkResponse.java > 7b29efb > > server/src/com/cloud/api/ApiResponseHelper.java 8c97615 > > server/src/com/cloud/network/NetworkServiceImpl.java d38d1f8 > > server/src/com/cloud/network/dao/NetworkVO.java d51a065 > > server/src/com/cloud/network/vpc/VpcManagerImpl.java 7197c36 > > server/test/com/cloud/network/MockNetworkManagerImpl.java 4a24f9a > > server/test/com/cloud/vpc/MockNetworkManagerImpl.java bcaaa26 > > server/test/com/cloud/vpc/MockVpcManagerImpl.java 0a44a49 > > setup/db/create-schema.sql f89c885 > > setup/db/db/schema-40to410.sql d771a15 > > > > Diff: https://reviews.apache.org/r/9180/diff/ > > > > > > Testing > > ------- > > > > Tested manually the following scenarios: > > Applying reservation when there are running VMs inside the guest_vm_cidr. > > Applying reservation when there are running VMs outside the > > guest_vm_cidr.(not allowed) Applying reservation when external device like > Netscaler is configured in the guest_cidr. > > Applying reservation in VPC tiers. > > Applying reservation outside the range of guest_cidr.(not allowed) > > > > > > Thanks, > > > > Saksham Srivastava > > > >
