pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217 )
Change subject: Modems: Introduce Android UEs ...................................................................... Patch Set 1: Code-Review-1 (30 comments) https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1//COMMIT_MSG@9 PS1, Line 9: To expand the test capacities we would like to introduce I'm really lacking here some description on the setup to understand how the android phones are used, how are they hooked to OGT. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py File src/osmo_gsm_tester/core/process.py: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py@391 PS1, Line 391: self.remote_port = remote_port Please add the port param as a different patch prior to the android one (this patch). https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py@400 PS1, Line 400: self.remote_user = 'root' All of this needs to get out from here. In the end is simply setting the remote_user to root which can be passed as an argument... https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py@419 PS1, Line 419: if is_ue_cmd: this is not really UE/android specific, please introduce the port feature generically. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py@464 PS1, Line 464: run_dir = run_dir.new_dir(name) This is a separate patch. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py@476 PS1, Line 476: run_dir = run_dir.new_dir(name) This too, in the same patch as line 464. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py@480 PS1, Line 480: Different patch. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/remote.py File src/osmo_gsm_tester/core/remote.py: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/remote.py@31 PS1, Line 31: def __init__(self, run_dir, remote_user = 'root', remote_host = 'localhost', remote_cwd=None, remote_port=None): This all goes into the generic "add port" patch. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/enb_srs.py File src/osmo_gsm_tester/obj/enb_srs.py: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/enb_srs.py@117 PS1, Line 117: MainLoop.sleep(3) What's this? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/iperf3.py File src/osmo_gsm_tester/obj/iperf3.py: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/iperf3.py@269 PS1, Line 269: if self._run_node.is_local() and ue.ue_serial: This most probably needs to go into a subclass of RemoteProcess or Process or alike. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/iperf3.py@291 PS1, Line 291: None, self._run_node.ssh_port()) This goes into the "add port patch". https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py File src/osmo_gsm_tester/obj/ms_android.py: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@1 PS1, Line 1: # Author: Nils Fürste <[email protected]> Please add a Copyright header with license. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@38 PS1, Line 38: class AndroidUE(MS, srslte_common): Please split all the methods in this class according to their scope. I know it's not done perfectly in lots of places but let's do it here since there's lots of code. Use the following categories, see obj/bts.py as an example: ############## # PROTECTED ############## ######################## # PUBLIC - INTERNAL API ######################## ################### # PUBLIC (test API included) ################### You can get an idea of what is what by looking at other ms classes and which methods are used by tests, or by other classes inside obj/ or core/. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@75 PS1, Line 75: Drop space https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@80 PS1, Line 80: if self.enable_pcap: use this boolean when calling this method, not here. Or at least use early return. if not self.enable_pcap: return https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@85 PS1, Line 85: diag_parser_proc.launch() tis launch needs to go after the if/else. Actually.... you don't need to scp back if you are running locally... so you can drop the if and the else condition altogether.. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@97 PS1, Line 97: time.sleep(2) Mainloop.sleep You have actually Mainloop.wait(), you should use it here instead of this selfmade timeout + sleep you use here https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@103 PS1, Line 103: if not self._run_node.is_local(): Again, if you are scping the file it's because you are running remotely, so makes no sense to have this check here. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@116 PS1, Line 116: self.rx_monitor_proc.terminate() You shouldn't need this if you make OGT "remember" to have it killed. You need that to make sure the process doesn't end up dangling when the test is done successfuly or fails. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@122 PS1, Line 122: if self.diag_monitor_proc: It probably makes sense to have DiagMonitor as a different class either in this same file or a separate file. See for instance the pcap_recorder.py, or AbisIpFind, etc. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@134 PS1, Line 134: self.scp_back_pcap() if not is local and pcap enabled, then self.scp_back_pcap() https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@152 PS1, Line 152: popen_args_clear_run_dir = ['sudo', 'rm', '-rf', '-v', str(self.remote_run_dir) + '/*', '||', 'true'] Please describe somewhere why is that needed but I'm not liking having a sudo rm -rf /* here ;) Feel free to contact me and explain me your needs so we can work out the best way. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@167 PS1, Line 167: self.run_netns_wait("kill-adb", ['sudo', 'adb', 'kill-server']) why is this run on a netns? I lack a description of the setup in the commit message. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@182 PS1, Line 182: emm_conn_chk_proc.launch() launch below if/else? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@218 PS1, Line 218: self.rx_monitor_proc.launch() All these processes you launch should be monitored by OGT using self.testenv.remember_to_stop(self.process). And please, rather split some code into different classes if possible. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@227 PS1, Line 227: DIAG_PARSER = 'osmo-gsm-tester_androidue_diag_parser.sh' QcDiag class? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/run_node.py File src/osmo_gsm_tester/obj/run_node.py: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/run_node.py@33 PS1, Line 33: def __init__(self, type=None, run_addr=None, ssh_user=None, ssh_addr=None, run_label=None, remote_port=None): All of this goe sto the "add port" commit. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/sysmocom/default-suites.conf File sysmocom/default-suites.conf: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/sysmocom/default-suites.conf@175 PS1, Line 175: - 4g:androidue@mi5g+srsenb-rftype@uhd+mod-enb-nprb@25+mod-enb-txmode@1 That's definetly going to fail in our setup ;) Better simply describe it as an example in the commit description. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/sysmocom/scenarios/androidue%40.conf File sysmocom/scenarios/[email protected]: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/sysmocom/scenarios/androidue%40.conf@3 PS1, Line 3: - label: ${param1} name of the scenario doesn't make much sense, since the "label" is generic. Rather name it "[email protected]" or similar. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/utils/bin/osmo-gsm-tester_androidue_conn_chk.sh File utils/bin/osmo-gsm-tester_androidue_conn_chk.sh: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/utils/bin/osmo-gsm-tester_androidue_conn_chk.sh@2 PS1, Line 2: while true; do Is this some monitoring tool? Please describe what is it used for here. -- To view, visit https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-gsm-tester Gerrit-Branch: master Gerrit-Change-Id: Iab37a0de59d6643d21bced34ddca06ff40fa62df Gerrit-Change-Number: 21217 Gerrit-PatchSet: 1 Gerrit-Owner: srs_andre <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Nils <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Comment-Date: Tue, 17 Nov 2020 17:59:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
