OpenStack Compute (nova)

Code review comment for lp:~justin-fathomdb/nova/justinsb-hpsan2

justinsb-hpsan2
Merge into trunk

Revision history for this message

justinsb (justin-fathomdb) wrote on 2011-02-21:

I documented the return value in VolumeDriver create_volume & create_export.

I changed the name of the model state change Dictionary from db_update to model_update, based on your suggestion. The volume drivers are tightly coupled to the model definitions, so I don't think we're violating any separation of concerns. However, the naming "DB" was violating the separation, so I renamed it. Thanks for pointing this out.

The two new properties are 'provider_auth' and 'provider_location'. They're intended to be opaque properties for the driver's own use, but the natural assumption is that _location would be data as to the location and _auth info about authentication. I could use a dictionary instead, but I tried to pick two sensible fields that are hopefully fairly generic across drivers. Using a dictionary is problematic in terms of how it gets serialized efficiently. I documented the default iSCSI use of provider_location and provider_auth on ISCSIDriver.

The iSCSI discovery code is inherited, though the indentation level changed (I'm probably not blameless on the original code though!) I agree that it's fragile, so I'd like to move away from it. HP SANs and Solaris 'SANs' no longer use it; I can fix OpenISCSI next but that's a behavioural change on code that is potentially in active use so probably belongs in its own patch. There's a note that discovery is deprecated, but this code path is there until I can fix it for OpeniSCSI, but it is the same code as before. If you think I should clean it up in this patch I can do that, but I think it's best not to.

The CHAP authentication issue with discovery will go away when discovery is removed. HP SANs don't use discovery, and they're also the only ones using CHAP right now. If CHAP is supported with discovery (?), we'd definitely have to pass some extra parameters which aren't being passed right now, and I don't see the point of making this chaneg if I'm going to remove this code anyway :-)

I documented the current iscsi_properties (though I really don't know how to format this so it doesn't look terrible)

I documented the return value in VolumeDriver create_volume & create_export.

I changed the name of the model state change Dictionary from db_update to model_update, based on your suggestion.  The volume drivers are tightly coupled to the model definitions, so I don't think we're violating any separation of concerns.  However, the naming "DB" was violating the separation, so I renamed it.  Thanks for pointing this out.

The two new properties are 'provider_auth' and 'provider_location'.  They're intended to be opaque properties for the driver's own use, but the natural assumption is that _location would be data as to the location and _auth info about authentication.  I could use a dictionary instead, but I tried to pick two sensible fields that are hopefully fairly generic across drivers.  Using a dictionary is problematic in terms of how it gets serialized efficiently.  I documented the default iSCSI use of provider_location and provider_auth on ISCSIDriver.

The iSCSI discovery code is inherited, though the indentation level changed (I'm probably not blameless on the original code though!)  I agree that it's fragile, so I'd like to move away from it.  HP SANs and Solaris 'SANs' no longer use it; I can fix OpenISCSI next but that's a behavioural change on code that is potentially in active use so probably belongs in its own patch.  There's a note that discovery is deprecated, but this code path is there until I can fix it for OpeniSCSI, but it is the same code as before.  If you think I should clean it up in this patch I can do that, but I think it's best not to.

The CHAP authentication issue with discovery will go away when discovery is removed.  HP SANs don't use discovery, and they're also the only ones using CHAP right now.  If CHAP is supported with discovery (?), we'd definitely have to pass some extra parameters which aren't being passed right now, and I don't see the point of making this chaneg if I'm going to remove this code anyway :-)

I documented the current iscsi_properties (though I really don't know how to format this so it doesn't look terrible)

« Back to merge proposal