Martin, Thank you very much for this review - you found a number of things that need attention, but there are also a few comments that (probably unbeknownst to you) represent design decisions that cannot be revisited courtesy of the amount of deployed iSCSI "running code".
Responses are inline below. Thanks, --David > -----Original Message----- > From: gen-art-boun...@ietf.org [mailto:gen-art-boun...@ietf.org] On Behalf Of > Martin Thomson > Sent: Tuesday, July 24, 2012 6:54 PM > To: draft-ietf-storm-iscsi-sam....@tools.ietf.org; gen-art@ietf.org > Subject: [Gen-art] GenART review of draft-ietf-storm-iscsi-sam-06 > > I have been selected as the General Area Review Team (Gen-ART) > reviewer for this draft (for background on Gen-ART, please see > http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html). > > Please wait for direction from your document shepherd > or AD before posting a new version of the draft. > > Document: draft-ietf-storm-iscsi-sam-06 > Reviewer: Martin Thomson > Review Date: 2012-07-24 > IESG Telechat date: ? > > Summary: This document has a few issues that would need to be > addressed before it is published as a Standards Track RFC. > > Major Issues: None > > Minor Issues: > > Is the intent to reference RFC 3720 or the new -cons draft (as > published)? Given that -cons is intended to obsolete 3720, I would > have expected no reference to 3720 at all. That's a correct expectation, and results from some confusion over what the final status of RFC 3720 would be. As RFC 3720 is being obsoleted by the -cons draft, RFC3720 cannot be normatively referenced by either the -sam or -cons drafts, and hence most of the citations in the body of the -sam draft need to be to the -cons draft. > Section 5.1 / 5.1.1 does not define how many bits the PRI field > consumes. I might infer from the diagram that is the four most > significant bits of the second byte, but the diagram is unclear. In > any case, the diagram should only be used for illustrative purposes > and the text should provide the definitive answer. > > Section 5.2.1 also relies on the diagram. Though in this case it is > clearer and I can easily infer that the status qualifier is one byte, > I'm still relying on the diagram. These two are minor, but I have no problem with changing them as suggested. > Why is Section 6.2.1 not Section 5.1.2? Same for 6.2.2 and 6.2.3. > Aren't these new (?) additions to the Command PDU? Or is there > another PDU that these apply to? There's another PDU that these apply to, the Task Management Function Request PD - its layout diagram needs to be copied into the -sam draft from section 11.5 of the -cons draft so that the -sam draft is somewhat more self-contained. > Section 7.1.1 What does it mean to not claim support for any > particular level? Obviously you can't use the features described > here, but what else? It just means that the system is running "iSCSI" without providing any additional details. The ability to announce support without claiming support for any particular level is a SCSI feature that is applied to all SCSI-related standards. The other party in the communication needs to make least common denominator assumptions about supported functionality, subject to further checks that may provide additional information (e.g., iSCSI text key negotiation, read a particular SCSI VPD page). > Section 8 makes a bold claim that needs substantiation. Given the > global nature of task identifiers, is it necessary to prevent one user > from querying a task that another created? No it is not - to the extent that iSCSI has a notion of user, that notion is session-wide across all connections. > What about triggering reset on a I_T nexus that another user is using? The I_T NEXUS RESET function in the -sam draft can't do that because that function is reflexive - it resets the I_T nexus on which the function is sent. Yes, this really is a SCSI version of "shoot yourself in the foot" :-) and it is useful because target systems generally respond by cleaning out all of the state related to the nexus which can clean up some situations. Nonetheless, there are already SCSI-defined means in iSCSI to take out other I_T nexuses, and the security measures for those (primarily configuration-based access control) are handled at the SCSI level. Something else to keep in mind is that the usual notion of "user" as a person does not apply to SCSI and iSCSI. The initiator is typically an operating system driver or iSCSI offload HBA, and hence the identity is a system identity as opposed to a user identity. > Questions (for -cons): > > Section 6.2 makes the following requirement: > If the connection is still active (it is not undergoing an > implicit or explicit logout), QUERY TASK MUST be issued on the > same connection to which the task to be queried is allegiant at > the time the Task Management request is issued. If the > connection is implicitly or explicitly logged out (i.e., no other > request will be issued on the failing connection and no other > response will be received on the failing connection), then a > QUERY TASK function request may be issued on another connection. > This Task Management request will then establish a new allegiance > for the command being queried. > > The comment is probably more for of -cons (Section 4.2.5.1 and Section > 7.2.2). Obviously, this design is long-standing, and I'm only reading > parts of the specification. And there's a lot of "running code" that uses this design. > There is an implication that tasks have identification that is > globally unique, even if the normal behaviour is to bind (ally) a > request with a connection. The reasons for allowing this are not > explained, but it imposes some fragility on the system. For instance, > the requirement that the old connection be closed with a "remove the > connection for recovery" seems counter to the goal of failure > recovery. If the point of this is for failure recovery, then a > primary failure mode would be network failure - in which case this > mechanism cannot be used. There are multiple notions of "global" involved here. Task identity is global across the session, but not across independent sessions. SCSI multipathing (e.g., as used with SANs) involves independent iSCSI sessions across which there is no shared iSCSI state. The primary motivation for this iSCSI failure recovery mechanism is partial network failure, e.g., the two TCP connections are assigned to two different server NICs on the same network, and one of those NICs fails. Complete network failure is not a motivation, as that would be handled at a higher level, e.g., by SAN-oriented SCSI multipathing across iSCSI sessions on different networks, and obviously, there's not much that can be done if all network connectivity is lost between a host and its storage. > What purpose does this allegiance feature address? Isolates connection failure effects to I/Os that use the connection as opposed to making them global. It's not ideal, in that the CmdSN window will likely run out by the time that the failure is detected and recovered from, but at least failure of one connection doesn't stop all I/Os in their tracks. This also avoids a functionality shift when moving from one-TCP-connection sessions (which are common) to multiple-TCP-connection sessions. > What are the security implications of using allegiance? Not much that I can immediately think of. For any individual I/O, the state and data are on a single connection, but security considerations generally do not enter into connection selection - load balancing tends to be the primary consideration. > Nits: > > I don't know what tooling was used to generate this document, but > there are some strange artifacts. In particular, diagrams and tables > are misaligned in a number of places. Guilty as charged (sorry), and there's actually a mistake in the mapping table courtesy of another tool glitch. A more careful check will be done on the next version before it's submitted. This is partly my fault, as I leaned on the editor to get this version submitted with a text correction, not realizing that the tooling would take advantage of the resulting haste to make waste ... > There are a few terms like "I_T nexus" that are not explained prior to > use. These are defined in -cons. However, I find the existence of a > terminology mapping table in this draft to be strange. Wouldn't that > table be more useful in the "core" document? Good suggestion, I've cc:'d the author of the -cons draft, and we'll figure this out in consultation with the WG. > > Section 4.1: > > This seems unnecessarily convoluted: > > Note that an operational value of "2" or higher for this key on > an iSCSI session does not influence the SCSI level features in > any way on that I_T nexus. An operational value of "2" or higher > for this key permits the iSCSI-related features defined in this > document to be used on all connections on this iSCSI session. > SCSI level hand-shakes (e.g. commands, mode pages) eventually > determine the existence or lack of various SAM-5 features > available for the I_T nexus between the two SCSI end points). To > summarize, negotiation of this key to "2" or higher is a > necessary but not a sufficient condition of SAM-5 compliant > feature usage at the SCSI protocol level. > > How about: > > In addition to negotiating a value of "2" or higher, support for an > individual > MUST also be signaled using SCSI level hand-shakes prior to use. > > I know that adds 2119 language, but if the goal is to prevent someone > from attempting to use a feature prior to getting positive > confirmation of support, then this should be ok. I think the addition of a "MUST" here is fine, and we'll see about crafting some language that better reflects the SCSI aspects, as "hand-shakes" is actually not a common SCSI term. > Section 6.2 describes multiple tasks. It would be good if the > description of each of the tasks were given a sub-section so that they > could be cross referenced and more readily found. Unfortunately, RFC 3720 and the consolidated draft were not written that way, and we'd prefer to keep that structure. > > --Martin > _______________________________________________ > Gen-art mailing list > Gen-art@ietf.org > https://www.ietf.org/mailman/listinfo/gen-art _______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art