pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302 )
Change subject: Introduce Android UEs as new modems ...................................................................... Patch Set 2: (48 comments) https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2//COMMIT_MSG@32 PS2, Line 32: Infrastructure explaination: You probably want to add all this documentarion into the UserManual too (osmo-gsm-tester.git/doc/), feel free to add a new section or whatever if you want. fyi here's the nightly output of the documentation: http://ftp.osmocom.org/docs/latest/osmo-gsm-tester-manual.pdf https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2//COMMIT_MSG@55 PS2, Line 55: dropbearmulti dropbear -F -E -p 130 -R -T /data/local/tmp/authorized_keys -U 0 -G 0 -N root -A what's this dropbearmulti? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2//COMMIT_MSG@87 PS2, Line 87: ue_serial: '8d9d79a9' what's this for? can you check if you can reuse any of the existing fields? like "path" ? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2//COMMIT_MSG@88 PS2, Line 88: apn_name: 'srsapn' looks like you want to have a node here: apn: name: 'bla' mcc: '901' mnc: '70' sel: True https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2//COMMIT_MSG@100 PS2, Line 100: ue_ssh_port: 130 ssh_port https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/bitrate_monitor.py File src/osmo_gsm_tester/obj/bitrate_monitor.py: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/bitrate_monitor.py@3 PS2, Line 3: # Copyright (C) 2020 by sysmocom - s.f.m.c. GmbH That's not from sysmocom, probably from SRS ;) https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/bitrate_monitor.py@44 PS2, Line 44: def run_androidue_cmd(self, name, popen_args, sync=True): Better don't do the launch() here and simply return the proc object and let the caller decide what to do with it. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/bitrate_monitor.py@58 PS2, Line 58: self.rem_host.remote_user = 'root' You should ideally use sudo with sudoers file limit use of certain commands, that what we usually did so far, but I'm not going to block this. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/bitrate_monitor.py@72 PS2, Line 72: popen_args_rx_mon = ['while', 'true;', 'do', I think it'd make more sense to use something like: ['sh', '-c', 'while true; do echo; cat /sys/class/net/ + self.ue_data_intf + '/statistics/rx_bytes'] https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/bitrate_monitor.py@79 PS2, Line 79: self.testenv.remember_to_stop(self.rx_monitor_proc) As I shared before, return run_androidue_cmd without calling launch() on it, then AFTER calling remember_to_stop() you call proc.launch(). Imagine someone sent a SIGINT to OGT in between, you end up with dangling processes. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/bitrate_monitor.py@88 PS2, Line 88: brate_rx_raw = self.rx_monitor_proc.get_stdout().split('\n') if you run the process locally you may find out the output you are looking for is in stderr, be careful here (when we launch stuff on ssh due to tty (-t -t) related topics everything ends up in stdout locally) https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/iperf3.py File src/osmo_gsm_tester/obj/iperf3.py: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/iperf3.py@235 PS2, Line 235: def prepare_test_proc(self, dir=None, netns=None, time_sec=None, proto=None, bitrate=0, tos=None, ue=None): Drop ue param, add new param "adb_serial=None" and only pass that (see coments below, you don't need the ue at all). https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/iperf3.py@267 PS2, Line 267: drop ws line change https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/iperf3.py@305 PS2, Line 305: self.process = self.rem_host.RemoteProcess(self.name(), popen_args, remote_port=remote_port, env={}) remote_port should already be part of rem_host (see previous patch comments) so this doesn't need to change. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/iperf3.py@320 PS2, Line 320: if netns: if netns: ... elif adb_serial: ... else: ... process.Process or do you use both netns and adb at the same time? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/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/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@3 PS2, Line 3: # Copyright (C) 2020 by sysmocom - s.f.m.c. GmbH wrong copyright https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@43 PS2, Line 43: 'ue_serial': schema.STR, you can use "path" here instead, from base class. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@105 PS2, Line 105: def _restart_adb_inst(self, as_root=False): Why do you need to run this everytime? Sounds like a system configuration which should be done once at startup? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@118 PS2, Line 118: def check_device_availability(self): where is this used? looks like a private/protected method. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@123 PS2, Line 123: def get_assigned_addr(self, ipv6=False): private/protected? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@125 PS2, Line 125: proc = self.run_androidue_cmd("get-ip", ['ip', 'addr', 'show']) I think we already have some python code to get addresses in util.py, give it a look. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@134 PS2, Line 134: def get_carrier_id(self, carrier_name): private/protected? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@145 PS2, Line 145: def set_new_carrier(self, apn_parameter, carr_id): private/protected? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@170 PS2, Line 170: def set_preferred_apn(self, carr_id): private/protected? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@175 PS2, Line 175: def select_apn(self, carr_name): private/protected? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@185 PS2, Line 185: def delete_apn(self, carr_name): private/protected? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@192 PS2, Line 192: def run_androidue_cmd(self, name, popen_args, sync=True): I remember seeing this exact same method somewhere else, am I right? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@207 PS2, Line 207: proc = self.rem_host.RemoteProcess(name, popen_args, remote_env={}, remote_port=self._run_node.remote_port()) remote_port inside rem_host https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@228 PS2, Line 228: proc.launch() self.testenv.remember_to_stop beforehand, or simply don't call launch() inside here. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@231 PS2, Line 231: def run_netns_wait(self, name, popen_args): This is a public function IIRC (used by tests), so move it below to the public section. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@239 PS2, Line 239: if not self.check_device_availability(): how can this happen?sounds like this should be done by the system at startup (have some startup script running a loop checking this until all show up). How can that change during runtime? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@246 PS2, Line 246: def stop(self): I see in general you wanting to stop too much stuff manually. That's why we have "remember_to_stop", so that the infrastructure takes care of processes, no need to explicitly stop them. Of course if you need to do other stuff then it's fine. stop() method is not used outside of the class btw, so it's private/protected. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@260 PS2, Line 260: def configure(self): private/protected https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@281 PS2, Line 281: "carrier": str(values['ue']['apn_name'] or 'default'), As I mentioned it probably makes more sense to have a subnode for all apn settings in ms resources.conf, like you are doing here internally. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@320 PS2, Line 320: self.rem_host, self.ue_serial, self._run_node.remote_port()) remote_port in rem_hosy, you don't need specific param. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@346 PS2, Line 346: timer -= 1 so for each 2 seconds you sleep you decrease timer 1? this looks wrong (probably everybody would expect self.connect_timeout to be in seconds?) https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@400 PS2, Line 400: def get_remote_port(self): Not needed, info is available in run_node https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@403 PS2, Line 403: def set_airplane_mode(self, apm_state): This is probably private/protected. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@410 PS2, Line 410: def set_apn(self, apn_param, sel_apn): This is probably private/protected? is it used in tests? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/ms_android.py@421 PS2, Line 421: def scp_back_pcap(self): This is probably private/protected. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/qc_diag.py File src/osmo_gsm_tester/obj/qc_diag.py: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/qc_diag.py@47 PS2, Line 47: def _clear_work_dirs(self): This seems to be repeated from UE class? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/qc_diag.py@67 PS2, Line 67: def run_androidue_cmd(self, name, popen_args, sync=True): Sounds like you want to have a base class with this methods implemented once instead of 3 times? Something like "AndroidHost" or "AdbSshRemoteHost" or something like that. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/qc_diag.py@120 PS2, Line 120: def stop(self): This is probably protected/private https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/qc_diag.py@124 PS2, Line 124: def write_pcap(self): This is probably protected/private https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/qc_diag.py@158 PS2, Line 158: def get_rrc_state(self): Is this used by tests? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/qc_diag.py@167 PS2, Line 167: def get_paging_counter(self): Is this used by tests? https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/src/osmo_gsm_tester/obj/qc_diag.py@171 PS2, Line 171: def running(self): That's probably used internally only. https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/sysmocom/suites/4g/iperf3_dl.py File sysmocom/suites/4g/iperf3_dl.py: https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302/2/sysmocom/suites/4g/iperf3_dl.py@26 PS2, Line 26: proc = iperf3cli.prepare_test_proc(iperf3cli.DIR_DL, ue.netns(), bitrate=max_rate, ue=ue) ue.path() instead of pasing ue? Or ue.serial_device()? Of even better, check ue.features('qcdiag') or whatever? -- To view, visit https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21302 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: I79a5d803e869a868d4dac5e0d4c2feb38038dc5c Gerrit-Change-Number: 21302 Gerrit-PatchSet: 2 Gerrit-Owner: ninjab3s <nils.fuer...@softwareradiosystems.com> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Mon, 23 Nov 2020 20:38:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
