Subject: Review of the draft-ietf-netconf-partial-lock-01

From: "Ersue, Mehmet (NSN - DE/Muenich)" <[email-masked]>
Date: Tue, 4 Mar 2008 14:13:34 +0100

Hi Balazs,

I reviewed the -01 version of the draft "Partial Lock RPC for NETCONF". 
In general the draft looks good but needs some improvements.

Please find below my comments:

Section 2.1., paragraph 2:

as David H. states please use explicit wording from RFC 2119:
"MUST ensure" instead of "ensures" 

s/be//

Section 2.1., paragraph 3:

Would change for more clarity:
"The partial lock operation starts when the partial lock is granted 
and lasts until either the corresponding <partial-unlock> operation 
succeeds or the NETCONF session terminates."

Section 2.2.:

Please use MUST or SHOULD to indicate what needs to be supported 
as a minimum and what can be supported additionally.

Section 2.4.1., paragraph 4:

I was wondering whether we need more explanation of the atomic 
operation. I would suggest to extend the text with following for clarity

even if it looks redundant:

"I.e., the server MUST ensure to unlock all already locked parts if one 
of the requested parts cannot be locked."

Section 2.4.1., paragraph 7, bullet 2:

The conclusion of the discussion last December was that we don't 
want to change Netconf RFC chapter 7.5, which says:
"An attempt to lock the configuration MUST fail if an existing session 
or other entity holds a lock on any portion of the lock target."

The wording "another management sesion" implies excluding the current
session.
Based on the conclusion of our discussion, "overlapping partial locks by
the 
same session are not allowed", I would suggest to change 2nd bullet to:

o  Any part of the scope to be locked is already locked by any
   management session/protocol, including the current session and 
   other NETCONF sessions using the <partial-lock> or any other 
   non-NETCONF management method.

Section 2.4.1., paragraph 7, bullet 3:

IMO "at least some basic access rights" is too vague. We need to state
explicitely which access rights are necessary.
I assume the reason why a session uses partial-lock is to be able to
write in
an atomic way (this is at least mentioned in Section 2.4.1., paragraph
9). 
So, why is it sufficient to have "read rights"? Was the intention here
to prevent 
others from writing?

Section 2.4.1., paragraph 8:

I would like to suggest to use more explicite wording.
I assume you mean "sessions" if you say "users" and  "different locking
scopes" 
if you say "different parts".

My question is: How is it possible that two sessions trying to lock
"different 
parts" get dead-locked? IMO the dead-lock happens only if session A and 
session B try to lock the "same locking scope" and session A does not
free 
the partial-lock it executed and session B waits forever.

Based on the rule defined in Section 2.4.1., paragraph 4 the
partial-lock 
(of potentially many lock areas) is handled in an atomic way. So i.e.:
Locking a set of "locking scopes" is a good strategy if session A
doesn't 
want to be disturbed during it's work.
Locking a set of "locking scopes" at once does not help against waiting 
of session B if something is already locked by session A.

Section 2.4.1., paragraph 9:

I would skip "you get what you asked for".
"It is the intention to keep partial-locking simple. Partial 
lock aims to lock a set of nodes for writing only." 

I would like to ask the same question as David H. What are the issues 
that are intentionally not addressed. But I see in your mail that you
want 
to s/./:/ ;-).

Section 2.4.1., paragraph 9, bullet 2 and 3:

'Section 2.4.1., paragraph 8', 'Section 2.4.1., paragraph 9, bullet 2'
and 
'Section 2.4.1., 9th paragraph, bullet 3' have the same general
statement 
in mind and seem to be redundant. I would reduce some of the redundant 
text with a clear statement:

"It is the operator's responsibility to lock all parts of the 
datastore that are crucial for a specific management action."

Section 2.4.1., paragraph 10:

It is interesting for clarity if you add this or exchange it with parts
of the
current text:

"Partial and global lock follow the same rules as defined in RFC4741."

Section 2.4.1., paragraph 16 (beginning with "If any select expression 
returns anything but ..."):

I don't agree with David H., if the select expression is erroneous the
<error-tag> shall be 'invalid-value'.
Also here we should use RFC 2119 wording: "SHALL"

I would change the paragraph for more clarity:
"If the select expression is erroneous and does not point to 
a node set, the <error-tag> SHALL be 'invalid-value'.

Section 3.:

IMO it is important to extend this section. We should state what we mean

with "some access control mechanism" at least to some extend and
although
we have no access control draft yet.

<***>
A general issue I anticipate here is that we most likely cannot get the 
"Partial Lock" draft and also other drafts which depend on "NETCONF 
access control mechanisms" through the IESG. I assume IESG will ask 
for it as a companion draft and we need to provide the AC mechanism 
in parallel. We should discuss this on the maillist and need also the
advice 
of Dan.
</***>

Section 4.:

Since we had some issues during the shepperding of the Notifications
draft I would like to suggest to use the wording and template from the 
Notifications draft (other useful examples are available in RFC 4825).

Appendix A:

I would divide it in two sub-sections or appendices: Change log, Open
issues.

Appendix A - Open Issues:

I think we shouldn't introduce a special rule for locking non-existent
nodes. 
IMO it is better if the operator creates "empty nodes" if he wants to
reserve
an object or it's name/key. 

We should include more detailed information in error results to help
debug 
lock conflicts but how detailed should this information be. If you are
in 
favor please provide a proposal and we can discuss and decide in the WG.

IMO we should allow users to lock parts of multiple datastores. This is
in 
support of the "atomic lock operation" for a set of "locking scopes" at
once, 
which is useful and necessary even beyond the borders of a datastore and

even the current (global) <lock> operation does not support this.

Appendix B, C:

I understand you would like to see YANG as the "normative DML" but we 
need to choose one of the models in the appendices as the normative 
reference of today. As I understood Dan correctly, currently XSD can be 
used as a the "normative reference".

Section 9.:

Please add YANG draft to the list of references.

Cheers, 
Mehmet