Closed Daimakaimura closed 1 year ago
Thanks for the PR!
I will do a more exahustive review a later, but what I can already tell so you already know is that analysis-module/src/analysis_cproc.py
will require tests, as well as any modified python files. So when I run the tests, all are passed.
Also, I would not start to throw more code in main.py
. Lets try to decouple that into external files/functions.
If you want test examples (some of them are not up to date, WIP right now), you can check them in analysis-module/tests
.
BTW, thanks for starting the selection of what analysis to run inside the container, I wanted to do that but was left until now.
@AlbertoSoutullo @0xFugue when you try it before approving please run it with both infrastructures:
sh ./build.sh
sh ./run.sh cadvisor
docker run --network "host" -v $(pwd)/wakurtosis_logs:/simulation_data/ -v $(pwd)/config/topology_generated:/tomls/ --add-host=host.docker.internal:host-gateway analysis -p <prometheus_port>
sh ./run.sh container-proc
docker run --network "host" -v $(pwd)/wakurtosis_logs:/simulation_data/ -v $(pwd)/config/topology_generated:/tomls/ --add-host=host.docker.internal:host-gateway analysis -i container-proc
Yeap, I can see there are conflicts now, there were none when I first created the PR. I will give the tests a try and fix the the conflicts. Regarding the comments, I have no clue why you cannot see them. Let me check because now I can see them in the PR websites
Added tests for container-proc. Please, try to run for both infra and the tests. Container-proc plotting doesnt take into account the new plotting configuration in config.json
Example:
To run tests:
@AlbertoSoutullo just added an update to build the cproc venv in the root folder of the repo. Let's see if you still get the RPC size limit error.
@AlbertoSoutullo just added an update to build the cproc venv in the root folder of the repo. Let's see if you still get the RPC size limit error.
It still doesn't work.
Kurtosis compress the folder of the project and push it to github (I don't know exactly how this internally works).
This is why there is the gRPC limit of 4mb exists. Inside the repository, we cannot have files larger than that. The virtual environment needs to be created outside the repository folder. Either upper path, or where you want. Or if you don't want to do this, you can create a Docker Image. In your last commit, you are still creating the venv inside the repository folder.
Did you actually tried your last changes? Becase if it works for you, we have a problem. It is impossible that Kurtosis can work with a venv inside the folder. So check again if you are testing correctly your changes, because then we should have a meet and discuss why it works for you and not for me.
Regards.
Great. Yes, it does work in my machine in both cases (before and after the last commit). I am not getting the RPC size limit error, no idea why it is complaining in your machine but not in mine, the size of the repo should be similar and the RPC limit is the same. Did you try to do a clean_repo before just in case?
Great. Yes, it does work in my machine in both cases (before and after the last commit). I am not getting the RPC size limit error, no idea why it is complaining in your machine but not in mine, the size of the repo should be similar and the RPC limit is the same. Did you try to do a clean_repo before just in case?
I was just checking the issue. What we are doing in clean_repo.sh
is not actually "cleaning". We are basically invoking Git garbage collector and pruning references that are no longer usable. But we cannot ensure that this can be a solution for everyone (for me, it is not).
The only thing we need to do so it works for everyone is to not create a python venv inside wakurtosis folder. Either this, or dockerize your python script. Just create it in the upper folder assuming the user can, and lets just move on with this, imo.
Ok, just pushed the changes with the monitoring container. I still need to solve the conflicts before asking for a re-re-re-review
Fixed the conflicts and added small logic in run.sh to call the analysis module according to the infra we are running
Goal:
Include the container process level monitoring and analysis in the framework
Main changes:
run.sh
logic to handle the container process level monitoring with the'container-proc'
optionanalytsis_cproc.py
to handle metrics from the container process level monitoringconfig.py
)-i, --infra
) at the moment the only option taken into account is'container-proc'
should default to the previous behaviour (cadvisor
) for any other valuebuild.sh
To run:
sh ./build.sh
sh ./run.sh container-proc
docker run --network "host" -v $(pwd)/wakurtosis_logs:/simulation_data/ -v $(pwd)/config/topology_generated:/tomls/ --add-host=host.docker.internal:host-gateway analysis -i=container-proc