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