close
The Wayback Machine - https://web.archive.org/web/20210117062450/https://github.com/Azure/sonic-mgmt/pull/2195
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

Insert prefix to temp netdev to support multiple add-topo tasks run concurrently #2195

Merged
merged 1 commit into from Oct 27, 2020

Conversation

@gord1306
Copy link
Contributor

@gord1306 gord1306 commented Sep 10, 2020

Description of PR

The temp netdev is name as ethX_t in currenty library, if there are multiple
add-topo jobs run concurrently in a host, these tasks will operate same netdev
and cause the error.

This patch inserts the prefix to temp netdev to avoid the conflict issue.
And to extend retry times to reduce the ifconfig -a report not found error due
the netdev creation/removal

Signed-off-by: Gord Chen [email protected]

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Approach

What is the motivation for this PR?

To support multiple add-topo tasks be performed concurrently in a host

How did you do it?

How did you verify/test it?

perform multiple add-topo jobs

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@microsoft-cla
Copy link

@microsoft-cla microsoft-cla bot commented Sep 10, 2020

CLA assistant check
All CLA requirements met.

@daall daall requested review from yxieca and lguohan Sep 11, 2020
@yxieca
yxieca approved these changes Sep 14, 2020
@yxieca yxieca self-requested a review Sep 14, 2020
@yxieca
Copy link
Collaborator

@yxieca yxieca commented Sep 14, 2020

@gord1306 I think you have a great idea here. But I am a bit worried that prefixing the vm set name might cause interface name to exceed 15-char Linux limit. Is there a way to get something unique but short?

@gord1306
Copy link
Contributor Author

@gord1306 gord1306 commented Sep 14, 2020

. But I am a bit worried that prefixing the vm set name might cause interface name to exceed 15-char Linux limit. Is there a way to get something unique but short?

@yxieca , yes, it indeed a problem, I am also thinking about this issue, maybe shorten to a random value by hash.

@yxieca
Copy link
Collaborator

@yxieca yxieca commented Sep 14, 2020

. But I am a bit worried that prefixing the vm set name might cause interface name to exceed 15-char Linux limit. Is there a way to get something unique but short?

@yxieca , yes, it indeed a problem, I am also thinking about this issue, maybe shorten to a random value by hash.

Yes. You might be able to use system generated tmp file name for this purpose. The key is to make it random but unique.

@gord1306 gord1306 force-pushed the gord1306:temp_intf_name branch from ad542e9 to 7812f25 Sep 17, 2020
@wangxin
Copy link
Contributor

@wangxin wangxin commented Oct 10, 2020

retest this please

Copy link
Contributor

@wangxin wangxin left a comment

LGTM

@wangxin
Copy link
Contributor

@wangxin wangxin commented Oct 13, 2020

retest this please

@gord1306 gord1306 closed this Oct 14, 2020
@gord1306 gord1306 reopened this Oct 14, 2020
@gord1306
Copy link
Contributor Author

@gord1306 gord1306 commented Oct 14, 2020

retest this please
The test result is failed.

fatal: [STR-ACS-VSERV-01]: FAILED! => {"changed": false, "msg": "ret_code=1, error message=b'Cannot find device \"ddd8c3mgmt\"\\n'. cmd=nsenter -t 475823 -n ip link set ddd8c3mgmt up"}

It looks likes the mgmt interface not found in the ptf container. But it's strange, my source doesn't set up the netdev with prefix in the ptf container, only up the netdev without prefix

@wangxin
Copy link
Contributor

@wangxin wangxin commented Oct 15, 2020

It looks likes the mgmt interface not found in the ptf container. But it's strange, my source doesn't set up the netdev with prefix in the ptf container, only up the netdev without prefix

It looks like there is a hidden issue in this change. Could you fix it?

…oncurrently

The temp netdev is name as ethX_t in currenty library, if  there are multiple
add-topo jobs run concurrently, they will operate same netdev and cause error.

This patch insert the prefix to temp netdev to avoid the conflict issue.
And to extend retry times to reduce the ifconfig -a report not found error due
the netdev creatation/removal

Signed-off-by: Gord Chen <[email protected]>
@gord1306 gord1306 force-pushed the gord1306:temp_intf_name branch from 7812f25 to f9fa72d Oct 23, 2020
@gord1306
Copy link
Contributor Author

@gord1306 gord1306 commented Oct 23, 2020

It looks likes the mgmt interface not found in the ptf container. But it's strange, my source doesn't set up the netdev with prefix in the ptf container, only up the netdev without prefix

It looks like there is a hidden issue in this change. Could you fix it?

I observed the testing log seems not remove topology every time, it means the PTF container may already have the mgmt interface. Therefore, I updated the commit, and add one condition check to ignore if the mgmt interface already exists in the PTF container

        if MGMT_PORT_NAME not in self.cntr_ifaces:
            tmp_mgmt_if = hashlib.md5((PTF_NAME_TEMPLATE % self.vm_set_name).encode("utf-8")).hexdigest()[0:6] + MGMT_PORT_NAME
            self.add_br_if_to_docker(mgmt_bridge, PTF_MGMT_IF_TEMPLATE % self.vm_set_name, tmp_mgmt_if)

The new testing result is passed

@wangxin wangxin merged commit 553e9ff into Azure:master Oct 27, 2020
3 checks passed
3 checks passed
LGTM analysis: Python No new or fixed alerts
Details
license/cla All CLA requirements met.
Details
vsimage Build finished. 54 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.