I believe there is work to be done if there is a timer running or a previous reconciliation effort is in progress so likely you need an else part. While the connection was severed (not due to fpmsyncd restart) the warmStartEnabled may have changed state and lots of things won't work after that.

@nikos-github thanks for your comments.

You have a point here. For the above to happen we need an unlikely sequence of events to take place:

1. two or more zebra crashes happening in a small window of time, and
2. a user deactivating routing-warm-reboot feature right between the two zebra crashes

Unlikely but possible. I'll adjust my code to deal with this scenario.

If warm restart is found to be disabled, we should still recover and cleanup. There should be no requirement to disrupt the lower sonic layers now that we are adding such infra/functionality.

Please amend the code to address initial comment.

@nikos-github i missed this comment. Yes, i'm about to send one (hopefully final) update that explicitly clears the state of the data-structures used within WarmStartHelper class. That should take care of your previous concerns for the bgp/zebra restarting case.

I believe some cleanup is required if there is a timer already running.

Please address the comment.

Yes, i'm taking care of it, see my other comment on this matter.

Before recovering the appdb state, you need to either clear or check that the producer queue is empty.

Yep, already took care of that, my next patch will have it.

Will there be too many logs for pipeline flush in normal running?

@jipanyang I'm just adding one extra "Pipeline flushed" log, which will be executed only once upon restart-timer expiration. During normal operation we will continue generating this log the same way we are doing it today -- see that i'm not adding this.

The original log level is DEBUG, now it is changed to NOTICE.

Sure, i can set all "Pipeline flushed" logs to debug.

"warmState == WarmStart::INITIALIZED", It is confusing, while warm state is INITIALIZED and route is injected to appDB directly. Do you mean warm start has been disabled?

My understanding, based on a recent discussion where we discussed/agreed on this issue, is that INITIALIZED state will be utilized for applications with warm-reboot feature disabled -- so these ones will permanently display INITIALIZED state. Based on that discussion i had to re-adjust my code and add this INITIALIZED checkpoint. My original code was setting all applications to RECONCILED state, regardless of WarmReboot being enable or not, as this is the last FSM stage. Looks like our problem here is simply a consequence of INITIALIZED state being slightly overloaded. The simplest solution is to provided another FSM state (DEACTIVATED). Will ping you offline to discuss FSM semantics.

Maybe I missed some of the discussions. My understanding for WarmStart state is that it is only meaningful when warm start has been enabled. If warm start is disabled, people won't care about WarmStart state.
Putting too much state change seems to make the problem unnecessary complicated.

As you know, applications like fpmSyncd need to be able to invoke WarmRestart logic on-demand, and not only when when it boots-up. So the initialization of WarmRestart logic is not necessarily connected to its activation: you could have WarmRestart being turned on at any given point in the future without having to restart fpmSyncd. To fix this problem there are two possible solutions: 1) we overload one of the current FSM states (INITIALIZED), or 2) we create a new UNINITIALIZED/DEACTIVATED FSM state.

For fpmsyncd case, whenever warm restore/reconcile is needed and warm start has been enabled, the INITIALIZED state could be set before starting restore to indicate that warm start is initiated, after restore, RESTORED; then RECONCILED.

I don't see the need to overload INITIALIZED or add new DEACTIVATED state.

Actually "INITIATED" looks like a better name than "INITIALIZED" if we are talking about warm restart state.

I suggest you use RECONCILED always. I agree with Jipan that INITIALIZED is confusing and at the same time UNINITIALIZED/DEACTIVATED will be equally confusing if warm restart is turned on after fpmsyncd is up because the state won't match with what's configured.

Nikos, you are agreeing with me then. This is precisely my original approach: all apps end up in RECONCILED state. As i said above, i had to re-adjust my code to address the 'requirement' of having apps (with warm-reboot OFF) using the INITIALIZED state.

Jipan's approach is not using any state to represent apps with warmReboot turned off, which is something that i'm struggling with. Btw, the approach that you're suggesting wouldn't add a new state, but RECONCILED state would be conveying two semantics: 1) application with warm-reboot ON going through all FSM states, and 2) application with warm-reboot OFF skipping all FSM states. Either way, we are slightly overloading the FSM, which i'm totally fine with. But there's no free lunch here: either we overload or we add a new FSM state.

I'm trying to point out a distinction between FSM state and whether the feature is enabled or not. Trying to infer what's the feature's state (on or off) from the FSM isn't the right thing in my opinion. If you want to convey both then you either need 2 variables, no overloading of FSM state and introduce another state in the FSM, OR you need 1 variable that has states plus feature enabled/disabled (via a bitmask), no overloading of FSM state or use reconciled. If you don't want any of the above, then from my point of view reconciled is the right state whether the feature is disabled or later enabled.

@nikos-github just to close the loop here. As discussed with the rest of the folks working in warm-reboot, we will rely on the assumption that no FSM state implicitly indicates the lack of warm-reboot functionality, or the fact that this one hasn't been turned on. The three FSM states (initialized, restored, reconciled) will be exclusively used for apps that are warm-reboot capable.

I have no problem with that. The state should be reconciled though when the app is warm reboot capable but warm reboot isn't enabled or is enabled later.

I suggest you use RECONCILED always. I agree with Jipan that INITIALIZED is confusing and at the same time UNINITIALIZED/DEACTIVATED will be equally confusing if warm restart is turned on after fpmsyncd is up because the state won't match with what's configured.

reconcile not reconciliate

done

We should recover gracefully here by falling back to cold restart.

Not sure what do you mean by 'gracefully'. I understand that we should do something about it, but that's something the process-manager (supervisord) should take care of upon the app being cold-booted after the failed assertion.

How will supervisord achieve a cold boot since warm restart is enabled? In any case, asserting is shouldn't be here in my opinion - that's way too harsh. Flushing the db and re-establishing the connection will cause the routes to re-download. That's why I mean by gracefully.

The original log level is DEBUG, now it is changed to NOTICE.

Is there "else" to take care of remaining conditions?

if()
    {
    }
else if
    {
    }

Which other conditions are you referring to? There are only three conditions that we care about here: 1) warmStartTimer expired, which triggers reconciliation, 2) warm-restart disabled -- to deal with legacy/current scenarios, and 3) scenarios with warm-restart reconciliation completed. In 2) and 3) the actions are exactly the same, that's why i'm bundling them together. What would be the use case for the 'else'?

I was not checking the internal logic. If the "else if" covers everything, why don't you just change it to else {}.

Based on your current implementation, it looks some default processing is missing.

if (condition 1)
{
}
else if (condition2)
{
}
else {
// default processing;
}

Got your point now. The "else if" is there to make it more explicit to the reader that those two conditions could take place. Performance-wise there's no doubt the "else" is slightly cheaper. Have no strong opinion here, i can make use of an "else" if everyone agrees.

could you get away from static_cast? Using something like size_type j = 0, or auto j = splitValues.size();

same as previous comment.

When comparing two vectors, how to ensure they are of same size?
Say, a route has two fields in usual case: nexthop and intf, what if for some routes, the third fields "blackhole" is set?

### ROUTE_TABLE
    ;Stores a list of routes
    ;Status: Mandatory
    key           = ROUTE_TABLE:prefix
    nexthop       = *prefix, ;IP addresses separated “,” (empty indicates no gateway)
    intf          = ifindex? PORT_TABLE.key  ; zero or more separated by “,” (zero indicates no interface)
    blackhole     = BIT ; Set to 1 if this route is a blackhole (or null0)

Good point. For routing-app there's this "blackhole" corner-case which brakes the uniformity for routing schema. Basically we are allowing a route to be defined as: "prefix/len | blackhole = true". No other app/scenarios that i'm aware of allow this inconsistency. In my humble opinion, we should keep uniformity by allowing the common schema to be re-utilized as this: "prefix/len | nh: blackhole | if: null". This approach will also simplify the addition of new attributes in the future (AFI, SAFI, etc).

Also, if you look at orchagent you'll see that we are not passing down "blackhole" routes -- they are being eliminated right there in routeOrch. The only blackhole route being pushed down to ASIC_DB is the default route, and this one is not even exposed to AppDB. So in reality, no one is really consuming these black-hole schema.

In my opinion, I think this assert() should be left here as it prevents real problems down the road. And concerning the blackhole routes, i would like to make a very minor adjustment in its representation to avoid this uniformity i talked about.

I was not checking the internal logic. If the "else if" covers everything, why don't you just change it to else {}.

Based on your current implementation, it looks some default processing is missing.

if (condition 1)
{
}
else if (condition2)
{
}
else {
// default processing;
}

I had a difficult time understanding the logic here.
What if there are multiple fields? It looks whenever the field is checked, it return immediately without checking other fields.

It doesn't really matter how many fields we have as long as the schema they utilize keeps its consistency. The purpose of this function is to enforce an order within the map, so technically there's no real need to compare all the elements within a tuple. For example, if you are comparing these two tuples corresponding to a particular prefix...

left -> nh: 10.1.1.1, if: eth0
right-> nh: 10.1.1.2, if: eth1

... you can exit() the 'comparator' method as long as you have found a common-field within each tuple that can be used for comparison. Please let me know if you can think of a use-case that would require the complete iteration of the tuple.

As i said before, the above can only work if the schema is kept uniform, which is the case for all apps so far (with the exception of the 'blackhole' route).

And yes, i'm planning to add some comments to document what this function does.

"fvVector[i].second" and "fvValue(fvVector[i])" should be equal. Use one format.

Is the order of fields in fvRestorationMap guaranteed?
I saw you put comment in fvComparator

/*
             * Notice that we are forced to iterate through all the tuples in the
             * right-vector given that the order of fields within a tuple is not
             * fully deterministic (i.e. 'left' could hold 'nh: 1.1.1.1 / if: eth0'
             * and 'right' could be 'if: eth0, nh: 1.1.1.1').
             */

Should they be private?

sure, i can make them private.

What if v1 is superset of v2?

If by 'superset' you're referring to v1 having more fields than v2, that shouldn't ever happen as you commented further below. I'll add more details to my code-comments about this expectation.

Could you use CLI to config it as other test cases?
config warm_restart enable/disable [APP]

Agree, that's the way to go.

"config warm_restart bgp_timer" should be prepared as that for arp:

root@sonic:/home/admin# config warm_restart --help
Usage: config warm_restart [OPTIONS] COMMAND [ARGS]...

  warm_restart-related configuration tasks

Options:
  -s, --redis-unix-socket-path TEXT
                                  unix socket path for redis connection
  --help                          Show this message and exit.

Commands:
  disable
  enable
  neighsyncd_timer
root@sonic:/home/admin# 

To avoid deferring this PR merge, i'll make this small adjustment right after i write this addition to sonic-utils. Then i'll come back to this UT code and will adjust accordingly.

What is the reason for using external pipeline for m_restorationTable here? m_restorationTable is for get only. Asking the caller to provide pipeline brings no benefit.

Not sure i got this one. How can we create the table without its pipeline? You mean, using the other table constructor? -- Table(const DBConnector *db, const std::string &tableName). Even if that's what you imply, we would need to create a DBConnector if we don't want the caller to provide it, right?

Please clarify...

Sorry, my previous comment is confusing. WarmStartHelper() has both pipeline and syncTableName as parameters, the only purpose of them is to prepare a Table handle in WarmStartHelper class. Could a Table handle be prepared by the callers directly and remove pipeline/syncTableName, just like what has been done for "ProducerStateTable *syncTable"?