David, I think that you successfully covered most of the concerns. I fully expected there to be a number of decisions that would be immutable for that reason. I'll look for the update.
--Martin On 13 August 2012 14:32, Black, David <david.bl...@emc.com> wrote: > 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
