Not optimal build process for DASH repo, SAI and saithrift

[kcudnik]

@chrispsommers hey, i have been playing with building dash repo for couple days, and i noticed that there are a lot of chmod -R a+w or o+w in many directories, mainly in SAI directory, which touches each file, and because of that, make file is rebuilding the same stuff over and over again when it don't need to, and it's really annoying at this point since for example during make all, SAI metadata and libsai is rebuild 3 times, which takes a lot of time and is pointless, i tried to fix it but it's all over the place and current build files and thrift makefiles are complex, that im worried that i will break something here

can we start to fix it ? why chmod wad done in the first place ? it's because docker container user is dash and have no permissions to write user host file when dash repo is located and build? maybe it would be easier to add local host user to bmv2 docker and set it as default user when running docker, then it would have permissions to write those files and chmod would not be needed at all

for example i see this:

  9715951      4 -rw-rw-r--   1 kcudnik   kcudnik      1914 sie 27 12:59 ./Makefile.3rdpty
  9715950     28 -rw-rw-r--   1 kcudnik   kcudnik     25305 sie 27 12:59 ./Makefile
...
  9719459    712 -rw-r--r--   1 4321     4321       725020 sie 27 13:27 ./SAI/SAI/test/saithriftv2/sai_headers.py.bk
  9717973     12 -rw-r--r--   1 4321     4321        11504 sie 27 13:36 ./SAI/SAI/experimental/saiexperimentaldashtag.h
...
  9717986     12 -rw-r--r--   1 4321     4321        12011 sie 27 13:36 ./SAI/lib/utils.h
  9717984      4 -rw-r--r--   1 4321     4321         2901 sie 27 13:36 ./SAI/lib/Makefile
  9717969     40 -rw-r--r--   1 4321     4321        39629 sie 27 13:00 ./bmv2/dash_pipeline.bmv2/dash_pipeline_p4rt.json
  9717971    536 -rw-r--r--   1 4321     4321       548597 sie 27 13:00 ./bmv2/dash_pipeline.bmv2/dash_pipeline.json
  9717968     76 -rw-r--r--   1 4321     4321        75937 sie 27 13:00 ./bmv2/dash_pipeline.bmv2/dash_pipeline.p4i
  9717970     36 -rw-r--r--   1 4321     4321        34680 sie 27 13:00 ./bmv2/dash_pipeline.bmv2/dash_pipeline_p4rt.txt
  9719506    256 -rwxr-xr-x   1 4321     4321       258728 sie 27 13:30 ./tests/libsai/vnet_out/vnet_out
  9719507    972 -rwxr-xr-x   1 4321     4321       993096 sie 27 13:30 ./tests/libsai/init_switch/init_switch

so suddenly next to my local user, there is dash user 4321 in docker, and there are issues with writings files, that's why chmod

some of the changes for chmod comes from this PR #202

[chrispsommers]

Hi @kcudnik thanks for your feedback. Sorry if the build process seems inefficient. BTW it used to take >1 hour, part of the optimization was to create many small pre-built containers pulled from ACR and these have dashuser 4321 as default user instead of root (which had caused its own set of problems which PR #202 attempted to remedy, with some quirks as you've noticed). So I agree the current situation is non-ideal but the amount of effort which went into getting it to even this state was considerable and for most people, so far, performance has been acceptable. I haven't felt the need to open this can of worms since then. :)

Also, I am hoping someday someone solves Issue #395 which forces building saimeta into libsai, which takes most of the time and is probably the item getting built 2-3 times (I was only aware of 2 builds not 3).

Some questions:

• Out of curiosity, how long does make all take to run in your setup? It takes around 5 min for me, despite some redundant re-builds.
• Are you possibly using make all more than necessary? There are build targets e.g. to just rebuild libsai which should take much less time, make all can be used after intermediate work is done. For example:
  • make clean p4 - needed one time, takes 8 sec on my machine
  • <edit python p4-sai codegen or libsai c++ code>
  • make sai - 67 sec on my machine (51 sec if sai-meta is not built e.g. in my experimental branch no-saimeta-in-libsai created for Issue SAI Meta out of libsai #395

So, the recommendation is to use the make sai target until you need to run sai-thrift tests, which requires make all and takes 5x longer (5 minutes). Take a look at Concise Developer Workflows if you haven't already.

As far as making substantial changes, I am a bit leery because it can be very time-consuming and if we need to change Docker images stored in ACR, it gets even more involved. I'll keep the idea open for now.

[kcudnik]

Hey, yea it's can of worms, i am actually trying to fix this, but its so convoluted, there are many different problems on so many different levels, so mainly the makefiles are not optimal, and wrong dependencies are made, so for example:

• when building saithriftv2, it always does clean at all every time which rebuilds metadata and rpc each time and saiserver which take a long time to compile since those generated files are in megabytes: https://github.com/opencomputeproject/SAI/blob/master/test/saithriftv2/Makefile#L68
• when gensairpc.pl is executed location is based on XML generated files for SAI/meta but it was generated outside the docker, and when rpc target is built then it uses that location which is invalid inside docker and fails to generate correct sai.thrift file https://github.com/opencomputeproject/SAI/blob/master/meta/gensairpc.pl#L315, thats why clean is performed at start
• thfirt binary is not regenerating files if the files exists even if sai.thrift changed, (i spend an hour to find this) https://github.com/opencomputeproject/SAI/blob/master/test/saithriftv2/Makefile#L79 i think this is a bug i thrift, there is no option to override, and there is no message that files exits, so option here is to remove existing generated files if sai.thrift changed

there is also another problem that i don't understand: https://github.com/opencomputeproject/SAI/blob/master/test/saithriftv2/Makefile#L84 in this code PY headers depends on sai headers, but those are not sai in SAI/inc headers, but headers from /usr/include/sai, i'm not sure if that is mistake or intended behaviour, but for example sair_rpc_server depends on correct ones https://github.com/opencomputeproject/SAI/blob/master/test/saithriftv2/Makefile#L93 ../../inc, and this is really wired since those headers are copied to /usr/include/sai here: https://github.com/sonic-net/DASH/blob/main/dash-pipeline/SAI/saithrift/Makefile#L36 which make no sense XD i would update that to use ../../inc but im not sure whether this is used somewhere outside DASH repo and this: https://github.com/sonic-net/DASH/blob/main/dash-pipeline/SAI/saithrift/Makefile#L36 and what can be broken, oh well i will do that anyway and see what will break XD

i spent last week analyzing all makefiles in dash-pipeline, how this go, what is build where, and man, this is just house of cards, im really surprised that this works at all, but at this stage this is not maintainable in the long run :(

my goal is to create deb target to generate deb packages for libsai from dash, but it would be nice if build system would be more efficient and not clean and regenerate everything each time and 3 times in a row

i tested that the containers can use host user as long as sudo is not used, it requires to override -u user:guid in docker run command and all created files in docker mounted host directories are with the same user as host, but when sudo is used then there is and issue it dont work

i'm trying to fix all this, but its a long slow way, im starting from SAI repo, take a look at latest commits if you are interested https://github.com/opencomputeproject/SAI/commits/master

as for #395 why saimeta would be needed there ? so far libsai.so from DASH is not using that, it can of course, that would be helpfull if libsai gets more complicated or need some internal validation,

im not building all, just sai, and saithrift-server, but it's a long build for me, i dont have timer but defiantly over 10min, im using old laptop and preparing to buy some new dev machine :P

[chrispsommers]

Hi Kamil, I admire your dilligence, it is indeed complex. I wouldn't call it a house of cards, but it is mildly elaborate. Less so than SONiC IMO.
A lot of the complexity in DASH repo is to integrate with pre-existing saithrift server code in SAI. I won't comment on saithrift server build process other than to say I made a few small improvements to it to accommodate DASH, you might even have reviewed those PRs. I had to learn it all the hard way and getting it into DASH was weeks of work.

As for your question:

as for #395 why saimeta would be needed there

That is a good question, please see #395 (comment), this is what I'd like to see resolved. You can be the hero to fix it! If this can be resolved we can save some build time and complexity in DASH.
I just added some more comments: #395 (comment)

Building saithrift server is the slowest part of the overall build, so I would only do that as needed. I did not attempt to do any more work on saithrift server than necessary to get DASH working.

[kcudnik]

Thanks for comments, i didn't noticed those functions used by saiserver since it compiles for me without explicit saimetadata.o being added, this is interesting, maybe because libsai.so is compiles with saimetadata.o thats why saithrift compiles for me locally fine when i use libsai.so from DASH, but yes, other vendors may not have metadata in their sai, then saithrift will not compile, the sai_rpc_frontent.cpp uses that to identify attribute value type, including saimetadata.o to the saithrift directly will resolve that, and it's easy actually to do since all *.o needed are already generated when saithrivtv2 is build, but it's also an overkill, since this is just only functionality that is uses, so another approach could be to generate those objecttype/attrid mapping to value only in sai_rpc_server.cpp which is generated via gensairpc.pl, but that a little bit harder task than including some *.o in builds

i will think about this, after i fix current makefiles

anyway, sorry for house of cards, what i have in mind, is that when i touched anything in dash-pipeline/Makefile something exploded something else XD i was trying to fix -R o+w to not touch those files becasue this messes up make build process and rebuilds stuff over and over again

[chrispsommers]

I get it, and feel partially responsible, but can't keep investing more time into perfecting it. I am so happy that you bring fresh energy and ideas!

[kcudnik]

Start of long road:

• [meta] [rpc] Use location in local inc headers for gensairpc.pl opencomputeproject/SAI#1867
• [test] [saithriftv2] Check return error code from sai api opencomputeproject/SAI#1868

[kcudnik]

Final touches

• [SAI] Refactor SAI and add new features to be compatible with sonic-sairedis  #423
• [sai-api-gen] Write files only when changes are detected #429
• [dash-pipeline] Refactor Makefiles #432

now build is fast and clean, closing this issue