Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why the vlan cli always add member port to cfg_db VLAN table field 'members'? #323

Open
dawnbeauty opened this issue Jan 16, 2019 · 7 comments

Comments

@dawnbeauty
Copy link
Contributor

  • VLAN Schema

members of VLAN table -- https://github.com/Azure/SONiC/wiki/Configuration
What's the mean of field 'members' of VLAN table?
According to the code logic of vlanmgr, it only process the items in members@, but not members. And it treat all items in members@ as untagged members. The comment explain that This is to be compatible with access VLAN configuration from minigraph. So, can we ignore this field if we use config_db.json instead of minigraph?

  • VLAN CLI
    config vlan member add always add member port to the filed 'members' of VLAN table, regardless of tagged or untagged member. But, as previous section, we known that the members field is only used for untagged member.

It seems to be a bug and should clarify the role of members(or members@)

@guxianghong
Copy link
Collaborator

I should be a bug. In Table::get( ), @ in member@ have been stripped.

bool Table::get(const string &key, vector &values)
{
RedisCommand hgetall_key;
hgetall_key.format("HGETALL %s", getKeyName(key).c_str());
RedisReply r = m_pipe->push(hgetall_key, REDIS_REPLY_ARRAY);
redisReply *reply = r.getContext();
values.clear();

if (!reply->elements)
    return false;

if (reply->elements & 1)
    throw system_error(make_error_code(errc::address_not_available),
                       "Unable to connect netlink socket");

for (unsigned int i = 0; i < reply->elements; i += 2)
{
    values.emplace_back(stripSpecialSym(reply->element[i]->str),
                                reply->element[i + 1]->str);
}

return true;

}

@jipanyang
Copy link
Contributor

The "members@" field in vlan configDB table is for backward compatibility. If you use configb.json directly, VLAN_MEMBER_TABLE would be a better choice for vlan member configuration: https://github.com/Azure/sonic-swss/blob/master/doc/swss-schema.md#vlan_member_table

@guxianghong
Copy link
Collaborator

Thanks for clarification. JSON configuration example is from the page, it's better to remove the vlan member configuration for this page: https://github.com/Azure/SONiC/wiki/Configuration:
"VLAN": {
"Vlan1000": {
"dhcp_servers": [
"192.0.0.1",
"192.0.0.2",
"192.0.0.3",
"192.0.0.4"
],
"members": [
"Ethernet0",
"Ethernet4",
"Ethernet8",
"Ethernet12"
],
"vlanid": "1000"
}
},

@dawnbeauty
Copy link
Contributor Author

dawnbeauty commented Mar 7, 2019

@jipanyang thanks. And what do you think about the behavior of cli config vlan member add? The cli add ports not only VLAN_MEMBER_TABLE but also VLAN_TABLE|members. The port will be automatically set to be an untagged member even if we just want to add a tagged member.
Seems to me, it's a bug.
https://github.com/Azure/sonic-utilities/blob/8b6aea19ce048741b49fcc48979730b086ac62ea/config/main.py#L702

@jipanyang
Copy link
Contributor

VLAN_TABLE|members

If "config vlan member add" adds VLAN_TABLE|members unconditionally, I agree with you that is a bug. Would you provide a fix to the problem? thanks!

@dawnbeauty
Copy link
Contributor Author

Sure, later will link to the PR.

@dawnbeauty
Copy link
Contributor Author

dawnbeauty commented Dec 17, 2019

@jipanyang please help to review(sonic-net/sonic-utilities#768)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants