cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Saksham Srivastava" <saksham.srivast...@citrix.com>
Subject Re: Review Request: IP Address Reservation within a Network
Date Tue, 19 Feb 2013 12:58:43 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message