incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Animesh Chaturvedi <animesh.chaturv...@citrix.com>
Subject RE: Review Request: IP Address Reservation within a Network
Date Thu, 14 Feb 2013 03:35:09 GMT
Saksham 

Please respond to review comments with updated patch

Animesh

> -----Original Message-----
> From: Murali Reddy [mailto:noreply@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
> >
> >

Mime
View raw message