You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@cloudstack.apache.org by Bill Rich <bi...@gmail.com> on 2012/12/07 18:21:35 UTC

Review Request: Bugfix CLOUDSTACK-591: Fixed bridge name parsing in security_group.py to support bridge names that include dashes

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8406/
-----------------------------------------------------------

Review request for cloudstack.

Description
-------

When checking for rebooted VMs, security_group.py assumed bridge names would not include dashed. The original code split the name found in iptables and used the 2nd member of the returned array. In a case where the bridge had a hyphen in the name, only a partial name was returned. For example, with a bridge named br-public, the line 'iptables-save |grep physdev-is-bridged |grep FORWARD |grep BF |grep '\-o' |awk '{print $9}'' returns BF-br-public. The name is parsed by splitting the name by hyphens and taking the 2nd member of the array. This returns br resulting in the script attempting to modify chains 'BF-br-IN' and 'BF-br-OUT' which don't exist.

I changed the code to use regular expressions to remove "^BF-" from the chain name. This will consistently remove just the unwanted part of the chain name to get the standard brname used throughout the rest of the script.

This addresses bug CLOUDSTACK-591.

Diffs
-----

scripts/vm/network/security_group.py b079890

Diff: https://reviews.apache.org/r/8406/diff/

Testing
-------

-Modified /var/run/cloud/<VM>.log on hv to include a different ID.
-Waited for security_group.py get_rule_logs_for_vms to be run.
-Checked /var/log/cloud/security_group.log for errors and that the correct information was being parsed
-Confirmed network connectivity for VM

Thanks,

Bill Rich

Re: Review Request: Bugfix CLOUDSTACK-591: Fixed bridge name parsing in security_group.py to support bridge names that include dashes

Posted by Rohit Yadav <ro...@citrix.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8406/#review14472
-----------------------------------------------------------

Ship it!


Thanks Bill, thank you for your patch. 
Applied on master; (pl. test and let me know if this works, I cleaned up some code, see the commit after yours)

commit 1ae2d720a3ca3bde2baacb0d06610e7466f95bfe
Author: Bill Rich <bi...@gmail.com>
Date:   Fri Dec 7 08:39:13 2012 -0800

    CLOUDSTACK-591: Changed bridge name parsing in security_group.py to support bridges named with dashes

I removed the list pop-ing which makes no sense and instead of getting and splitting on newline and take the first element from the list, I fixed the execute statement to take the first from the list, using head -1:

commit 6f29317a8492008d37c7eb0770f0cee650c14c40
Author: Rohit Yadav <bh...@apache.org>
Date:   Thu Dec 13 15:26:05 2012 -0800

    CLOUDSTACK-591: Fix execute and string processing logic for reboot_vm in security_group
    
    - Since we're always getting the first from the list, use head -1 to get the first
      of the results instead of processing again
    - Remove unecessay pop (why was it even there)
    
    Signed-off-by: Rohit Yadav <bh...@apache.org>

- Rohit Yadav


On Dec. 7, 2012, 5:21 p.m., Bill Rich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8406/
> -----------------------------------------------------------
> 
> (Updated Dec. 7, 2012, 5:21 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Description
> -------
> 
> When checking for rebooted VMs, security_group.py assumed bridge names would not include dashed. The original code split the name found in iptables and used the 2nd member of the returned array. In a case where the bridge had a hyphen in the name, only a partial name was returned. For example, with a bridge named br-public, the line 'iptables-save  |grep physdev-is-bridged |grep FORWARD |grep BF |grep '\-o' |awk '{print $9}'' returns BF-br-public. The name is parsed by splitting the name by hyphens and taking the 2nd member of the array. This returns br resulting in the script attempting to modify chains 'BF-br-IN' and 'BF-br-OUT' which don't exist.
> 
> I changed the code to use regular expressions to remove "^BF-" from the chain name. This will consistently remove just the unwanted part of the chain name to get the standard brname used throughout the rest of the script.
> 
> 
> This addresses bug CLOUDSTACK-591.
> 
> 
> Diffs
> -----
> 
>   scripts/vm/network/security_group.py b079890 
> 
> Diff: https://reviews.apache.org/r/8406/diff/
> 
> 
> Testing
> -------
> 
> -Modified /var/run/cloud/<VM>.log on hv to include a different ID. 
> -Waited for security_group.py get_rule_logs_for_vms to be run.
> -Checked /var/log/cloud/security_group.log for errors and that the correct information was being parsed
> -Confirmed network connectivity for VM
> 
> 
> Thanks,
> 
> Bill Rich
> 
>

Re: Review Request: Bugfix CLOUDSTACK-591: Fixed bridge name parsing in security_group.py to support bridge names that include dashes

Posted by Rohit Yadav <ro...@citrix.com>.

Thanks Bill applied on master, pl. close the review and help verify if that works now for you.

Regards.

On 13-Dec-2012, at 3:27 PM, Bill Rich <bi...@gmail.com>> wrote:

Cool. Thank you!

On Thu, Dec 13, 2012 at 3:16 PM, Rohit Yadav <ro...@citrix.com>> wrote:
Hi Bill,

No need to wait now, people had their chances :)
I'll commit that right away, may be remove some bloat as well.

Regards.

On 13-Dec-2012, at 2:39 PM, Bill Rich <bi...@gmail.com>> wrote:

Hi Rohit,

Would you still like to wait to give others a chance to review?

Bill

On Fri, Dec 7, 2012 at 9:56 AM, Rohit Yadav <ro...@citrix.com>> wrote:
This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8406/

Looks good, if ^BR is not found, it would skip replacing. tsp, anyone else want to take a look?

- Rohit

On December 7th, 2012, 5:21 p.m., Bill Rich wrote:

Review request for cloudstack.
By Bill Rich.

Updated Dec. 7, 2012, 5:21 p.m.

Description

Testing

Bugs: CLOUDSTACK-591
Diffs

* scripts/vm/network/security_group.py (b079890)

View Diff<https://reviews.apache.org/r/8406/diff/>

Re: Review Request: Bugfix CLOUDSTACK-591: Fixed bridge name parsing in security_group.py to support bridge names that include dashes

Posted by Rohit Yadav <ro...@citrix.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8406/#review14164
-----------------------------------------------------------


Looks good, if ^BR is not found, it would skip replacing. tsp, anyone else want to take a look? 

- Rohit Yadav


On Dec. 7, 2012, 5:21 p.m., Bill Rich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8406/
> -----------------------------------------------------------
> 
> (Updated Dec. 7, 2012, 5:21 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Description
> -------
> 
> When checking for rebooted VMs, security_group.py assumed bridge names would not include dashed. The original code split the name found in iptables and used the 2nd member of the returned array. In a case where the bridge had a hyphen in the name, only a partial name was returned. For example, with a bridge named br-public, the line 'iptables-save  |grep physdev-is-bridged |grep FORWARD |grep BF |grep '\-o' |awk '{print $9}'' returns BF-br-public. The name is parsed by splitting the name by hyphens and taking the 2nd member of the array. This returns br resulting in the script attempting to modify chains 'BF-br-IN' and 'BF-br-OUT' which don't exist.
> 
> I changed the code to use regular expressions to remove "^BF-" from the chain name. This will consistently remove just the unwanted part of the chain name to get the standard brname used throughout the rest of the script.
> 
> 
> This addresses bug CLOUDSTACK-591.
> 
> 
> Diffs
> -----
> 
>   scripts/vm/network/security_group.py b079890 
> 
> Diff: https://reviews.apache.org/r/8406/diff/
> 
> 
> Testing
> -------
> 
> -Modified /var/run/cloud/<VM>.log on hv to include a different ID. 
> -Waited for security_group.py get_rule_logs_for_vms to be run.
> -Checked /var/log/cloud/security_group.log for errors and that the correct information was being parsed
> -Confirmed network connectivity for VM
> 
> 
> Thanks,
> 
> Bill Rich
> 
>