| === janitha75 is now known as janitha7 | ||
| === janitha76 is now known as janitha7 | ||
| meena | my queendom for a pry and pry-debug in python | 13:13 |
|---|---|---|
| minimal | meena: in your testing of BSD support for cc_ntp have you changed test_ntp_the_whole_package in tests/unittests/config/test_cc_ntp.py yet? That's the test that is failing for me with Alpine since I added a modified manage_service function to cloudinit/distros/alpine.py | 13:18 |
| meena | minimal: i'm also on it right now, and really lost as to what else to mock | 13:18 |
| meena | meena: https://github.com/canonical/cloud-init/pull/1759/commits/bbf5225d0cb29a5f2152fa8721ba774a67c7282c | 13:19 |
| -ubottu:#cloud-init- Pull 1759 in canonical/cloud-init "cc_ntp: add support for BSDs" [Open] | 13:19 | |
| minimal | meena: the problem with it I'm seeing is that the test results in systemd/systemctl command being used which obviously doesn't match the expect "rc-service" on Alpine | 13:19 |
| meena | minimal: same! | 13:20 |
| minimal | meena: I did add a uses_systemd function in cloudinit/distros/alpine.py that returns False but that made no difference | 13:20 |
| meena | https://gist.github.com/igalic/2886219030bcf2afed9c444721a1da9e | 13:21 |
| meena | minimal: you don't need that, the mock there says it's false | 13:22 |
| minimal | I just decided to add it anyway to ensure in general on Alpine that uses_systemd is False | 13:22 |
| minimal | re: your gist, yeah looks like you're seeing the same as me, it returning "systemctl reload-or-restart ntp" instead of the distro-override | 13:25 |
| minimal | the thing is this testcase passed until I added manage_service in alpine.py so the manage_service in __init__.py WAS using "rc-service" fine | 13:26 |
| meena | which now makes me wonder how the distro object is constructed | 13:27 |
| meena | looks good, actually | 13:28 |
| meena | i just invoked tests with --pdb | 13:29 |
| meena | it's a far cry from pry/pry-debug, but it's better than nothing | 13:29 |
| meena | minimal: uses_systemd() is defined in __init__.py, no? | 13:31 |
| minimal | meena: yes but I decided to override it in alpine.py | 13:34 |
| meena | minimal: uses_systemd correctly returns False in my test, so i have no idea what's happening | 13:35 |
| minimal | meena: me neither | 13:39 |
| meena | simplified test, and it's still failing: https://github.com/canonical/cloud-init/pull/1759/commits/bce6271dc096e982f1c851df4bb97d2e9195fef3 | 13:47 |
| -ubottu:#cloud-init- Pull 1759 in canonical/cloud-init "cc_ntp: add support for BSDs" [Open] | 13:47 | |
| meena | i need to take a break and bring kiddo to ballet | 13:48 |
| meena | meena: but I'm glad to see we're both stuck in the same place | 13:48 |
| meena | at least you're not stuck here tho: https://gist.github.com/igalic/75c61c84c09a8edbe357713ca3aa1f14#file-log-stdout-L4493 | 13:49 |
| falcojr | meena: is the subp failure on trying to ifconfig? | 13:51 |
| meena | falcojr: dunno, haven't looked at the bigger picture, just trying to get ntp working, and minimal and i are having the same issue | 13:51 |
| meena | no ifconfig involved, just manage_services() | 13:51 |
| minimal | falcojr, blackboxsw, rharper: any thoughts on the ntp test failure issue that meena and myself are having? | 14:30 |
| rharper | minimal: remind me where to see the unittest log with the error ? | 14:32 |
| rharper | minimal: couple of questions, in the unittest, you expect a service 'restart', in the cc_ntp code, you use 'reload' ... | 14:42 |
| minimal | rharper: tests/unittests/config/test_cc_ntp.py in the test_ntp_the_whole_package test | 14:48 |
| rharper | oh, I was looking for output from a failed run of it... | 14:49 |
| rharper | but I think the expected service call doesn't match, reload vs restart | 14:49 |
| minimal | that test passed for me using cloud-init master, however I have some local changes (added a manage_service to alpine.py) and if fails after that | 14:50 |
| minimal | meena posted her failed test output here: https://gist.github.com/igalic/2886219030bcf2afed9c444721a1da9e | 14:51 |
| minimal | basically what both she and I am seeing is that the actual subp call has "systemctl..." rather than the expected distro-specific alternative (e.g. for Alpine "rc-service ...") | 14:52 |
| minimal | in her case for FreeBSD it should be "service ..." | 14:53 |
| minimal | basically the testcase doesn't seem to be handling the non-systemd situations (even though those set uses_systemd = False) | 14:54 |
| rharper | m_subp.which.return_value = True , where in the distro class are you implementing the manage_service? and does that support more than one service binary ? | 14:55 |
| minimal | rharper: my changes aren't submitted yet so you can't see them but I implemented manage_service (and also uses_systemd) in cloudinit/distros/alpine.py | 15:00 |
| minimal | rharper: meena's changes are visible in cloudinit/distros/freebsd.py | 15:01 |
| rharper | it looks like maybe the m_sysd mock is not the object that's being used when invoking the distro.manage_service path ... ... one other idea instead of setting .return_value, you can use a side_effect and a function | 15:01 |
| rharper | the mock paths look right. but hard to say without bringing it up in the interpreter and inspecting the mock objects, | 15:02 |
| meena | I was knee deep in pdb before leaving, and now I'm without a laptop | 15:03 |
| rharper | another option is to instrument manage_service to see what the result of uses_systemd() is from the distro object perspective | 15:04 |
| meena | wird that it's not failing on https://github.com/canonical/cloud-init/pull/1759 | 15:05 |
| -ubottu:#cloud-init- Pull 1759 in canonical/cloud-init "cc_ntp: add support for BSDs" [Open] | 15:05 | |
| rharper | it does seem odd since the mock patches are not new ... so possibly then something in the distro object is constructed differently | 15:05 |
| rharper | does that have to do with the host environment running the unittest? | 15:06 |
| minimal | meena: you didn't change the testcase test for BSD there though? so it would be comparing systemctl against systemctl still? | 15:06 |
| meena | minimal: that test exercises all distros that ntp supports | 15:07 |
| meena | rharper: tests are running on an Ubuntu, of sorts (Elementary OS) | 15:07 |
| minimal | meena: yupe, but before you added the cases for freebsd/openbsd to check the correct commands it would have been checking systemctl instead... | 15:08 |
| minimal | rharper: in my situation the tests are running on Alpine (as part of the packaging of cloud-init) | 15:08 |
| meena | minimal: but alpine was also in that list, and should've used "service" | 15:09 |
| meena | now that it's specialised, it fails | 15:09 |
| meena | ah, that sounds like a clue | 15:09 |
| minimal | meena: it did and passed BEFORE I creating a separate manage_service in distros/alpine.py, when it uses the one in __init__.py it passed | 15:09 |
| meena | exactly | 15:10 |
| rharper | oh, interesting, so getting default manage_service vs per-distro object one ? | 15:10 |
| minimal | rharper: yupe | 15:10 |
| meena | yes, apparently | 15:10 |
| === janitha72 is now known as janitha7 | ||
| rharper | maybe you need to mock the distro object in the for loop | 15:10 |
| meena | but even so, why are we getting the systemctl branch?? | 15:10 |
| rharper | instead of the base class | 15:10 |
| rharper | should be easy enough to dump some prints in base class manage and your distro class manage to see which one wins. But if you're testing on a systemd host and not mocking the uses_systemd, that's going to return true | 15:11 |
| rharper | which won't get you the path you want | 15:11 |
| minimal | rharper: in my situation I'm testing on Alpine (non-systemd) with a uses_systemd in alpine.py that returns False and the testcase sets uses_systemd=False anyway for alpine distro | 15:13 |
| rharper | and somehow it's still calling systemctl? in that case, looking in the cc_ntp handle path to see where it's fetching the service command name, figuring out the object path to that should help us sort out what needs mocking... | 15:15 |
| meena | here's the thing, in pdb, i can see all the things I'm setting to be true / false to have the value they should have, but it still does the wrong thing | 15:16 |
| meena | so i reckon the clue with the distro mock being done to early makes sense | 15:18 |
| minimal | rharper: cc_ntp.py calls cloud.distro.manage_serve only once | 15:19 |
| minimal | s/server/service/ | 15:19 |
| meena | minimal: not after my change lol, but still | 15:21 |
| meena | after my change, on a BSD, it calls either once, or three times | 15:23 |
| === janitha75 is now known as janitha7 | ||
| minimal | I'm out of my depth with the mocking stuff so hoping meena can figure it out lol | 15:30 |
| meena | minimal: im back to the computer in about 3 hours | 15:38 |
| minimal | meena: no rush, have other cloud-init stuff to work on in the meantime :-) | 15:39 |
| holman | thanks for the merge @falcojr - I'll rebase the other ansible branch now | 16:21 |
| meena | rharper: so, this looks correct, actually | 17:13 |
| meena | for distro in cc_ntp.distros: | 17:13 |
| meena | mycloud = self._get_cloud(distro) | 17:13 |
| meena | rharper: so the distro object is setup correctly—ish… | 17:14 |
| rharper | I'm almost there .. it's just not clear what subp is getting called so much... | 17:14 |
| rharper | https://hastebin.com/qixudoyepi.sql | 17:15 |
| rharper | that *feels* like we need to clear the m_dsubp.calls stack between each loop... lemme find that code | 17:16 |
| meena | rharper: what's producing that view? | 17:17 |
| rharper | print(f"m_dsubp.calls: {distro}:") | 17:18 |
| rharper | print(f" {m_dsubp.call_args_list}") | 17:18 |
| rharper | print(f" {m_dsubp.subp.call_args_list}") | 17:18 |
| rharper | right after the handle() call | 17:18 |
| rharper | ok, and then with a m_dsubp.reset_mock() at the top of the loop, I can see that the freebsd manage_service() returns the correct call to subp ... but somehow, the subp in there is *not* the distro.subp being mocked ... | 17:20 |
| rharper | it might be cloudinit.distro.bsd.subp ... since it's a derived class... not sure yet. | 17:20 |
| rharper | not quite clear why the sub classes in linux, like rhel or debian/ubuntu don't have this issue yet. | 17:20 |
| meena | rharper: which code are you looking at, btw? the original, or my patchset? | 17:21 |
| rharper | your branch, and then I'm running make check | 17:22 |
| rharper | igalic-github/fix/cc_ntp_bsd | 17:23 |
| meena | a whole make check, that sounds excessive… | 17:23 |
| rharper | yeah, I can't recall how to run the single test | 17:23 |
| rharper | been too long | 17:24 |
| rharper | if you toss that here, that'll speed things up =) | 17:24 |
| meena | i'm doing: python3 -m pytest --pdb -vvv tests/unittests/config/test_cc_ntp.py | 17:24 |
| rharper | thanks | 17:24 |
| meena | that drops me into pdb when the test in that file fails | 17:24 |
| rharper | cool, that'll help | 17:24 |
| meena | you can probably still corner it down to just one test function | 17:24 |
| rharper | yeah, I think it takes :<test_name> ... will play with that | 17:25 |
| meena | python3 -m pytest --pdb -vvv tests/unittests/config/test_cc_ntp.py -k ntp_the_whole_package | 17:25 |
| meena | or that | 17:25 |
| meena | it's always annoying when the code actually works, but the tests break for no discernable reason | 18:01 |
| meena | I much more like when the tests break in a way that reveals something broken about the code, not about the tests | 18:02 |
| meena | rharper: any progress? | 18:07 |
| rharper | slight ... | 18:07 |
| meena | getting any better errors yet? | 18:07 |
| rharper | if I replace subp.subp in freebsd.py distro class, with bsd.subp.subp (since you're derived from bsd distro object which imports subp) then if I mock cloudinit.distros.bsd.subp, I can get those calls ... and they're correct ... but I've not yet sorted out why the subp.subp in freebsd.py is not cloudinit.distro.subp | 18:08 |
| meena | rharper: the out mock is created at the creation of the function, at that point, with nothing attached to it, the default would be to create the distro object from __init__.py, and so, from Linux | 18:10 |
| meena | can you nest `with` calls | 18:15 |
| meena | rharper: i've got it | 18:19 |
| rharper | yes | 18:19 |
| meena | or not, i dunno | 18:19 |
| rharper | you can with foo, with bar, | 18:19 |
| rharper | with mock.patch("cloudinit.config.cc_ntp.util") as m_util: | 18:19 |
| rharper | with mock.patch("cloudinit.distros.freebsd.subp") as m_bsdsubp: | 18:19 |
| meena | rharper: https://gist.github.com/d092af0a4610d739326dba25beb4ad97 | 18:21 |
| meena | E AssertionError: expected call not found. | 18:21 |
| meena | E Expected: subp(['systemctl', 'reload-or-restart', 'ntp'], capture=True) | 18:21 |
| meena | E Actual: not called. | 18:21 |
| rharper | but AFAICT, real issue is with the construction of the FreeBSD distro object ... the subp we import should be the base class one (cloudinit.distros.subp) ... but Maybe because the bsd distro object imports it, and then we derive freebsd from that we're getting one specific to the freebsd object ... I dunno. I have half a mind to change the unittest to mock out cloudinit.distro.<flavor>.subp ... but some of the flavors just pass | 18:21 |
| rharper | down to lower base class (like almalinux is just a RHEL clone)... so we get an import attribute error subp when trying to mock that attribute. | 18:21 |
| rharper | yeah, that's what I tracking down, freebsd.py's subp is not the base distro one... it's bsd.py subp IIRC. | 18:22 |
| rharper | ok, I need to run, but I'll keep poking | 18:22 |
| === janitha75 is now known as janitha7 | ||
| rharper | meena: https://hastebin.com/iliqabubus.py -- this passes ... and minimal I think the reset_mock() will fix your issue. basically, in bsd.py we import the distros's module subp and util, and then in bsd and subclasses always use distros.subp module ... someone else may sort out why the existing import/subclass structure isn't working, but I don't think this is a wrong approach. | 19:42 |
| meena | rharper: will test | 19:59 |
| meena | rharper: that's wild | 20:01 |
| meena | rharper: do you want me to commit this with you as author? | 20:01 |
| rharper | the other part that complicates things is that FileSystemMocking unittest class also mocks subp as well... so it's possible that we could find it under the class's self ... but ... I dunno, I want blackboxsw to peek at it.. its a bit of a mystery how the bsd classes didn't get subp from distros, but they are also constructed a little different. | 20:02 |
| holman | meena: since current policy is squash merge, multiple authors get squashed into one commit per PR iirc :/ | 20:03 |
| rharper | I can submit a PR to your branch | 20:03 |
| rharper | or you can just merge it in | 20:03 |
| meena | rharper: it doesn't apply cleanly | 20:04 |
| meena | holman: i can always just credit @rharper as another author in the commit message, no? | 20:04 |
| holman | meena: yes, that's works | 20:04 |
| meena | Wasn't there a Co-Author stanza? | 20:04 |
| holman | *that works | 20:05 |
| meena | Co-authored-by: name <name@example.com> | 20:05 |
| meena | rharper: i have to apply it manually, and I'll happily do that, since you're probably a bit busy with a billion other things | 20:05 |
| rharper | sure, whatever works for you: I've been committing to cloudinit with: Ryan Harper <rharper@woxford.com> | 20:06 |
| meena | lol i somehow didn't notice that OpenBSD inherits from NetBSD | 20:15 |
| meena | (to be fair, that is historically accurate) | 20:18 |
| meena | *professor Farnsworth voice* Good news, everyone! ==== 128 failed, 4347 passed, 10 skipped, 255 warnings in 87.46s (0:01:27) ===== | 20:55 |
| meena | (same amount of failures with rharper's patch when make check is ran on FreeBSD) | 20:55 |
| meena | i was hoping that would've magically resolved like half of those failures, but, alas… | 20:56 |
| rharper | so the cc_ntp test fails on freebsd, but passes on linux hosts | 20:56 |
| meena | rharper: no, no, that passes | 20:56 |
| rharper | oh, heh, I see but still more work to do | 20:57 |
| meena | but a buttload of other tests fail: https://gist.github.com/igalic/75c61c84c09a8edbe357713ca3aa1f14#file-log-stdout-L4493 | 20:57 |
| rharper | gotcha | 20:57 |
| meena | That's not my top prio right now, just something to keep work away on | 20:57 |
| rharper | lots of network related things in there... that's not terribly surprising | 20:57 |
| meena | also: OpenBSD folks do not like adding ports, where the test system fails | 20:57 |
| rharper | makes sense | 20:58 |
| meena | fortunately, FreeBSD folks are less averse to that, or I might have never found this project… | 21:00 |
| meena | after this much effort, should I add a BSD specific test, too? one of those that says, oh, you're disabling ntpd? and enabling openntpd/chrony?? (you weirdo) | 21:09 |
| meena | (btw: openntpd starts so much faster than chrony and i think ntpd, too) | 21:10 |
| rharper | certainly worth adding tests freebsd, the different bsd's may diverge and having the tests would help catch those differences. | 21:11 |
| meena | rharper: i think this might be enough to start with: https://gist.github.com/035af9846bf65153773d99d57bede942 | 21:27 |
| rharper | you'll get more than just the one call... in the case where you disable it, there's the stop and then the reload/restart ... currently the check only looks for the final 'restart' ... so the subp.assert would need to loop over items in the expected_service call | 21:39 |
| meena | rharper: it's too bad i can't just make it an array of arrays | 21:43 |
| rharper | you can, or make a dict with keys of a type... lots of ways. | 21:44 |
| === EugenMayer50 is now known as EugenMayer5 | ||
| meena | hrm, my intuition was somewhat wrong: ==== 127 failed, 4348 passed, 10 skipped, 255 warnings in 86.27s (0:01:26) ===== | 22:14 |
| meena | just 1 test more passing, after removing the NoCloud iso from the machine | 22:15 |
| minimal | rharper: testcases now all passing on Alpine after I applied all the various changes you suggested (not just the reset_mock one). Now doing some local testing... | 23:36 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!