Skip to content

[chassis] Enhance copp to work for chassis#5097

Merged
abdosi merged 28 commits intosonic-net:masterfrom
abdosi:copp
Feb 2, 2023
Merged

[chassis] Enhance copp to work for chassis#5097
abdosi merged 28 commits intosonic-net:masterfrom
abdosi:copp

Conversation

@abdosi
Copy link
Copy Markdown
Contributor

@abdosi abdosi commented Feb 9, 2022

What I did:

  1. Added change to work for chassis by Minigraph facts from namespace rather than relying on Host Facts
  2. Make sure to get correct Port to PTF Index mapping.
  3. Added Support to swap syncd also.

Todo:

Enhance the newly added test case test_add_new_trap , test_remove_trap and test_trap_config_save_after_reboot to work for multi-asic enviroment. Skipping them for now

How I verify:

Verified on Chassis.

@abdosi abdosi requested a review from a team as a code owner February 9, 2022 01:01
@abdosi abdosi requested a review from arlakshm February 9, 2022 01:01
@abdosi
Copy link
Copy Markdown
Contributor Author

abdosi commented Feb 11, 2022

cc @vperumal

@abdosi abdosi changed the title [chassis-packet] Enhance copp to work for chassis [chassis] Enhance copp to work for chassis Feb 12, 2022
@abdosi
Copy link
Copy Markdown
Contributor Author

abdosi commented Jan 27, 2023

@judyjoseph @arlakshm please review. Have updated changes.

@abdosi
Copy link
Copy Markdown
Contributor Author

abdosi commented Jan 27, 2023

cc @vperumal this PR is ready.

Comment thread tests/copp/test_copp.py Outdated
@pytest.fixture(scope="function", autouse=False)
def backup_restore_config_db(duthosts, enum_rand_one_per_hwsku_frontend_hostname):
duthost = duthosts[enum_rand_one_per_hwsku_frontend_hostname]
if duthost.is_multi_asic:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like this check/return here is causing few tests where we skipped for multi_asic -- to exit with the following error,

31/01/2023 21:34:51 __init__._fixture_generator_decorator    L0071 INFO   | -------------------- fixture backup_restore_config_db setup starts --------------------
31/01/2023 21:34:51 __init__._fixture_generator_decorator    L0078 ERROR  |
StopIteration()
Traceback (most recent call last):
  File "sonic-mgmt-int/tests/common/plugins/log_section_start/__init__.py", line 74, in _fixture_generator_decorator
    res = next(it)
StopIteration
31/01/2023 21:34:51 __init__._fixture_generator_decorator    L0079 INFO   | -------------------- fixture backup_restore_config_db setup ends --------------------

maybe we can SKIP them via test_mark_conditions file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated. test_mark_conditions file i don;t think we can do as we skip only for multi-asic dut which is run-time property.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@judyjoseph : updated to use tests_mark_conditions.yaml

Comment thread tests/copp/test_copp.py Outdated
# Add Rule to communicate to http/s proxy from namespace
dut.command("sudo iptables -t nat -A POSTROUTING -p tcp --dport 8080 -j SNAT --to-source {}".format(mgmt_ip))
if http_proxy != https_proxy:
dut.command("sudo ip -n {} rule add from all to {} pref 3 lookup default".format(test_params.nn_target_namespace, https_proxy))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we not add the perf 1 and perf 2 rules that was there before ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch @judyjoseph . We don;t need all this change now.

@abdosi abdosi added the multi-ASIC Multi ASIC platform label Feb 1, 2023
Comment thread tests/copp/test_copp.py Outdated
Comment on lines +120 to +121
if duthost.is_multi_asic:
pytest.skip("Skipping as test needs changes to supoort multi-asic dut")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we add conditional mark iso for skipping here?
Example:

qos/test_buffer_traditional.py:
skip:
reason: "Buffer traditional test is only supported 201911 branch and not yet supported on multi-ASIC platform."
conditions:
- "is_multi_asic==True or release not in ['201911']"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks @arlakshm . updated.

Signed-off-by: Abhishek Dosi <[email protected]>
@abdosi abdosi merged commit cc26308 into sonic-net:master Feb 2, 2023
@abdosi abdosi deleted the copp branch February 2, 2023 16:59
wangxin pushed a commit that referenced this pull request Feb 3, 2023
sanmalho-git added a commit to sanmalho-git/sonic-mgmt that referenced this pull request Feb 8, 2023
QoS multi-asic tests are failing when run against a multi-asic DUT.

Reason:
PR sonic-net#5097 introduced namespace with default value of DEFAULT_NAMESPACE in swap_syncd in tests/common/system_utils/docker.py
For multi-asic DUT, if namespace is not specified, then syncd is only swapped on the first asic of DUT.
Since, the PR did not update swapSyncd in tests/conftest.py to pass any value to namespace, syncd is swapped only on asic0.

Fix:
For backward compatability, modified swap_syncd that if DUT is multi asic and namespace is DEFAULT_NAMESPACE, then
swap syncd on all the asics in the DUT
arlakshm pushed a commit that referenced this pull request Feb 15, 2023
#7412)

QoS multi-asic tests are failing when run against a multi-asic DUT.

Reason:
PR #5097 introduced namespace with default value of DEFAULT_NAMESPACE in swap_syncd in tests/common/system_utils/docker.py
For multi-asic DUT, if namespace is not specified, then syncd is only swapped on the first asic of DUT.
Since, the PR did not update swapSyncd in tests/conftest.py to pass any value to namespace, syncd is swapped only on asic0.

Fix:
For backward compatability, modified swap_syncd that if DUT is multi asic and namespace is DEFAULT_NAMESPACE, then
swap syncd on all the asics in the DUT
wangxin pushed a commit that referenced this pull request Feb 17, 2023
#7412)

QoS multi-asic tests are failing when run against a multi-asic DUT.

Reason:
PR #5097 introduced namespace with default value of DEFAULT_NAMESPACE in swap_syncd in tests/common/system_utils/docker.py
For multi-asic DUT, if namespace is not specified, then syncd is only swapped on the first asic of DUT.
Since, the PR did not update swapSyncd in tests/conftest.py to pass any value to namespace, syncd is swapped only on asic0.

Fix:
For backward compatability, modified swap_syncd that if DUT is multi asic and namespace is DEFAULT_NAMESPACE, then
swap syncd on all the asics in the DUT
kellyyeh pushed a commit to kellyyeh/sonic-mgmt that referenced this pull request Mar 31, 2023
kellyyeh pushed a commit to kellyyeh/sonic-mgmt that referenced this pull request Mar 31, 2023
sonic-net#7412)

QoS multi-asic tests are failing when run against a multi-asic DUT.

Reason:
PR sonic-net#5097 introduced namespace with default value of DEFAULT_NAMESPACE in swap_syncd in tests/common/system_utils/docker.py
For multi-asic DUT, if namespace is not specified, then syncd is only swapped on the first asic of DUT.
Since, the PR did not update swapSyncd in tests/conftest.py to pass any value to namespace, syncd is swapped only on asic0.

Fix:
For backward compatability, modified swap_syncd that if DUT is multi asic and namespace is DEFAULT_NAMESPACE, then
swap syncd on all the asics in the DUT
kazinator-arista pushed a commit to kazinator-arista/sonic-mgmt that referenced this pull request Mar 4, 2026
<!--
 Please make sure you've read and understood our contributing guidelines:
 https://github.com/Azure/SONiC/blob/gh-pages/CONTRIBUTING.md

 failure_prs.log skip_prs.log Make sure all your commits include a signature generated with `git commit -s` **

 If this is a bug fix, make sure your description includes "fixes #xxxx", or
 "closes #xxxx" or "resolves #xxxx"

 Please provide the following information:
-->

#### Why I did it

The build fails if kdump is enabled on the build host, even though the relevant build step is performed in a dockerized chroot.

```
+ sudo LANG=C chroot ./fsroot-cisco-8000 kdump-config symlinks 5.10.0-23-2-amd64
Cannot change symbolic links when kdump is loaded ... failed!
```

Fixes sonic-net#5097, Fixes sonic-net#17023

##### Work item tracking
- Microsoft ADO **(number only)**:

#### How I did it

kdump installation checks if kdump is already running and aborts if so. This is good in most cases, but it's not relevant when installing into a chroot inside a docker container. This adds a basic patch to disable this check during build.

Note that the kdump status of the build host is imported into the docker build container via the sysfs file system:

```
$ ls -id /sys/kernel/kexec_crash_loaded && cat /sys/kernel/kexec_crash_loaded
7824 /sys/kernel/kexec_crash_loaded
0

$ docker run --rm debian bash -c "ls -id /sys/kernel/kexec_crash_loaded && cat /sys/kernel/kexec_crash_loaded"
7824 /sys/kernel/kexec_crash_loaded
0
```

The inodes and file content are identical inside and outside of the container.

#### How to verify it

<!--
If PR needs to be backported, then the PR must be tested against the base branch and the earliest backport release branch and provide tested image version on these two branches. For example, if the PR is requested for master, 202211 and 202012, then the requester needs to provide test results on master and 202012.
-->

1. Enable kdump on the build host
2. Confirm baseline build fails with the "Cannot change symbolic links when kdump is loaded" error
3. Apply this change and build succeeds

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

<!--
- Note we only backport fixes to a release branch, *not* features!
- Please also provide a reason for the backporting below.
- e.g.
- [x] 202006
-->

- [x] 202405
- [x] 202411
- [x] 202505

#### Tested branch (Please provide the tested image version)

<!--
- Please provide tested image version
- e.g.
- [x] 20201231.100
-->

master (2975205)

#### Description for the changelog
<!--
Write a short (one line) summary that describes the changes in this
pull request for inclusion in the changelog:
-->

[build] fix build failure on kdump-enabled hosts

<!--
 Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.
-->

#### Link to config_db schema for YANG module changes
<!--
Provide a link to config_db schema for the table for which YANG model
is defined
Link should point to correct section on https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-yang-models/doc/Configuration.md
-->

#### A picture of a cute animal (not mandatory but encouraged)
kazinator-arista pushed a commit to kazinator-arista/sonic-mgmt that referenced this pull request Mar 4, 2026
sonic-net#23035)

<!--
 Please make sure you've read and understood our contributing guidelines:
 https://github.com/Azure/SONiC/blob/gh-pages/CONTRIBUTING.md

 failure_prs.log skip_prs.log Make sure all your commits include a signature generated with `git commit -s` **

 If this is a bug fix, make sure your description includes "fixes #xxxx", or
 "closes #xxxx" or "resolves #xxxx"

 Please provide the following information:
-->

#### Why I did it

The build fails if kdump is enabled on the build host, even though the relevant build step is performed in a dockerized chroot.

```
+ sudo LANG=C chroot ./fsroot-cisco-8000 kdump-config symlinks 5.10.0-23-2-amd64
Cannot change symbolic links when kdump is loaded ... failed!
```

Fixes sonic-net#5097, Fixes sonic-net#17023

##### Work item tracking
- Microsoft ADO **(number only)**:

#### How I did it

kdump installation checks if kdump is already running and aborts if so. This is good in most cases, but it's not relevant when installing into a chroot inside a docker container. This adds a basic patch to disable this check during build.

Note that the kdump status of the build host is imported into the docker build container via the sysfs file system:

```
$ ls -id /sys/kernel/kexec_crash_loaded && cat /sys/kernel/kexec_crash_loaded
7824 /sys/kernel/kexec_crash_loaded
0

$ docker run --rm debian bash -c "ls -id /sys/kernel/kexec_crash_loaded && cat /sys/kernel/kexec_crash_loaded"
7824 /sys/kernel/kexec_crash_loaded
0
```

The inodes and file content are identical inside and outside of the container.

#### How to verify it

<!--
If PR needs to be backported, then the PR must be tested against the base branch and the earliest backport release branch and provide tested image version on these two branches. For example, if the PR is requested for master, 202211 and 202012, then the requester needs to provide test results on master and 202012.
-->

1. Enable kdump on the build host
2. Confirm baseline build fails with the "Cannot change symbolic links when kdump is loaded" error
3. Apply this change and build succeeds

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

<!--
- Note we only backport fixes to a release branch, *not* features!
- Please also provide a reason for the backporting below.
- e.g.
- [x] 202006
-->

- [x] 202405
- [x] 202411
- [x] 202505

#### Tested branch (Please provide the tested image version)

<!--
- Please provide tested image version
- e.g.
- [x] 20201231.100
-->

master (2975205)

#### Description for the changelog
<!--
Write a short (one line) summary that describes the changes in this
pull request for inclusion in the changelog:
-->

[build] fix build failure on kdump-enabled hosts

<!--
 Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.
-->

#### Link to config_db schema for YANG module changes
<!--
Provide a link to config_db schema for the table for which YANG model
is defined
Link should point to correct section on https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-yang-models/doc/Configuration.md
-->

#### A picture of a cute animal (not mandatory but encouraged)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants