<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 21, 2015 at 3:22 PM, Ben Swartzlander <span dir="ltr"><<a href="mailto:ben@swartzlander.org" target="_blank">ben@swartzlander.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
  
    
  
  <div text="#000000" bgcolor="#FFFFFF">
    [Resending my response as unknown forces ate my original message]<span class=""><br>
    <br>
    <div>On 08/20/2015 08:30 AM, Bjorn Schuberg
      wrote:<br>
    </div>
    <blockquote type="cite">
      <div dir="ltr">
        <div>
          <div>
            <div>Hello everyone,<br>
              <br>
            </div>
            <div>this is my first thread on this mailing list, and I
              would like to take the opportunity to say that it was
              great to see you all at the midcycle, even if remote.<br>
            </div>
            <div><br>
            </div>
            <div>Now, to my question; I've been looking into an issue
              that arise when deleting access to a share, and then
              momentarily after, deleting the same share. The delete
              fails due to a race in <span>`<span style="font-family:monospace,monospace">_remove_share_access_rules</span>`
                in the `</span><span style="font-family:monospace,monospace">ShareManager</span>`,
              which attempts to delete all granted permissions on the
              share before removing it, but one of the access
              permissions is concurrently deleted due to the first API
              call, see;<br>
            </div>
            <a href="https://github.com/openstack/manila/blob/master/manila/share/manager.py#L600" target="_blank"><span>https://github.com/openstack/manila/blob/master/manila/share/manager.py#L600</span></a></div>
          <div><span></span></div>
          <div><span><br>
            </span></div>
          <span>I think an acceptable fix to this would be
            to wrap the `</span><span style="font-family:monospace,monospace">_deny_access</span>`
          call with a `<span style="font-family:monospace,monospace">try</span>`...
          `<span style="font-family:monospace,monospace">except</span>`
          block, and log any attempts to remove non-existing
          permissions. The problem is that there seems to be no contract
          on the exception to throw in case you attempt to delete an
          `access` which does not exist -- each driver behaves
          differently.<br>
        </div>
        <div><br>
        </div>
        <div>This got my attention after running the tempest integration
          tests, where the teardown <i>sometimes</i> fails in <span style="font-family:monospace,monospace">tempest.api.share.test_rules:ShareIpRulesForNFSTest</span>.<br>
          <br>
          Any thoughts on this? Perhaps there is a smoother approach
          that I'm not seeing.<br>
        </div>
      </div>
    </blockquote>
    <br></span>
    This is a good point. I'm actually interested in purusing a deeper
    overhaul of the allow/deny access logic for Mitaka which will make
    access rules less error prone in my opinion. I'm open to short term
    bug fixes in Liberty for problems like the one you mention, but I'm
    already planning a session in Tokyo about a new share access driver
    interface. The reason it has to wait until Mitaka is that all of the
    drivers will need to change their logic to accommodate the new
    method.<br>
    <br>
    My thinking on access rules is that the driver interface which adds
    and removes rules one at a time is too fragile, and assumes too much
    about what backends are capable of supporting. I see the following
    problems (in addition to the one you mention):<br>
    * If addition or deletion of a rules fails for any reason, the set
    of rules on the backend starts to differ from what the user intended
    and there is no way to go back and correct the problem.<br>
    * If backends aren't able to implement rules exactly as Manila
    expects, (perhaps a backend does not support nested subnets with
    identical rules) then there are situations where a certain set of
    user actions will be guaranteed to result in broken rules. Consider
    (1) add rw access to <a href="http://10.0.0.0/8" target="_blank">10.0.0.0/8</a> (2) Add rw access to <a href="http://10.10.0.0/16" target="_blank">10.10.0.0/16</a>
    (3) Remove rw access to <a href="http://10.0.0.0/8" target="_blank">10.0.0.0/8</a> (4) Try to access share from
    10.10.10.10. If step (2) fails because the backend ignored that rule
    (it was redundant at the time it was added) then step (4) will also
    fail, even though it shouldn't.<br>
    * The current mechanism doesn't allow making multiple changes
    atomically -- changes have to be sequential. This will case problems
    if we want to allow access rules to be defined externally (something
    which was discussed during Juno and is still desirable) because
    changes to access rules may come in batches.<br>
    <br>
    My proposal is simple. Rather than making drivers implement
    allow_access() and deny_access(), driver should implement a single
    set_access() which gets passed a list of all the access rules.
    Drivers would be required to compare the list of rules passed in
    from the manager to the list of rules on the storage controller and
    make changes as appropriate. For some drivers this would be more
    work but for other drivers it would be less work. Overall I think
    it's a better design. We can probably implement some kind of
    backwards compatibility to avoid breaking drivers during the
    migration to the new interface.<br></div></blockquote><div><br>I like the idea of a review of the permission logic. While making it atomic really makes sense, I also think it is important to take the opportunity to define and document the error cases. Eg,<br>- In case a `set_access()` fails, a certain exception should be thrown by the driver, and the backend must gurarantee that the previous permissions are intact. Otherwise we still might have a mismatch between the permissions in Manila and the storage controller.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
    <br>
    It's not something I intend to push for in Liberty however.<br>
    <br>
    -Ben<br>
    <br>
    <br>
    <blockquote type="cite">
      <div dir="ltr">
        <div>Cheers,<br>
        </div>
        <div>Björn<br>
        </div>
      </div><span class="">
      <br>
      <fieldset></fieldset>
      <br>
      <pre>__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: <a href="mailto:OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a>
</pre>
    </span></blockquote>
    <br>
  </div>

<br>__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
<br></blockquote></div><br></div></div>