----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12810/#review23649 -----------------------------------------------------------
patches/systemvm/debian/config/root/dnsmasq.sh <https://reviews.apache.org/r/12810/#comment47548> You don't need so complicate way to find dns is enabled or not. Just check /var/cache/cloud/cmdline for entry " dns1", " dns2"(remember add leading space to avoid ip6dns) And if cmdline contained "useextdns=true", then dns1/dns2 should be used directly in such case. You don't need to check eth0 ip is dns or not. In fact I don't think you need to add dhcp-option=tag:xxx,6,xxx for dns server of every range you added. You should able to use the original dhcp-option=6,xxx to cover all the cases. It should by default cover everything. patches/systemvm/debian/config/root/dnsmasq.sh <https://reviews.apache.org/r/12810/#comment47547> You should use "logger -t cloud xxxx" if you want to have any log for script. See other scripts for example. patches/systemvm/debian/config/root/dnsmasq.sh <https://reviews.apache.org/r/12810/#comment47552> You'd better add log here, using "logger -t cloud", say you're clearing the old entries. patches/systemvm/debian/config/root/dnsmasq.sh <https://reviews.apache.org/r/12810/#comment47551> I don't know dhcp-option has <set> keyword. I thought you meant dhcp-range but it's already cleared. So what here for? patches/systemvm/debian/config/root/dnsmasq.sh <https://reviews.apache.org/r/12810/#comment47556> I think we can do it better by create an separate configuration file in e.g. /etc/dnsmasq.d/multiple_ranges.conf for your dhcp-range and related dhcp-option changed. The first range created by cloud-early-config can remain in dnsmasq.conf until DnsMasqConfig command clear it and setup separate ranges in the separate configuration file. You can check vpc_guestnw.sh for reference. Do remember to clean the multiple_ranges.conf in cloud-early-config so it would be cleaned every time when VR reboot. patches/systemvm/debian/config/root/dnsmasq.sh <https://reviews.apache.org/r/12810/#comment47553> For every change you added to dnsmasq.conf, please add log. It would be easier to tell what's wrong in product. patches/systemvm/debian/config/root/dnsmasq.sh <https://reviews.apache.org/r/12810/#comment47559> Need logger -t rather than echo. patches/systemvm/debian/config/root/dnsmasq.sh <https://reviews.apache.org/r/12810/#comment47557> This is wrong. It cannot fail silently. The return result should be $result, rather than $?. should be: unlock_exit $result $lock $locked patches/systemvm/debian/config/root/dnsmasq.sh <https://reviews.apache.org/r/12810/#comment47558> Should be $result too. - Sheng Yang On July 22, 2013, 12:54 p.m., bharat kumar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12810/ > ----------------------------------------------------------- > > (Updated July 22, 2013, 12:54 p.m.) > > > Review request for cloudstack, Alena Prokharchyk and Sheng Yang. > > > Bugs: CLOUDSTACK-3694 > > > Repository: cloudstack-git > > > Description > ------- > > https://issues.apache.org/jira/browse/CLOUDSTACK-3694 > > wrote the dnsmasq config in bash instead of creating the config file in java > and overwriting. > > > Diffs > ----- > > core/src/com/cloud/agent/api/routing/DnsMasqConfigCommand.java 521ad70 > > core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java > 0b26220 > core/src/com/cloud/network/DnsMasqConfigurator.java 3fc61df > patches/systemvm/debian/config/root/createIpAlias.sh 5498195 > patches/systemvm/debian/config/root/dnsmasq.sh b70e2d3 > > plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java > c7f487e > > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java > f80d4b6 > scripts/vm/hypervisor/xenserver/vmops f8c0253 > server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java > 6c0f7a1 > > Diff: https://reviews.apache.org/r/12810/diff/ > > > Testing > ------- > > Tested on old master using xenserver. > Could not test on the latest one as it is broken. > > > Thanks, > > bharat kumar > >
