[xcvrd] Enhance xcvrd to handle new system level event/error

[keboliu]

This PR has a dependency on PRs:
sonic-net/sonic-platform-common#50
sonic-net/sonic-buildimage#3261

In the above PRs introduced the new system-level event, this PR to enhance xcvrd to be more robust by handling the more new system-level event:

1. wait for a certain period on system_not_ready event to overcome race condition when xcvrd started but system not ready.

2. start to routing work on system_become_ready event, this event indicates that the system is ready for SFP change event report.

3. exit on system_fail event

[qiluo-msft]

Receive [](start = 42, length = 7)

Past tense is better. As original 'Got'. Many time below. #Closed

[stephenxs]

fixed.

[qiluo-msft]

[](start = 32, length = 1)

wrong indentation. #Closed

[stephenxs]

fixed.

[qiluo-msft]

[](start = 33, length = 3)

inconsistent indentation #Closed

[stephenxs]

fixed.

[qiluo-msft]

platform_sfputil.get_transceiver_change_event [](start = 29, length = 45)

You can call platform_sfputil.get_transceiver_change_event function only once, and implement the logic as a normal state machine.

I don't mean your implementation is wrong, state machine may be much more readable and future proof. #Closed

[stephenxs]

Hi Qi,
I admit that rewriting it in a state machine approach can make the code a little shorter by eliminating some duplicate code.
But I'm afraid it can also introduce negative effects: We have the logic to control retry timespan and times and introduce some local variables to control that. Rewriting it as a state machine will combine the retry control logic into the main procedure of xcvrd, which make the main function complicated. #Closed

[qiluo-msft]

The benefit for state machine pattern is not shorter code, but a correct design and implementation.

For the design part, you need to consider any sequence of change events returned by vendor plugin code. For example, SYSTEM_NOT_READY followed by an inserted event. You need to draw the state machine on paper first to consider all the events at each state. You need to document the state machine diagram first to consider all the events at each state.

For the implementation part, you could design you state transition code in standalone function, and achieve 'control retry timespan and times' or 'local variables'.

---

In reply to: 310075723 [](ancestors = 310075723)

[stephenxs]

Hi Qi,
I understand the advantage of state machine approach. I just feel that the logic of xcvrd daemon is relatively simple and it seems that using state machine is less necessary.
IMO, state machine is adopted in event handling systems which have a lot of state and event, like network protocols. In those systems using state machine makes the flow clear and relatively easy to understand and also benefits the formal verification of that kind of system.
But for systems with less states and events the state machine approach doesn't have overwhelming advantage, since the current logic is straightforward. I have considered the state machine approach and found that only 2 states and 4 events. In this sense it's less necessary to go in this way.
meanwhile, I've created a PR in which I've updated the handling logic in a state-machine-like way, I think it is better than the current implementation. but it's not a strictly-speaking state machine. the pr is sonic-platform-daemons PR#5. You can check it.

[qiluo-msft]

As comments
Also blocked on sonic-net/sonic-platform-common#50

[qiluo-msft]

Reviewed again. Pending on #39 (comment)

[stephenxs]

As comments
Also blocked on Azure/sonic-platform-common#50

code updated with state machine approach
states definition

• Initial state: INIT, before received system ready or a normal event
• Final state: EXIT
• other state: NORMAL, after has received system-ready or a normal event

events definition

• SYSTEM_NOT_READY
• SYSTEM_BECOME_READY
• NORMAL_EVENT
  • sfp insertion/removal
  • timeout returned by sfputil.get_change_event with status = true
• SYSTEM_FAIL

State transmit:

1. SYSTEM_NOT_READY
   • INIT
     • retry < RETRY_TIMES_FOR_SYSTEM_READY
       retry ++
     • else
       max retry reached, treat as fatal, exit
   • NORMAL
     Treat as a fatal error, exit
2. SYSTEM_BECOME_READY
   • INIT
     transmit to NORMAL
   • NORMAL
     log the event
     nop
3. NORMAL_EVENT
   • INIT (for the vendors who don't implement SYSTEM_BECOME_READY)
     transmit to NORMAL
     handle the event normally
   • NORMAL
     handle the event normally
4. SYSTEM_FAIL
   treat as a fatal error

State event next state
INIT SYSTEM NOT READY INIT / EXIT
INIT SYSTEM BECOME READY NORMAL
NORMAL SYSTEM BECOME READY NORMAL
INIT/NORMAL SYSTEM FAIL EXIT
INIT/NORMAL NORMAL EVENT NORMAL
NORMAL SYSTEM NOT READY EXIT
EXIT -

[qiluo-msft]

for key, value in port_dict.iteritems(): [](start = 12, length = 40)

Why you put the for-loop outside the event/state branches? Can you move it inside a smaller branch? #Closed

[stephenxs]

Yes. You are correct.
Only for NORMAL_EVENT is this for loop required.
The "break" statements which originally break the inner "for" loop are also removed.

[qiluo-msft]

'INIT STATE' [](start = 13, length = 12)

You can use integer as value, don't rely on python to optimize the string comparison. #Closed

[stephenxs]

I think to use string is better since it makes logging information more human-readable.
Even though a dictionary can be adopted to map from integer value to string, the case that the state is not in the key set of the dictionary has to be handled. That's too complicated.
BTW, I'm not sure whether to use string address to compare the string is part of python's language specification or just optimization that depends on python interpreter's implementation?

[stephenxs]

states have been updated to integer value.
The events, like SYSTEM_FAIL, are already used by the plugins. In this sence, we'd better not to change them. It seems that to use string has already been a common practice in plugins, like the normal plugin/out events which are represented by '1', '0' respectively.

[qiluo-msft]

next_state [](start = 12, length = 10)

You may initialize it with state, next_state will never be None. #Closed

[qiluo-msft]

You can remove unexpected_state. This is redundant logic. #Closed

[stephenxs]

This is only for log.
By setting unexpected_state to true we can know why the state machine reach STATE_EXIT.

[stephenxs]

This is only for log.
By setting unexpected_state to true we can know why the state machine reach STATE_EXIT.

I decide to remove this. It is very unlikely to enter an unexpected state since all states are handled for each event.

[qiluo-msft]

repeated code, you can extract function. #Closed

[stephenxs]

now, only next_state is set when facing unexpected state. in this sense, no need to extract function.

[qiluo-msft]

You can move this part into event handling branches. #Closed

[stephenxs]

It's better to set it here since it can ensure that wherever the state reaches STATE_NORMAL the timeout can be properly set.

[qiluo-msft]

Please check my new comment, and also please resolve merge conflict for this PR.

[qiluo-msft]

if not next_state == state: [](start = 12, length = 27)

For readability, use if next_state != state

[stephenxs]

fixed.