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

Revision history for this message
justinsb (justin-fathomdb) wrote :

Thanks Vish & Devin.

Devin: I formatted the note as you suggested.

Vish: I moved ssh_execute into utils. It'll end up there in the end, and this way change tracking is easier. I do want to introduce SSH connection pooling in future.

I moved the comments into a docstring, though I don't know how they will look once they're the doc is generated. Once this is locked down, definitely agree this should be moved into docs.

Finally, I totally agree that the iSCSI target address vs san_ip vs iscsi_ip_prefix thing is ugly. I'm not actually sure what iscsi_ip_prefix is for (I had to rewrite the discovery function anyway, and I didn't need it) But long term, I think that a single volume controller may be managing multiple SAN nodes, and so I'd like to fix this properly at that stage. It will need a new attribute in the volume model, so I stayed away from it initially to keep the patch small. So I'd like to defer this to another patch if that's OK!

review: Needs Resubmitting

« Back to merge proposal