[multi-asic] Fixed systemd-sonic-generator for multi-asic by anamehra · Pull Request #7633 · sonic-net/sonic-buildimage

anamehra · 2021-05-17T23:33:58Z

Signed-off-by: Anand Mehra [email protected]

Why I did it

systemd-sonic-generator limits multi-asic unit file instances to 10 (single digit instance number 0 - 10). This limitation needs to be removed to handle more than 10 asics.
MAX_NUM_TARGETS and MAX_NUM_INSTALL_LINES limits to 15 which is not sufficient for systems with more than 15 asics.
Inside get_unit_files(), strcmp produce incorrect results due to non null terminated string being compared.

How I did it

1. In function insert_instance_number instance_string was malloced for 2 char
   size which was limiting the instance number value in instance_name to 1 digit.
   Fixed insert_instance_number to use asprintf to generate instancd_name for
   any number of instances. Added _GNU_SOURCE to CFLAGS for asprintf.

2. Fixed get_unit_files() to use calloc instead of malloc. Uninitialized memory
   was causing incorrect string mismatch error while comparing unit file name
   string.

3. Increased MAX_NUM_TARGETS and MAX_NUM_INSTALL_LINES values to 48 to handle more
   asic instances.

4. Added build UT support for systemd-sonic-generator:
    a. Refactor systemd-sonic-generator.c to be used with UT infra.
    b. Added UT infra to run build UT for systemd-sonic-generator
    c. Added functional level and program level UT class and test cases.

How to verify it

On a single asic system:

Verify Unit file symlinks at following path for single instance system:
/run/systemd/generator/multi-user.target.wants/
/run/systemd/generator/sonic.target.wants/
/run/systemd/generator/swss.service.wants/

On a multi asic system:

Verify Unit file symlinks at following path for multi instance system:
/run/systemd/generator/multi-user.target.wants/
/run/systemd/generator/sonic.target.wants/
/run/systemd/generator/[email protected]/ where n is instance num 0 - (num_asic-1)

Build UT logs:
systemd-sonic-generator_1.0.0_amd64.deb.log

Single asic system logs:
Single_asic.logs.txt

Multi asic system logs:
multi-asic.logs.txt

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

201811
201911
202006
202012

Description for the changelog

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

1. In function insert_instance_number instance_string was malloced for 2 char size which was limiting the instance number value in instance_name to 1 digit. Fixed insert_instance_number to use asprintf to generate instancd_name for any number of instances. Added _GNU_SOURCE to CFLAGS for asprintf. 2. Fixed get_unit_files() to use calloc instead of malloc. Uninitialized memory was causing incorrect string mismatch error while comparing unit file name string. 3. Increased MAX_NUM_TARGETS and MAX_NUM_INSTALL_LINES values to 48 to handle more asic instances. 4. Added build UT support for systemd-sonic-generator: a. Refactor systemd-sonic-generator.c to be used with UT infra. b. Added UT infra to run build UT for systemd-sonic-generator c. Added functional level and program level UT class and test cases. Signed-off-by: Anand Mehra <[email protected]>

ghost · 2021-05-17T23:34:11Z

All CLA requirements met.

jleveque

As comments.

And thank you for adding the unit tests!

src/systemd-sonic-generator/ssg-test.cc

src/systemd-sonic-generator/systemd-sonic-generator.c

lguohan · 2021-05-18T19:43:14Z

@theasianpianist to check

src/systemd-sonic-generator/systemd-sonic-generator.c

1. Explicitly setting global pointers to NULL in definitions. 2. Added a space before ": public" in class definitions to align style with SONiC C++ files. Signed-off-by: Anand Mehra <[email protected]>

Signed-off-by: Anand Mehra <[email protected]>

anamehra · 2021-05-19T19:40:07Z

Hi @lguohan , The build errors reported after new commit does not look like related. Were these observed earlier? May we rerun the builds? Thanks

theasianpianist · 2021-05-19T21:42:24Z

/Azp run

azure-pipelines · 2021-05-19T21:42:46Z

Azure Pipelines successfully started running 1 pipeline(s).

abdosi · 2021-05-25T19:54:38Z

/azp run

azure-pipelines · 2021-05-25T19:54:55Z

Azure Pipelines successfully started running 1 pipeline(s).

anamehra · 2021-05-27T07:54:51Z

/azp run

azure-pipelines · 2021-05-27T07:54:56Z

Commenter does not have sufficient privileges for PR 7633 in repo Azure/sonic-buildimage

anshuv-mfst · 2021-05-28T00:17:37Z

Hi @theasianpianist - could you please initiate azp run.

theasianpianist · 2021-05-28T01:11:13Z

/Azp run

azure-pipelines · 2021-05-28T01:11:29Z

Azure Pipelines successfully started running 1 pipeline(s).

anamehra · 2021-05-28T17:35:28Z

Thanks @theasianpianist for re-executing pipeline.
@anshuv-mfst As all build/test pass, could this be merged or do we need more approvals?

anshuv-mfst · 2021-05-28T17:37:39Z

@theasianpianist - thanks, could you please review and approve, kindly help with merge.

lguohan · 2021-06-09T15:20:14Z

revert this pr, please fix the armhf build.

)" This reverts commit 4f8dec9.

anamehra · 2021-06-21T20:54:10Z

Hi @lguohan , @theasianpianist , the build issue with armfh was because the static gtest lib was not being pacakged. I have fixed it by using -lgtest which works for both arm64 and armfh builds.

I was wondering if I should split this PR in two PRs, one for actual systemd-sonic-generator changes to required multi-asic enhancements and the other for UT code enhancement only. Please advise.

theasianpianist · 2021-06-21T21:29:51Z

@anamehra unless @lguohan has a preference, I am fine with either

anamehra · 2021-06-23T18:25:31Z

@theasianpianist Created #7954 with only change in Makefile for -lgtest

…7633) * Fixed systemd-sonic-generator for multi-asic 1. In function insert_instance_number instance_string was malloced for 2 char size which was limiting the instance number value in instance_name to 1 digit. Fixed insert_instance_number to use asprintf to generate instancd_name for any number of instances. Added _GNU_SOURCE to CFLAGS for asprintf. 2. Fixed get_unit_files() to use calloc instead of malloc. Uninitialized memory was causing incorrect string mismatch error while comparing unit file name string. 3. Increased MAX_NUM_TARGETS and MAX_NUM_INSTALL_LINES values to 48 to handle more asic instances. 4. Added build UT support for systemd-sonic-generator: a. Refactor systemd-sonic-generator.c to be used with UT infra. b. Added UT infra to run build UT for systemd-sonic-generator c. Added functional level and program level UT class and test cases. * Resolved review comments. 1. Explicitly setting global pointers to NULL in definitions. 2. Added a space before ": public" in class definitions to align style with SONiC C++ files. * Merged strtok_r statements in single command. Signed-off-by: Anand Mehra <[email protected]>

…nic-net#7633)" This reverts commit 4f8dec9.

anamehra requested a review from lguohan as a code owner May 17, 2021 23:33

jleveque suggested changes May 18, 2021

View reviewed changes

jleveque requested a review from theasianpianist May 18, 2021 01:31

jleveque added Enhancement ➕ Multi-ASIC Unit Tests labels May 18, 2021

theasianpianist requested a review from SuvarnaMeenakshi May 18, 2021 19:43

theasianpianist reviewed May 18, 2021

View reviewed changes

src/systemd-sonic-generator/systemd-sonic-generator.c Outdated Show resolved Hide resolved

Resolved review comments.

83638cf

1. Explicitly setting global pointers to NULL in definitions. 2. Added a space before ": public" in class definitions to align style with SONiC C++ files. Signed-off-by: Anand Mehra <[email protected]>

theasianpianist previously approved these changes May 18, 2021

View reviewed changes

Merged strtok_r statements in single command.

c079042

Signed-off-by: Anand Mehra <[email protected]>

anamehra dismissed theasianpianist’s stale review via c079042 May 19, 2021 00:29

jleveque approved these changes May 19, 2021

View reviewed changes

theasianpianist approved these changes May 28, 2021

View reviewed changes

theasianpianist merged commit 4f8dec9 into sonic-net:master May 28, 2021

dflynn-Nokia mentioned this pull request Jun 9, 2021

Build break: marvell-armhf builds on master branch appear to be broken #7835

Closed

lguohan added a commit that referenced this pull request Jun 9, 2021

Revert "[multi-asic] Fixed systemd-sonic-generator for multi-asic (#7633

c7bc0f0

)" This reverts commit 4f8dec9.

judyjoseph added the Request for 202106 Branch label Jun 22, 2021

anamehra deleted the anamehra/master/systemd-sonic-gen branch June 23, 2021 18:24

judyjoseph removed the Request for 202106 Branch label Jul 13, 2021

carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021

Revert "[multi-asic] Fixed systemd-sonic-generator for multi-asic (so…

fc37d5b

…nic-net#7633)" This reverts commit 4f8dec9.

Conversation

anamehra commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why I did it

How I did it

How to verify it

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

Description for the changelog

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

Uh oh!

ghost commented May 17, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jleveque left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lguohan commented May 18, 2021

Uh oh!

Uh oh!

anamehra commented May 19, 2021

Uh oh!

theasianpianist commented May 19, 2021

Uh oh!

azure-pipelines bot commented May 19, 2021

Uh oh!

abdosi commented May 25, 2021

Uh oh!

azure-pipelines bot commented May 25, 2021

Uh oh!

anamehra commented May 27, 2021

Uh oh!

azure-pipelines bot commented May 27, 2021

Uh oh!

anshuv-mfst commented May 28, 2021

Uh oh!

theasianpianist commented May 28, 2021

Uh oh!

azure-pipelines bot commented May 28, 2021

Uh oh!

anamehra commented May 28, 2021

Uh oh!

anshuv-mfst commented May 28, 2021

Uh oh!

lguohan commented Jun 9, 2021

Uh oh!

anamehra commented Jun 21, 2021

Uh oh!

theasianpianist commented Jun 21, 2021

Uh oh!

anamehra commented Jun 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

anamehra commented May 17, 2021 •

edited

Loading

ghost commented May 17, 2021 •

edited by ghost

Loading