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

[iccpd] Fix iccpd env #8945

Merged
merged 5 commits into from
Dec 8, 2021
Merged

Conversation

globaltrouble
Copy link
Contributor

@globaltrouble globaltrouble commented Oct 11, 2021

Why I did it

mclagsyncd source is in sonic-swss repo, its binary is inside iccpd container and binary use platform env var https://github.com/Azure/sonic-swss/blob/ef6b5d48297f7f19a8ec57e2d7b997fd0919bb9d/mclagsyncd/mclaglink.cpp#L191

But platform env is set only inside swss container services, for example https://github.com/Azure/sonic-buildimage/blob/7d40384c58848c368ecf7b2c520f7a17fcbc8bdb/dockers/docker-orchagent/orchagent.sh#L7

So code doesn't support branch with port isolation group, because env is not set.

How I did it

Added env to iccpds start script, like in orchagent

How to verify it

  • build sonic with iccpd
  • configure mclag
  • wait iccpd bring up
  • /var/log/syslog must contains row with text Delete all isolation group destination ports, hence code branch with port isolation group was used instead of ACL

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

@globaltrouble
Copy link
Contributor Author

@gitsabari note, previously code branch with port isolation group was dead and port isolation group was not used.

@globaltrouble
Copy link
Contributor Author

@lguohan could you please review it?

@globaltrouble
Copy link
Contributor Author

@qiluo-msft could you please review it?

@globaltrouble
Copy link
Contributor Author

@lguohan could you please review it?

@globaltrouble
Copy link
Contributor Author

@qiluo-msft could you please review it?

@globaltrouble
Copy link
Contributor Author

@gechiang could you please review it?

@akokhan
Copy link
Contributor

akokhan commented Nov 15, 2021

@gitsabari, @Praveen-Brcm , please take a look

@globaltrouble
Copy link
Contributor Author

@lguohan could you please help to review it?

@gechiang
Copy link
Collaborator

gechiang commented Dec 8, 2021

Looks good to me. But let's wait for @gitsabari, @Praveen-Brcm to review and approve.

@Praveen-Brcm
Copy link
Contributor

Changes looks good to me. Please add me to the reviwers list to approve. Thanks.

@gechiang
Copy link
Collaborator

gechiang commented Dec 8, 2021

Changes looks good to me. Please add me to the reviwers list to approve. Thanks.

@Praveen-Brcm
Looks like I am not able to add you as a reviewer... but since you have already reviewed it and approved, I will go ahead approve it and merge it.
Thanks!

@gitsabari
Copy link
Contributor

changes looks good to me also. Please add me to reviewer list to approve

Copy link
Collaborator

@gechiang gechiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @Praveen-Brcm has also reviewed and approved.

@gechiang gechiang merged commit 969cea0 into sonic-net:master Dec 8, 2021
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

Successfully merging this pull request may close these issues.

5 participants