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

[test] fix potential backbone name conflict when running cert suite #10596

Merged
merged 12 commits into from
Aug 13, 2024

Conversation

zesonzhang
Copy link
Contributor

@zesonzhang zesonzhang commented Aug 9, 2024

background

#10550 has introduced a new way to support multiple backbone nework in otbr docker test. Though it works good while running a single test, a bug exists when running cert-suite, which runs a batch of tests in parallel.

cert-suite allocates the name of the backbone interfaces dynamically by setting env PORT_OFFSET for each test, so there is potentially conflict exists if we hard code the backbone_network name in TOPOLOGY. This PR is targeting to fix this potential naming conflict.

how to fix

We fix it by assigning a number for backbone_network_id in each BR definition in TOPOLOGY, instead of setting a fixed backbone network name. The final backbone network name is decided by both PORT_OFFSET env and the number of backbone_network (in backbone{PORT_OFFSET}.{backbone_network} format)

For example, if PORT_OFFSET is 0 and backbone_network_id is 1, then backbone network name will be backbone0.1. For the tests that only use one backbone network and the backbone_network_id is not given, the backbone network name is by default backbone{PORT_OFFSET}.0.

New test case format

class NewTestCase(thread_cert.TestCase):
    ...
    BR = 1
    ...
    TOPOLOGY = {
        BR: {
            ...
	    'is_otbr': True,
	    'backbone_network_id': <backbone-id>,
	    ...
	}
	...
    }
    ...

<backbone-id> is any integer from 0, for each BR inside a single test, if <backbone-id> is different, the BR use different backbone interfaces; the same <backbone-id> inside a single test case means the same backbone network interface.

'backbone_network_id': <backbone-id> is optional for single backbone test cases, when it's not given while defining a otbr node, the backbone is default as backbone{PORT_OFFSET}.0.

For developers, if you are defining a new test which has multiple backbone interfaces, please ensure backbone_network_id is explicitly defined in each BR, otherwize an error is reported.

…LOGY

when running cert suite, multiple tests are running in parallel, the
extract backbone name is allocated by cert suite script based on
PORT_OFFSET.

For example, if PORT_OFFSET is 10, the default backbone network name is
"backbone10". This commit targets to defineing multiple backbone
networks inside a single tests. For examples, in test_multi_ail.py, if
PORT_OFFSET is 10, then "backbone10.0" and "backbone10.1" are created.

Also, then subnet prefix of the network format is chagnes to:
 backbone10.0 -> 9100:0::0/64 (9100::0/64)
 backbone10.1 -> 9100:1::0/64
BACKBONE_IPV6_ADDR_START is originally defined as the interger which
value is the sum of 0x9100 and PORT_OFFSET

This commit define BACKBONE_IPV6_ADDR_START as the string value.
@zesonzhang zesonzhang changed the title Test/fix backbone conflict [test] fix potential backbone name conflict when running cert suite Aug 9, 2024
Copy link

size-report bot commented Aug 9, 2024

Size Report of OpenThread

Merging #10596 into main(ddbc99b).

name branch text data bss total
ot-cli-ftd main 467752 856 66364 534972
#10596 467752 856 66364 534972
+/- 0 0 0 0
ot-ncp-ftd main 437076 760 61592 499428
#10596 437076 760 61592 499428
+/- 0 0 0 0
libopenthread-ftd.a main 236728 95 40302 277125
#10596 236728 95 40302 277125
+/- 0 0 0 0
libopenthread-cli-ftd.a main 57973 0 8075 66048
#10596 57973 0 8075 66048
+/- 0 0 0 0
libopenthread-ncp-ftd.a main 32541 0 5924 38465
#10596 32541 0 5924 38465
+/- 0 0 0 0
ot-cli-mtd main 365584 760 51236 417580
#10596 365584 760 51236 417580
+/- 0 0 0 0
ot-ncp-mtd main 348276 760 46480 395516
#10596 348276 760 46480 395516
+/- 0 0 0 0
libopenthread-mtd.a main 159091 0 25190 184281
#10596 159091 0 25190 184281
+/- 0 0 0 0
libopenthread-cli-mtd.a main 39795 0 8059 47854
#10596 39795 0 8059 47854
+/- 0 0 0 0
libopenthread-ncp-mtd.a main 25349 0 5924 31273
#10596 25349 0 5924 31273
+/- 0 0 0 0
ot-cli-ftd-br main 552528 864 131228 684620
#10596 552528 864 131228 684620
+/- 0 0 0 0
libopenthread-ftd-br.a main 325655 100 105142 430897
#10596 325655 100 105142 430897
+/- 0 0 0 0
libopenthread-cli-ftd-br.a main 72104 0 8099 80203
#10596 72104 0 8099 80203
+/- 0 0 0 0
ot-rcp main 62504 564 20636 83704
#10596 62504 564 20636 83704
+/- 0 0 0 0
libopenthread-rcp.a main 9750 0 5060 14810
#10596 9750 0 5060 14810
+/- 0 0 0 0
libopenthread-radio.a main 19257 0 222 19479
#10596 19257 0 222 19479
+/- 0 0 0 0

@zesonzhang zesonzhang force-pushed the test/fix-backbone-conflict branch from fa1c54d to da500fd Compare August 9, 2024 04:43
@zesonzhang
Copy link
Contributor Author

zesonzhang commented Aug 9, 2024

To Reviewers: I have separated this PR into multiple small commits which can make it much easy to review. Please kindly review by each commit. Thanks!

The link to the first commit: f2fba25

@bukepo @superwhd

@zesonzhang zesonzhang marked this pull request as ready for review August 9, 2024 04:48
@zesonzhang zesonzhang requested review from bukepo and superwhd August 9, 2024 04:49
tests/scripts/thread-cert/node.py Outdated Show resolved Hide resolved
tests/scripts/thread-cert/thread_cert.py Outdated Show resolved Hide resolved
tests/scripts/thread-cert/thread_cert.py Outdated Show resolved Hide resolved
@zesonzhang zesonzhang force-pushed the test/fix-backbone-conflict branch from da500fd to ac738f9 Compare August 12, 2024 02:47
@zesonzhang zesonzhang requested a review from bukepo August 12, 2024 03:11
tests/scripts/thread-cert/thread_cert.py Outdated Show resolved Hide resolved
tests/scripts/thread-cert/thread_cert.py Outdated Show resolved Hide resolved
tests/scripts/thread-cert/thread_cert.py Outdated Show resolved Hide resolved
tests/scripts/thread-cert/thread_cert.py Outdated Show resolved Hide resolved
tests/scripts/thread-cert/thread_cert.py Outdated Show resolved Hide resolved
@zesonzhang
Copy link
Contributor Author

@bukepo @superwhd PR is updated and ready for review, please kindly take a look, thanks!

@zesonzhang zesonzhang requested a review from superwhd August 12, 2024 08:26
tests/scripts/thread-cert/thread_cert.py Show resolved Hide resolved
tests/scripts/thread-cert/thread_cert.py Outdated Show resolved Hide resolved
tests/scripts/thread-cert/thread_cert.py Outdated Show resolved Hide resolved
tests/scripts/thread-cert/thread_cert.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Irving-cl Irving-cl left a comment

Choose a reason for hiding this comment

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

LGTM. Some small suggestions.

tests/scripts/thread-cert/thread_cert.py Show resolved Hide resolved
tests/scripts/thread-cert/thread_cert.py Outdated Show resolved Hide resolved
@zesonzhang zesonzhang force-pushed the test/fix-backbone-conflict branch from 702b8f5 to 52baf80 Compare August 13, 2024 02:30
@zesonzhang zesonzhang requested a review from superwhd August 13, 2024 03:12
@zesonzhang
Copy link
Contributor Author

@Irving-cl @superwhd @bukepo This PR is ready to be reviewed, please kindly take a look, thank!

Copy link
Contributor

@superwhd superwhd left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Left a nonblocking minor question.

tests/scripts/thread-cert/thread_cert.py Show resolved Hide resolved
tests/scripts/thread-cert/node.py Outdated Show resolved Hide resolved
tests/scripts/thread-cert/node.py Outdated Show resolved Hide resolved
tests/scripts/thread-cert/node.py Outdated Show resolved Hide resolved
@@ -500,9 +520,6 @@ def _parse_params(self, params: Optional[dict]) -> dict:
assert params.get('version', '') == '', params
params['version'] = ''

# set default backbone network for bbr/otbr/host if not specified
params.setdefault('backbone_network', config.BACKBONE_DOCKER_NETWORK_NAME)
Copy link
Member

Choose a reason for hiding this comment

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

keeping this line to ensure it will never be None, which can simply the _construct_backbone_network_name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The backbone_network_id is None for non otbr/host nodes by design in this PR. It can be strange (though no harm) to assign a default backbone network id for a non-BR node. Any thoughts?

@zesonzhang zesonzhang force-pushed the test/fix-backbone-conflict branch from d41ecb0 to 4a6c216 Compare August 13, 2024 08:10
@zesonzhang zesonzhang requested a review from jwhui August 13, 2024 09:32
@zesonzhang
Copy link
Contributor Author

Hi @jwhui , this PR is ready to be merged as a single commit, thanks!

@jwhui jwhui merged commit 509596f into openthread:main Aug 13, 2024
103 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants