Bug #3232
closedInaccurate log message when changing FacePersistency
100%
Description
FaceMgmt faces/create
command allows changing persistency setting of an existing face on the direction of on-demand -> persistent -> permanent.
When the persistency setting is changed, FaceManager
and Channel
logs are almost same as creating a new face, and FaceTable
would log a warning because FaceManager
attempts to add the existing face again.
nfdc
log is same as creating a new face.
These logs do not represent the true actions taken by NFD, and can cause confusion to operators.
Updated by Junxiao Shi about 9 years ago
See original complaint in #2993 note-16.
FaceManager Channel
logs can be improved easily, and FaceTable
warning can be eliminated easily.
nfdc
's indistinguishable log is a deeper problem: in FaceMgmt protocol, the response does not indicate whether the face is existing or newly created.
The straightforward solution is to use different StatusCode
s: 201 for new face, 202 for changing properties on existing face.
But I feel this solution is too limited:
- Semantically, creating face and changing properties are two different operations.
- There's still no way to change the persistency on permanent -> persistent -> on-demand direction.
Although it's less useful, this operation should be allowed.
A use case is: a core link on an overlay topology was created as a permanent face; due to a topology change, this link is no longer part of the topology. Instead of destroying the face, it's better to change the persistency to on-demand, so that pending Interests on this face can still be satisfied. - There's no way to change the persistency setting of a face that cannot be created by
faces/create
command.
A better solution is:
faces/create
command considers static properties, such as persistency, only for creating new face.faces/create
command does not change static properties on an existing face, but will return StatusCode 409 (conflict), with ControlParameters struct that describes the properties of the existing face.- A separate
faces/update
command can update static properties on an existing face. It requires FaceId parameter.
Updated by Junxiao Shi about 9 years ago
20151013 conference call, Alex approves note-1 design.
Updated by Junxiao Shi about 9 years ago
- Related to Feature #3229: nfdc register: implicitly create permanent face added
Updated by Junxiao Shi almost 9 years ago
- Blocked by Feature #3461: FaceMgmt: faces/update command added
Updated by Junxiao Shi almost 9 years ago
- Estimated time set to 6.00 h
Protocol definition proposed in note-1 and approved in note-2 will be worked in #3461.
This issue only contains the implementation of said protocol, including FaceManager
and nfdc
.
Updated by Junxiao Shi over 8 years ago
- Assignee set to Yanbiao Li
- Target version set to v0.5
20160412 call, Beichuan decides to assign faces/update
command implementation to @Yanbiao.
Updated by Yanbiao Li over 8 years ago
NFD code is finished, but to make it work and to enable unit-tests can pass, I think we need to update ndn-cxx first. For at least two reasons:
1) we must add support for FaceScope and LinkType in ndn::nfd::ControlParameters.
2) we must add request validation logics for FaceUpdate command in ndn::nfd::ControlCommand.
So, shall I add a separate commit for above updates first?
Updated by Junxiao Shi over 8 years ago
we need to update ndn-cxx first
Of course. It should be a commit on ndn-cxx repository, under the same Redmine issue.
ndn-cxx and NFD commits should be uploaded together. NFD commit will initially have a build failure, but that's okay.
Updated by Yanbiao Li over 8 years ago
- Status changed from In Progress to Code review
- % Done changed from 50 to 90
Updated by Junxiao Shi over 8 years ago
I notice https://gerrit.named-data.net/3012 patchset4 introduces an nfdc update
command with questionable arguments.
This is not right. This issue refs #3232 only needs to solve the inaccurate log message problem, and should otherwise maintain backwards compatibility on the semantics of nfdc create
command. The logic for nfdc create
command should be:
- invoke
faces/create
command - if a new face is created, report successful face creation then exit
- if an existing face is found, check whether persistency setting is greater than or equal to the requested persistency; if so, report face already exists then exit
- invoke
faces/update
command to upgrade (but not downgrade) face persistency (from persistent to permanent) - if upgrading is successful, report successful persistency upgrading then exit
- if step1 or step4 fails, report the error
Updated by Junxiao Shi over 8 years ago
- Blocks Feature #3731: FaceManager commands: LocalFieldsEnabled added
Updated by Yanbiao Li over 8 years ago
I'm confused about two issues:
1/ What are valid transitions?
from note-1 I know both onDemand -> persistent -> permanent and permanent -> persistent -> onDemand are valid.
but note-12 indicates only one way to update, namely persistent -> permanent.
2/ Where to check the validation of persistency transitions?
I think it should be performed in FaceManager.
Updated by Junxiao Shi over 8 years ago
FaceManager
allows all transitions. It does not check transitions, but must catch exception when invoking setPersistency
because each Transport
defines what transitions are permitted.
nfdc create
only allows on-demand => persistent, on-demand => permanent, persistent => permanent.
There's no nfdc update
command.
Updated by Junxiao Shi over 8 years ago
- Blocked by Feature #3739: Expose ControlResponse body in Controller::CommandFailCallback added
Updated by Junxiao Shi over 8 years ago
- Blocks deleted (Feature #3731: FaceManager commands: LocalFieldsEnabled)
Updated by Junxiao Shi over 8 years ago
- Blocked by Feature #3731: FaceManager commands: LocalFieldsEnabled added
Updated by Junxiao Shi over 8 years ago
- Status changed from Code review to Feedback
Updated by Davide Pesavento about 8 years ago
- Target version changed from v0.5 to v0.6
Updated by Davide Pesavento about 8 years ago
- Status changed from Feedback to Code review
Updated by Yanbiao Li about 8 years ago
There is a design issue need to address:
Currently, the validation of changing persistency will not be checked before setting persistency.
If we detect other invalid parameters after setting persistency, we have to roll back the setting. While such rolling-back is unsafe.
Here is an potential solution:
1/ expose an interface (something like validateChaningPersistency) in the transport class to check the validation of changing persistency (only simple checks are performed here).
2/ make beforeChaningPersistency call validateChaningPersistency to perform the required check, leaving back any other operations (e.g. changing the status).
3/ expose an interface in the Face class that calls transport::validateChaningPersistency to perform the required check.
4/ in updateCommand, we just need to call Face::validateChangingPersistency instead of Face::setPersistency to check the validation of parameters. And then call Face::setPersistency after all parameters are validated.
Updated by Junxiao Shi about 8 years ago
I agree with note-22 idea but disagree with naming and requiring beforeChangingPersistency
to call validateChangingPersistency
. I suggest the following:
class Transport
{
public:
bool
canChangePersistencyTo(FacePersistency newPersistency) const;
// This function invokes canChangePersistencyToImpl if newPersistency differs from current persistency.
// The reason for separation is single responsibility principle: this function serves as external API, while Impl is internal.
void
setPersistency(FacePersistency newPersistency);
// 1. assert(canChangePersistencyTo(newPersistency))
// 2. change the persistency
// 3. invoke afterChangePersistency
private:
virtual bool
canChangePersistencyToImpl(FacePersistency newPersistency) const;
// This function can safely assume newPersistency differs from current persistency.
// Base class implementation returns true.
virtual void
afterChangePersistency(FacePersistency oldPersistency);
// Instead of beforeChangePersistency, this is morphed to afterChangePersistency
// to clarify that this function should not check whether the persistency change
// should be performed, but should focus on updating the internal state after
// the persistency change is already performed.
// Base class implementation does nothing.
};
Updated by Yanbiao Li about 8 years ago
I agree with note-23.
Another question: shall I submit a new commit? or leaving it within the current one?
Updated by Junxiao Shi about 8 years ago
Another question: shall I submit a new commit? or leaving it within the current one?
It's always preferable to use smaller commits, which are easier to review.
Updated by Junxiao Shi almost 8 years ago
- Status changed from Code review to Resolved
Code is merged. NFD devguide update is needed.
Updated by Junxiao Shi over 7 years ago
- Related to Bug #3976: NLSR not converging due to NFD faces/create semantics change added
Updated by Davide Pesavento over 7 years ago
- Status changed from Resolved to Closed
Yanbiao Li wrote:
dev guide is updated
I've fixed a few inaccuracies and made several other improvements. I believe we can close this issue.