Hi Tim, Thanks for your review. I’ve fixed the tick-quote issue on the mentioned line. Please find the various issues addressed in this patch;
## 1. Issue with sm/util.py My concern with sm/util.py was that, previously on XS 6.2 the output of a typical util.pread2(<some args here>).split(‘\n’) would return a list like [a,b,c,d,”"] with the last element as an empty string possibly because an extra newline in the command output. Due to this all the code in vmops plugin followed this kind of patten where the programmer coded it to do a list.pop() after calling pread2().split(‘\n’). But in case of XS 6.5, I found that no newline was at the end and we should not assume it and doing a list.pop() would remove the last element which can be a valid string and not a newline (or empty string after calling split() on it). I traced this issue when I saw following methods fail in the /var/log/cloud/cloud.log and looked at the code: delete_rules_for_vm_in_bridge_firewall_chain destroy_ebtables_rules destroy_arptables_rules The fix I tried was to filter out empty and None elements from the list using a "filter(util.pread2(<some args>).split(‘\n’))” kind of pattern instead of doing a list.pop() which might remove a valid string. After this fix, I saw that SG rules applied correctly and the previous errors went away. I believe that this way of defensive coding would work on both XS 6.2 and 6.5, irrespective of the output having empty lines or not. ## 2. Issue with large CIDRs and ipset overflow The other issue I found was when I added an ingress or egress CIDR that was in the block /8 or /4. I found that ipset failed in the /var/log/cloud/cloud.log. On tracing the issue I found that ipset data structure has been modified to use iphash instead of iptreemap (done in bd6f03aa954d4b3e7ead7e8010c5674d5d1f9513) because recent versions of ipset (as in XS 6.5) did not have that data-structure. The issue with “iphash” in this case was that when someone added a CIDR or CIDRs, it would add individual IPs instead of the CIDR itself. Which failed for /8 or /4 because the default max length of “iphash” entry was 65535. The fix for this was to use “nethash” since we receive CIDRs only and it becomes memory/space efficient. Only the ipset entry for the VM itself is “iphash” based now (to store a VM’s primary and secondary IPs). ## 3. Issue with removing Ingress/Egress rules I saw that when an ingress or egress rule was removed while the iptable rules were removed/fixed, the ipset entry stayed there and cidrs in it were not getting removed. Every time I added or removed an ingress or egress entry, CloudStack would send the vmops plugin all the ingress and egress rules. This assumed that all add/remove operations were sort of idempotent and all information was applied every time we made any change. Therefore, as a pessimistic approach I added the fix to flush/remove the ipset entries before adding them back again (that the mgmt server sent to vmops plugin). This also solves an upgrade error for users who would upgrade to XS 6.5 or ACS 4.5, as previous ipset entries were of type “iptreemap” or “iphash”, but new entries (the ingress and egress ones) is of “nethash” type. ## Discuss - should we flush/remove ipset rules before applying new rules? Can you advise if we should have this pessimistic approach (as it flushes ipset entries when rules may be still in place?) or use an optimistic approach to keep the entries. There are two issues with the current optimistic approach: 1. When egress/ingress rules are removed, for a brief period (until new ipset entry replace the old one) old entries still persist. But since iptables rules are removed and no one is referencing them it is harmless. 2. In case of users upgrading, the old ipset data-structure is “iptreemap” or “iphash”, but the new one is “nethash”, how do we make sure that old ones are removed/deleted so new ones are of “nethash” type. > On 21-Apr-2015, at 7:51 pm, Tim Mackey <tmac...@gmail.com> wrote: > > Rohit, > > I just added a comment to update line 457 to tick-quote the vmchain as > you've done elsewhere. My main concern would be flushing the ipset while > the iptable entry still exists. > > I am curious what in sm/util.py concerned you. That's all storage > management code and should have nothing to do with security groups. I also > diffed a 6.5 and 6.2 version which didn't show anything obvious to explain > a security group issue. > > ipset definitely did change going from 4.5 to 6.11 to match our kernel > update. > > -tim > > On Tue, Apr 21, 2015 at 11:57 AM, Rohit Yadav <rohit.ya...@shapeblue.com> > wrote: > >> Hi all, >> >> We discovered that Security Groups don’t work in ACS 4.5+ when used with >> XenServer 6.5 due to ipset, sm/util.py changes. I’ve opened the issue here >> which was found to be reproducible by my colleagues Geoff and Abhi: >> https://issues.apache.org/jira/browse/CLOUDSTACK-8395 >> >> I’ve tried to fix it in a way such that vmops plugin would work on both XS >> 6.2 and 6.5 releases, here's the PR: >> https://github.com/apache/cloudstack/pull/186 >> >> One of the major changes it introduces it to use “nethash” instead of >> “iphash” when storing CIDRs received as part of a ingress/egress rule. I’m >> not sure how it will affect users that will upgrade to ACS 4.5, as a >> precaution I’ve added a change to flush and remove old ipset entry before >> adding a new one. (Assuming all network rule addition/removals are >> idempotent, as everytime we add/remove a rule, all rules are sent to be >> applied by the XS vmops plugins). >> >> Tim - since you’re one of the Xen gurus can you help review it and suggest >> any other changes? >> >> I wanted to bring this issue on dev ML since it’s a potential blocker for >> 4.5. I’m not sure if we officially support XS 6.5 on 4.4 branch, but if >> needed once we have a reviewed commit it can be cherry-picked on 4.4 as >> well. >> >> Regards, >> Rohit Yadav >> Software Architect, ShapeBlue >> M. +91 88 262 30892 | rohit.ya...@shapeblue.com >> Blog: bhaisaab.org | Twitter: @_bhaisaab >> >> >> >> Find out more about ShapeBlue and our range of CloudStack related services >> >> IaaS Cloud Design & Build< >> http://shapeblue.com/iaas-cloud-design-and-build//> >> CSForge – rapid IaaS deployment framework<http://shapeblue.com/csforge/> >> CloudStack Consulting<http://shapeblue.com/cloudstack-consultancy/> >> CloudStack Software Engineering< >> http://shapeblue.com/cloudstack-software-engineering/> >> CloudStack Infrastructure Support< >> http://shapeblue.com/cloudstack-infrastructure-support/> >> CloudStack Bootcamp Training Courses< >> http://shapeblue.com/cloudstack-training/> >> >> This email and any attachments to it may be confidential and are intended >> solely for the use of the individual to whom it is addressed. Any views or >> opinions expressed are solely those of the author and do not necessarily >> represent those of Shape Blue Ltd or related companies. If you are not the >> intended recipient of this email, you must neither take any action based >> upon its contents, nor copy or show it to anyone. Please contact the sender >> if you believe you have received this email in error. Shape Blue Ltd is a >> company incorporated in England & Wales. ShapeBlue Services India LLP is a >> company incorporated in India and is operated under license from Shape Blue >> Ltd. Shape Blue Brasil Consultoria Ltda is a company incorporated in Brasil >> and is operated under license from Shape Blue Ltd. ShapeBlue SA Pty Ltd is >> a company registered by The Republic of South Africa and is traded under >> license from Shape Blue Ltd. ShapeBlue is a registered trademark. >> Regards, Rohit Yadav Software Architect, ShapeBlue M. +91 88 262 30892 | rohit.ya...@shapeblue.com Blog: bhaisaab.org | Twitter: @_bhaisaab Find out more about ShapeBlue and our range of CloudStack related services IaaS Cloud Design & Build<http://shapeblue.com/iaas-cloud-design-and-build//> CSForge – rapid IaaS deployment framework<http://shapeblue.com/csforge/> CloudStack Consulting<http://shapeblue.com/cloudstack-consultancy/> CloudStack Software Engineering<http://shapeblue.com/cloudstack-software-engineering/> CloudStack Infrastructure Support<http://shapeblue.com/cloudstack-infrastructure-support/> CloudStack Bootcamp Training Courses<http://shapeblue.com/cloudstack-training/> This email and any attachments to it may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Shape Blue Ltd or related companies. If you are not the intended recipient of this email, you must neither take any action based upon its contents, nor copy or show it to anyone. Please contact the sender if you believe you have received this email in error. Shape Blue Ltd is a company incorporated in England & Wales. ShapeBlue Services India LLP is a company incorporated in India and is operated under license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is a company incorporated in Brasil and is operated under license from Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The Republic of South Africa and is traded under license from Shape Blue Ltd. ShapeBlue is a registered trademark.
