> On Feb. 11, 2013, 7:22 a.m., Murali Reddy wrote: > > api/src/com/cloud/network/NetworkProfile.java, line 71 > > <https://reviews.apache.org/r/9180/diff/4/?file=257005#file257005line71> > > > > put related fields, setters, initialisation code together > > > > i see same issue in multiple places
This happened due to merge conflicts with IPv6. I have changed the order now. > On Feb. 11, 2013, 7:22 a.m., Murali Reddy wrote: > > api/src/com/cloud/network/Network.java, line 293 > > <https://reviews.apache.org/r/9180/diff/4/?file=257004#file257004line293> > > > > 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 Changed getGuestCidr to getNetworkCidr, added comments for both. > On Feb. 11, 2013, 7:22 a.m., Murali Reddy wrote: > > api/src/org/apache/cloudstack/api/response/NetworkResponse.java, lines 49-50 > > <https://reviews.apache.org/r/9180/diff/4/?file=257010#file257010line49> > > > > keep guestCidr, cidr, reservedIprange to gether Done. > On Feb. 11, 2013, 7:22 a.m., Murali Reddy wrote: > > api/src/org/apache/cloudstack/api/response/NetworkResponse.java, line 59 > > <https://reviews.apache.org/r/9180/diff/4/?file=257010#file257010line59> > > > > comment for both guestCidr, cidr are not good enough for one to > > understand the difference and relation between them Added exhaustive comments for both. > On Feb. 11, 2013, 7:22 a.m., Murali Reddy wrote: > > server/src/com/cloud/network/NetworkServiceImpl.java, line 1666 > > <https://reviews.apache.org/r/9180/diff/4/?file=257012#file257012line1666> > > > > add comment explaining logic for both cases updating already configured > > value and new value bing configured Done. > On Feb. 11, 2013, 7:22 a.m., Murali Reddy wrote: > > server/src/com/cloud/network/NetworkServiceImpl.java, line 1667 > > <https://reviews.apache.org/r/9180/diff/4/?file=257012#file257012line1667> > > > > exception message is wrong > > > > you can mention it as guestVmcidr, need to be subset of gestCidr Changed the exception message. > On Feb. 11, 2013, 7:22 a.m., Murali Reddy wrote: > > server/src/com/cloud/network/NetworkServiceImpl.java, line 1669 > > <https://reviews.apache.org/r/9180/diff/4/?file=257012#file257012line1669> > > > > indentation is not correct Improved it. > On Feb. 11, 2013, 7:22 a.m., Murali Reddy wrote: > > server/src/com/cloud/network/NetworkServiceImpl.java, line 1671 > > <https://reviews.apache.org/r/9180/diff/4/?file=257012#file257012line1671> > > > > same here, exception message is not correct Changed. > On Feb. 11, 2013, 7:22 a.m., Murali Reddy wrote: > > server/src/com/cloud/network/NetworkServiceImpl.java, line 1678 > > <https://reviews.apache.org/r/9180/diff/4/?file=257012#file257012line1678> > > > > put this block under, previous if (guestVmCidr!= null ) {} block Done. > On Feb. 11, 2013, 7:22 a.m., Murali Reddy wrote: > > setup/db/create-schema.sql, line 249 > > <https://reviews.apache.org/r/9180/diff/4/?file=257018#file257018line249> > > > > 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' Added relevant comments for it. > On Feb. 11, 2013, 7:22 a.m., Murali Reddy wrote: > > setup/db/create-schema.sql, line 252 > > <https://reviews.apache.org/r/9180/diff/4/?file=257018#file257018line252> > > > > 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 network_cidr added in place of guest_cidr. Added intuitive comments. > On Feb. 11, 2013, 7:22 a.m., Murali Reddy wrote: > > setup/db/create-schema.sql, line 252 > > <https://reviews.apache.org/r/9180/diff/4/?file=257018#file257018line252> > > > > 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 network_cidr added in place of guest_cidr. Added intuitive comments. > On Feb. 11, 2013, 7:22 a.m., Murali Reddy wrote: > > setup/db/db/schema-40to410.sql, line 85 > > <https://reviews.apache.org/r/9180/diff/4/?file=257019#file257019line85> > > > > you should be using 4.1 to 4.2 upgrade script Changed it. - Saksham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9180/#review16414 ----------------------------------------------------------- On Feb. 19, 2013, 12:57 p.m., Saksham Srivastava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9180/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2013, 12:57 p.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 cd7d700 > > api/src/org/apache/cloudstack/api/command/user/network/UpdateNetworkCmd.java > 6777407 > api/src/org/apache/cloudstack/api/response/NetworkResponse.java 7b29efb > server/src/com/cloud/api/ApiResponseHelper.java a94e935 > server/src/com/cloud/network/NetworkServiceImpl.java 37b4903 > server/src/com/cloud/network/dao/NetworkVO.java d51a065 > server/src/com/cloud/network/vpc/VpcManagerImpl.java fbb5788 > server/src/com/cloud/upgrade/dao/Upgrade410to420.java a43727c > 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/db/schema-410to420.sql 65add75 > > 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 > >
