Change in osmo-gsm-tester[master]: Introduce Android UEs as new modems

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

pespin gerrit-no-reply at lists.osmocom.org
Mon Nov 23 20:38:13 UTC 2020


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.fuerste at softwareradiosystems.com>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Mon, 23 Nov 2020 20:38:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201123/9a2d0ea9/attachment.htm>


More information about the gerrit-log mailing list