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.

Sign up for GitHub

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

Jump to bottom

[dash-pipeline] Refactor Makefiles #432

Merged
merged 3 commits into from
Sep 7, 2023
Merged

[dash-pipeline] Refactor Makefiles #432

merged 3 commits into from
Sep 7, 2023

Conversation

kcudnik
Copy link
Collaborator

@kcudnik kcudnik commented Sep 1, 2023

  • compile everything with host user id and group
  • reduce number of required libs
  • simplify Makefiles
  • make proper dependencies

@kcudnik
[dash-pipeline] Refactor Makefiles
bc83e7b 
* compile everything with host user id and group
* reduce number of required libs
* simplify Makefiles
* make proper dependencies
@kcudnik kcudnik mentioned this pull request Sep 1, 2023
Merged
@kcudnik
Copy link
Collaborator Author

kcudnik commented Sep 1, 2023

@chrispsommers can you take a look at this error https://github.com/sonic-net/DASH/actions/runs/6052818653/job/16427025685?pr=432#step:15:275 ?? i have no clue why this fails, make docker-saithrift-client passes locally on my machine, make all, run-server and run all tests, all that passes, but here im a little bit lost

@kcudnik
Copy link
Collaborator Author

kcudnik commented Sep 1, 2023 •
edited
Loading 

#10 5.873 python3: can't open file 'setup.py': [Errno 2] No such file or directory
#10 ERROR: process "/bin/sh -c cd /saithrift-0.9 &&     sudo python3 setup.py install &&     cd / &&     sudo rm -rf saithrift-0.9 &&    cd thrift-0.11.0 &&     sudo python3 setup.py install &&     cd / &&    sudo rm -rf thrift-0.11.0 &&     cd /SAI/test/ptf &&     sudo python3 setup.py install" did not complete successfully: exit code: 2
------
 > [6/9] RUN cd /saithrift-0.9 &&     sudo python3 setup.py install &&     cd / &&     sudo rm -rf saithrift-0.9 &&    cd thrift-0.11.0 &&     sudo python3 setup.py install &&     cd / &&    sudo rm -rf thrift-0.11.0 &&     cd /SAI/test/ptf &&     sudo python3 setup.py install:

from previous logs seeems like setup.py from saithrift-0.9 and from thrift-0.11.0 was executed ok, and is missing from /SAI/test/ptf, no, but there is no SAI/ptf dir, only SAI/SAI/test/ptf, and it contains setup.py, maybe directory is wrong mounted ? since previously docker-saithrift-client was downloaded and not built from scratch, and somehow this PR triggered to rebuild it

Found it: ahh i missed ptf submodule init damn :D

@kcudnik
Copy link
Collaborator Author

kcudnik commented Sep 1, 2023

btw. those ptf tests, wait for 5 seconds and retry 10x times, which is hudge waist of time:

urn up port 281474[97](https://github.com/sonic-net/DASH/actions/runs/6053205361/job/16428197918?pr=432#step:17:98)6710658
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.

468 time.sleep(5)
469 print("port {}:{} is not up, status: {}. Retry. Waiting for Admin State Up.".format

SAI/ptf/sai_base_test.py

opencomputeproject/SAI#1677 @ravi861 please explain this sleeps if they are needed, they slow down out tests, waiting and retrying all time https://github.com/opencomputeproject/SAI/pull/1677/files#diff-79e5a6916a2dff14d4fe5f385b6d5bb183adad2f4e9ec02d3ced4c3030b73940R318

@kcudnik kcudnik requested a review from lguohan September 3, 2023 16:45
@kcudnik kcudnik added the enhancement New feature or request label Sep 5, 2023
@chrispsommers
Copy link
Collaborator

@ravi861 Please look into these sleeps, it's made the tests take very very long.

chrispsommers
chrispsommers reviewed Sep 6, 2023
View reviewed changes
Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

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

Please see comments, I'm unsure of the side-effect of adding SUDOs to makefiles.

@kcudnik
Copy link
Collaborator Author

kcudnik commented Sep 6, 2023

Please see comments, I'm unsure of the side-effect of adding SUDOs to makefiles.

should be none, if something appears, we will update

@ravi861
Copy link

ravi861 commented Sep 6, 2023

btw. those ptf tests, wait for 5 seconds and retry 10x times, which is hudge waist of time:

urn up port 281474[97](https://github.com/sonic-net/DASH/actions/runs/6053205361/job/16428197918?pr=432#step:17:98)6710658
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.
port 1:281474976710658 is not up, status: 0. Retry. Waiting for Admin State Up.

468 time.sleep(5) 469 print("port {}:{} is not up, status: {}. Retry. Waiting for Admin State Up.".format

SAI/ptf/sai_base_test.py

opencomputeproject/SAI#1677 @ravi861 please explain this sleeps if they are needed, they slow down out tests, waiting and retrying all time https://github.com/opencomputeproject/SAI/pull/1677/files#diff-79e5a6916a2dff14d4fe5f385b6d5bb183adad2f4e9ec02d3ced4c3030b73940R318

@kcudnik A simple solution for this in dash repo without affecting SAI is to add a fixed API impl in get port attribute SAI_PORT_ATTR_OPER_STATUS as UP always here.
https://github.com/sonic-net/DASH/blob/main/dash-pipeline/SAI/templates/dashsai.cpp.j2#L427

@kcudnik
Copy link
Collaborator Author

kcudnik commented Sep 6, 2023

@kcudnik A simple solution for this in dash repo without affecting SAI is to add a fixed API impl in get port attribute SAI_PORT_ATTR_OPER_STATUS as UP always here. https://github.com/sonic-net/DASH/blob/main/dash-pipeline/SAI/templates/dashsai.cpp.j2#L427

this seems like a workaround to me, but also it may not be that simple since bmv2 uses actual eth ports to forward trafiic so the status could be bound to them too

@ravi861
Copy link

ravi861 commented Sep 6, 2023

From what I know, bmv2 works using veth pairs which are usually always brought up because bmv2 itself expects them to be up before it initializes.
From the CI logs you pointed me to, I never see below line printed. So it seems ports are never created in a bmv2 context.
https://github.com/opencomputeproject/SAI/blob/master/ptf/sai_base_test.py#L757
My recommendation is we use the suggested workaround as the solution and check if it helps alleviates the delay.

@chrispsommers
Copy link
Collaborator

From what I know, bmv2 works using veth pairs which are usually always brought up because bmv2 itself expects them to be up before it initializes.

True, the veth ports are already up before the bmv2 swtich is run.

@kcudnik
Copy link
Collaborator Author

kcudnik commented Sep 7, 2023

From what I know, bmv2 works using veth pairs which are usually always brought up because bmv2 itself expects them to be up before it initializes.

True, the veth ports are already up before the bmv2 swtich is run.

ok, i can add code that will always return true, on each port, but then could be modified to reflect actual interface status, let see if that will help to speed up test execution

@kcudnik
[libsai] Return SAI_PORT_OPER_STATUS_UP on each port
ac2ec27 
Speeds up ptf test execution, since all ports are already up
@kcudnik
Copy link
Collaborator Author

kcudnik commented Sep 7, 2023

@chrispsommers please review and merge when ready

chrispsommers
chrispsommers approved these changes Sep 7, 2023
View reviewed changes
Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

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

LGTM, excellent improvements!

@chrispsommers chrispsommers merged commit 3bbe99f into sonic-net:main Sep 7, 2023
10 checks passed
@kcudnik kcudnik deleted the main1 branch September 7, 2023 18:40
@kcudnik
Copy link
Collaborator Author

kcudnik commented Sep 8, 2023

LGTM, excellent improvements!

https://www.youtube.com/watch?v=rR4n-0KYeKQ

@kcudnik kcudnik mentioned this pull request Sep 8, 2023
Closed
clarklee-guizhao added a commit to clarklee-guizhao/DASH that referenced this pull request Dec 3, 2023
@clarklee-guizhao
Merge branch 'main' into nat-hld
d5e6a83 
* main: (75 commits)
  [dash-SAI] Enable warnings as errors (sonic-net#466)
  [SAI] wrong code generated in libsai sonic-net#415 (sonic-net#463)
  Fix incorrect IP in SONiC-DASH HLD VNET to VNET example. (sonic-net#459)
  DASH pipeline packet flow update proposal. (sonic-net#449)
  [libsai] Add attr name logging when doing get api (sonic-net#451)
  Build libsai deb packages in github workflow (sonic-net#450)
  Add Private Link mapping (sonic-net#327)
  [SAI] Update SAI submodule to the latest origin/master (sonic-net#446)
  [dash] Add libsai-debs target to create libsai debian packages (sonic-net#444)
  update p4 compile dependancy to avoid parallel  docker runs (sonic-net#443)
  [dash] Refactor libsai (sonic-net#438)
  [dash] Update SAI to latest v1.13 (sonic-net#435)
  [dash-pipeline] Refactor Makefiles (sonic-net#432)
  Remove ACL tags from BM (sonic-net#425)
  [submodule] Update SAI submodule to origin/master (sonic-net#431)
  [sai-api-gen] Write files only when changes are detected (sonic-net#429)
  Adds SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION api to dash_underlay_routing (sonic-net#422)
  [SAI] Add missing check for api initialized
  [SAI] Print oids in hex form
  [SAI] Change asserts to return error codes and add missing switch api
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrispsommers chrispsommers chrispsommers approved these changes

@lguohan lguohan Awaiting requested review from lguohan

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kcudnik @chrispsommers @ravi861