Improve PortsOrch::doPortTask()

[qiluo-msft]

(Reduce the scope of this PR, please check the updated comment below)

I tested with virtual switch (vs) and Mellanox platform. The initial config of 'fec' is added in /usr/share/sonic/device/*/*/port_config.ini. However this manual test is not materialized as a unit test because of some difficulty.

Original implementation of PortsOrch::doPortTask() has some issues:

1. It was using a ConsumerStateTable, which makes the message coming out of order. The complex logic was used to handle the order. After replace ConsumerStateTable by ConsumerTable, we can simplify the logic.

2. The original ConsumerStateTable implementation will always pop all entry attributes even there is one attribute coming. This behavior is not by design and was changed after Improve ConsumerStateTable: del after consumed, and let consumer writes the final table sonic-swss-common#204 merged. The PortsOrch::doPortTask() takes advantage of this specific behavior and doesn't handle some port attributes correctly, if they come before PortConfigDone. These attributes are m_autoneg, speed, mtu, admin_status, fec_mode.

This PR solve these issues, and add vs test case.

[jipanyang]

If my understanding is correct, with the change, no initPort() will be called until PortConfigDone is received. The code logic below will block the au, speed, fec setting from being applied before PortConfigDone. Not sure how they will be applied later.

Also gBufferOrch->isPortReady(alias) brings more dynamics here.

            if (!gBufferOrch->isPortReady(alias))
            {
                // buffer configuration hasn't been applied yet. save it for future retry
                it++;
                continue;
            }

            Port p;
            if (!getPort(alias, p))
            {
                SWSS_LOG_ERROR("Failed to get port id by alias:%s", alias.c_str());
            }

Did you test on hardware platform with some au, fec setting in the initial config?
#Resolved

[qiluo-msft]

I could confirm "with the change, no initPort() will be called until PortConfigDone is received". Is there an issue here?

"gBufferOrch->isPortReady(alias)" will block these settings. The 'it' pointed entry is not erased and remains in consumer.m_toSync. Anytime unblocked, that entry will be picked up again and applied. It is a kind of postponed.

I tested with virtual switch (vs). The initial config of 'fec' is added in /usr/share/sonic/device/x86_64-dell_s6000_s1220-r0/Force10-S6000/port_config.ini. However this manual test is not materialized as a unit test because of some difficulty.

---

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

[jipanyang]

What if there is no buffer pg and queue configuration in configDB, will "gBufferOrch->isPortReady(alias)" be true and not block it? #Resolved

[qiluo-msft]

The behavior about "gBufferOrch->isPortReady(alias)" is unchanged.

---

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

[jipanyang]

The existing port config logic doesn't have hard dependency on gBufferOrch->isPortReady(alias), but more relying on PortConfigDone check. Now your whole solution relies on this gBufferOrch->isPortReady(alias) check, at least a clear explanation and understanding of the logic is needed, more comments are needed to record the complex interdependency of PortInitDone, isPortReady and port configuration.

It looks to me, one line change in existing code will have the same effect of solving the problems you mentioned :

            if (!m_portConfigDone)
            {
                // it = consumer.m_toSync.erase(it);
                it++;
                continue;
            }

[jipanyang]

Looks good to me.