Wednesday, 2017-03-08

*** Syed__ has quit IRC00:45
*** pwnall1337 is now known as zz_pwnall133700:52
openstackgerritMerged openstack/craton master: Pretty-print format all JSON response bodies  https://review.openstack.org/44258801:33
*** VW has joined #craton02:42
*** VW has quit IRC03:01
*** VW has joined #craton03:02
openstackgerritJim Baker proposed openstack/craton master: Updates Alembic migration to better match SQLAlchemy models  https://review.openstack.org/44164404:19
*** openstackgerrit has quit IRC09:03
*** VW_ has joined #craton10:17
*** VW has quit IRC10:17
sulojimbaker: thomasem: i left more comments on this patch:10:24
sulohttps://review.openstack.org/#/c/441644/10:24
*** david-lyle has quit IRC12:30
*** david-lyle has joined #craton12:33
sigmavirusmorning13:01
sulosigmavirus: morning13:04
*** VW_ has quit IRC13:16
sulobtw i took this bug: https://bugs.launchpad.net/craton/+bug/167056113:18
openstackLaunchpad bug 1670561 in craton "Create bootstrap user and project only via SQLAlchemy models" [Critical,New] - Assigned to Sulochan Acharya (sulochan-acharya)13:18
sulothomasem: ^ i know you've been meaning to look at it as well ... so fyi13:18
thomasemsulo: Thanks for the heads up!!13:35
*** openstackgerrit has joined #craton13:39
openstackgerritTomi Juvonen proposed openstack/craton master: docker install documentation not consistent  https://review.openstack.org/44312913:39
*** VW has joined #craton13:52
thomasemHere's the bug for JSONPath querying: https://bugs.launchpad.net/craton/+bug/167111614:17
openstackLaunchpad bug 1671116 in craton "Lacking support for searching nested variables" [High,New] - Assigned to Thomas Maddox (thomas-maddox)14:17
thomasemI would appreciate folks' thoughts on that one.14:20
thomasemI'm thinking it'll make sense to modify our data model to bring the variable keys down one layer and make it simply (id, _value) where _value is a JSON column.14:21
thomasemOtherwise, the existing potential for a document or a string won't work.14:21
thomasemin the _value column14:22
thomasemSo, where we'd have (1, "name", "value") or (2, "name", '{"foo": "bar"}'), we'd instead have (1, '{"name": "value"}') and (2, '{"name": {"foo": "bar"}}')14:23
thomasemFYI, this is especially because MySQL will validate that it's valid JSON being stored in that column, which makes sense.14:24
thomasemespecially painful* :)14:25
fsaadmorning guys14:36
fsaadthomasem: thanks for the jsonpath query bug link14:37
fsaadzz_pwnall1337: ^^ https://bugs.launchpad.net/craton/+bug/167111614:37
openstackLaunchpad bug 1671116 in craton "Lacking support for searching nested variables" [High,New] - Assigned to Thomas Maddox (thomas-maddox)14:37
thomasemfsaad: you bet!14:59
*** VW has quit IRC15:01
*** VW has joined #craton15:02
jimbakersulo, re https://review.openstack.org/#/c/441644/, i only rebased that patch. the users FK change in the alembic should stand, since the model is what it is and that's a requirement of the model15:16
jimbakerbut i can remove NetworkInterface mixin of VariableMixin15:16
jimbakerwe almost certainly want the project dependency to prevent dangling; but this is related to https://bugs.launchpad.net/craton/+bug/1668081, so i think it's best to combine in the tests that demonstrate the fix in with this patch15:18
openstackLaunchpad bug 1668081 in craton "500 on resource deletes from foreign key constraint error" [Undecided,New]15:18
jimbakersounds good?15:18
thomasemActually, I'm wrong. It looks like SQLAlchemy converts the string to valid JSON.15:18
thomasemHandy. Albeit a bit awkward, still.15:18
thomasemI still think it makes more sense to push the key_ down into a JSON document, but I can truck forward with this, I think.15:19
openstackgerritSulochan Acharya proposed openstack/craton master: Adds project/user bootstrap command to dbsync  https://review.openstack.org/44317015:21
sulojimbaker: rgr, also if you want to wait, i can finish up this (not ready yet) and my concerns around15:21
sulohaving to do manual db stuff will be taken care of too15:21
thomasemhttps://gist.github.com/thomasem/8649f8e0b07573cfb82975fba2930fd915:22
jimbakersulo, exactly - we don't want people using raw SQL against the DB. or even model code :)15:22
thomasemExcept that guy Thomas.15:22
thomasemHe can.15:22
* thomasem snickers15:23
jimbakerthomasem, oh sure, we always let thomas do what he wants15:23
sulojimbaker: that patch is mostly taking shape .. i need to update docs etc .. and if y'all can give it a looksee to see everyone is happy with how that command is looking15:23
suloi am talking about https://review.openstack.org/#/c/443170/15:23
jimbakercool, will take a look15:23
thomasemsulo: added a couple comments to that review regarding setting up variable association reference for those.15:27
jimbakerthomasem, same here, but with regards to API key15:28
sulothomasem: huh .. i think vars association gets taken care of auto15:28
thomasemIt does?15:28
* sulo jumps into the db to check15:29
thomasemI thought it didn't and that's why I had to add https://github.com/openstack/craton/blob/master/craton/tests/functional/__init__.py#L17215:29
jimbakersulo, thomasem, it does15:29
suloyeah it does15:29
sulothomasem: i think thats only if you insert records15:29
jimbakerthat's the beauty of using model code15:29
thomasemI guess it's because of SQLAlchemy15:29
suloi am using orm15:29
suloyeah15:29
thomasemsulo: okay, great. Neeevermind!15:29
jimbakermodel code = SQLAlchemy ORM models for craton15:29
thomasemWhat I get for not pulling it down and checking myself before opening my mouth.15:30
jimbakersulo, i like the idea of driving this from conf vs my idea of just creating user/project15:31
jimbakerhowever, can we can make this a get_object_and_create if necessary pattern?15:31
jimbakerbasically what happens if it is run multiple times?15:32
sulojimbaker: it should error15:32
tojuvoneHi15:32
fsaadsulo: can join ?15:32
jimbakeryes, that it should do15:32
suloi was going to catch that and throw proper msg back15:32
jimbakerbut maybe it can report back the API key at that time15:32
jimbakeragain if you have access to dbsync, you have access to the db. so might as well be useful15:33
sulojimbaker: sure, makes sense15:33
sulowhats the suggestion though ? not sure i am following15:34
sulofsaad: joining15:34
jimbakersulo, basically report back the same info as if created15:34
fsaadthanks sulo, zz_pwnall1337 is trying to convince me to adopt a pet so be quick15:34
jimbaker(it can say however, already created user or project, but here's the relevant info)15:35
fsaadplease :)15:35
sulojimbaker: oh i see what you are saying ... cool, will do15:35
jimbakercool. basically there are two randomly created pieces of info, project_id and api_key. we don't want the user to have to look into the db with mysql/whatever to look up if they have lost accss15:36
openstackgerritThomas Maddox proposed openstack/craton master: Move to MySQL 5.7 and SQLAlchemy>=1.1.0  https://review.openstack.org/44318615:45
openstackgerritThomas Maddox proposed openstack/craton master: Move to MySQL 5.7 and SQLAlchemy>=1.1.0  https://review.openstack.org/44318615:50
thomasemAnd it begins.15:50
sulothomasem: for sqlalchemy verison .. you updatd global reqs ?15:51
thomasemsulo: global as in top-level requirements.txt?15:51
thomasemfor Craton... or does that have some additional implication?15:51
sulothomasem: no i mean https://github.com/openstack/requirements/blob/master/global-requirements.txt#L27615:52
thomasemsulo: Oh, no. Did we want to? I thought the update to SQLAlchemy >= 1.1.0 was only for our project?15:53
sulootherewise that will get overwritten next update no ?15:53
thomasemI don't know? Will it? Are projects not able to have their own overrides?15:53
suloor does that not happen for <15:53
suloi dunno ... i am curious how it worked15:54
thomasemI have no idea. I'm not all that familiar with how that works?15:54
suloi thought the req's check would fail15:54
sulosigmavirus: ^15:54
thomasemI would hope we could override.15:54
suloyeah, same15:54
sigmavirussulo: they should fail15:55
thomasemSo, what's the proper way to do this in OpenStack land?15:55
sigmaviruswe opted into g-r automated updates15:55
sigmavirusthomasem: propose an update to g-r directly15:55
suloyeah thats what i though15:55
thomasemOuch... I hope that doesn't cause a bunch of grief.15:56
thomasemLemme go do that.15:56
thomasemThanks for calling that out15:56
thomasemSo, then, I'd submit that review, and say this Depends-On that15:57
thomasemIn my Craton commit15:57
thomasem?15:57
thomasemI was really hoping to not impact a bunch of other projects, was my original intent.15:58
sigmavirusthomasem: so bumping minimum versions isn't a big deal16:04
sigmavirusthat said, the other projects have these changes auto-syncrhonized for them (as do we)16:04
sigmavirusso it's fine16:04
sigmavirusbeing part of openstack means being a noisy neighbor16:05
thomasemLol, I see16:05
thomasemOkay, thanks sigmavirus sulo :)16:05
jimbakeri don't think anyone wants to stick on an old sqlalchemy either. such pinning should at best be temporary, during a specific dev cycle16:08
jimbakerotherwise, tech debt of the worst kind16:08
sigmavirusjimbaker: it's not pinning16:10
sigmavirusit's minimum supported versiono16:10
sigmavirusopenstack doesn't pin or cap16:10
sigmavirusand they tend to bump minimums at the start of each cycle as appropriate and throughout the cycle for projects with hard dependencies on changes in newer versinos16:11
sigmavirusthomasem: I have to add a dependency to global-requirements anyway16:14
sigmaviruswant me to send teh patch to bump the sqla version?16:14
sigmavirusMight also be worth checking if anyone else is bumping that before you do it =)16:14
sigmavirusOh sqla *is* capped16:24
sigmavirusweird16:24
sigmavirushttps://review.openstack.org/42319216:24
sigmavirusthomasem: ^16:25
jimbakerinteresting commentary, not to mention the keystone specific usage16:27
*** jovon has joined #craton16:27
thomasemsigmavirus: If you're already under the hood, I would appreciate it!16:30
thomasemsigmavirus: ohhh gotcha lookie there16:30
jimbakerthe other thing is requesting storyboard project creation for craton. ideally we can get this in motion so we can start using next week16:32
openstackgerritThomas Maddox proposed openstack/craton master: Move to MySQL 5.7 and SQLAlchemy>=1.1.0  https://review.openstack.org/44318616:33
thomasem+116:33
sigmavirusjimbaker: have you talked to diablo_rojo about timelines for migration?16:35
sigmavirusjimbaker: diablo_rojo hangs out in #openstack-dev so might be best to talk to them there about migration details?16:36
sigmavirusalternatively #openstack-infra probably16:36
thomasemfsaad: you making it to sync?16:46
fsaadthomasem: can't, please go on16:46
fsaadI did have a meeting a bit ago with sulo and zz_pwnall1337 on which we agreed pwnall has and will push the osic inventory and facts to sulo's demo box before next demo, so things looking good.16:49
openstackgerritIan Cordasco proposed openstack/python-cratonclient master: Add Betamax for testing  https://review.openstack.org/44216516:53
*** VW_ has joined #craton16:56
*** VW_ has quit IRC16:56
*** VW_ has joined #craton16:57
*** VW has quit IRC16:57
thomasemfsaad: Sounds good! Thanks for the update.17:01
fsaado/17:05
thomasemhttps://review.openstack.org/44324017:17
thomasemopt-out of synchronization ^^17:17
sulojimbaker: thomasem: fsaad: sorry i had to miss the meeting. had to run out to get something and got stuck in traffic17:18
thomasemsulo: no worries!17:19
thomasemWe know you wouldn't miss seeing our glowing faces unless you had something come up.17:19
fsaadno prob sulo, I'll cancel the remaining ones, if we have a need to do syncs we can do adhoc17:20
thomasem+117:20
sulothomasem: heh fsaad: +117:20
openstackgerritThomas Maddox proposed openstack/craton master: Move to MySQL 5.7 and SQLAlchemy>=1.1.0  https://review.openstack.org/44318617:21
jimbakertojuvone, thanks for the patch, just need a minor change done for https://review.openstack.org/#/c/443129/ and we get in17:22
tojuvonejimbaker, Thanks, I'll update17:24
tojuvonejimbaker, There would be plenty17:25
tojuvonejimbaker, sorry, I was to say about proxy stuff in different places, but that is irrelevant for most17:26
openstackgerritTomi Juvonen proposed openstack/craton master: docker install documentation not consistent  https://review.openstack.org/44312917:30
* sigmavirus wishes we were using py.test17:59
sigmavirus--pdb makes everything better17:59
*** VW_ has quit IRC18:03
*** VW has joined #craton18:05
*** VW has quit IRC18:06
*** VW_ has joined #craton18:06
jimbakersigmavirus, what do we need to move to pytest? i'm not a regular user of pdb, but i can see it be highly helpful in this case18:17
openstackgerritMerged openstack/craton master: docker install documentation not consistent  https://review.openstack.org/44312918:19
sigmavirusjimbaker: so generally trying to pry into running code in tests is a pain with testtools/testr18:20
sigmavirusAlso they capture all stdout/stderr printing and lose them typically18:20
sigmavirusso18:20
sigmavirusPrint debugging doesn't work18:20
sigmaviruslog debugging could work but that's not as useful ime18:20
sigmavirusSo, I'm going through the pains of making pdb work18:20
sigmavirusIt's not a big deal18:20
sigmavirusAnd switching to pytest wouldn't be a terrible amount of work18:21
sigmavirusBut it's a distraction18:21
sigmavirusI just yearn for testing tools designed in the last 10 years that focus on usability rather than ... Idk what18:21
sigmaviruseven nosetests provides similar funcationality18:21
sigmavirusEven if our craton tox.ini doesn't let use use it directly18:21
*** Syed__ has joined #craton18:45
thomasemBah, change to MySQL 5.7 + SQLAlchemy 1.1.6 breaks variables query AND... seeing empty result with SELECT * FROM variables WHERE value_ IN ("juno", "swift"); after upgrade. :\18:59
thomasemIsolating which part breaks it18:59
thomasemI'm betting it's the switch to JSON type, because it works when I wrap the values in single quotes.18:59
sigmavirusthomasem: interesting19:02
thomasemFun fun19:04
thomasemYep. That's it. JSON column is what breaks it.19:06
thomasemSQLAlchmy 1.1.6 + MySQL 5.7 works fine as long as that column winds up being TEXT instead of JSON type.19:07
sigmavirushuh19:08
jimbakersigmavirus, agreed about print debugging being less useful with current setup19:09
jimbakerthomasem, is this a problem with the use of sqlalchemy_utils?19:10
thomasemjimbaker: yeah, it's the way it papered over the lack of JSON support in MySQL19:10
jimbakeryeah, sort of what i expected19:10
jimbakerso maybe we can propose a patch for them19:10
thomasemby storing literal JSON strings.19:10
jimbakerit's a pretty small wrapper iirc19:11
jimbakerin SA utils19:11
thomasemTo be friendly? If we're switching to proper JSON columns, why not fix the codebase to handle those values properly?19:11
thomasems/the codebase/craton/19:11
thomasemAssuming there is a 'proper' way, of course. :D19:12
jimbakerhttps://github.com/kvesteri/sqlalchemy-utils/blob/master/sqlalchemy_utils/types/json.py19:12
thomasemAnd it's not just flat-out bugged.19:12
jimbakeri think there's a proper way. the json support in sqlalchemy_utils is postgres specific19:13
jimbakeri don't see any reason why we cannot just use our own type for now19:13
thomasemWell, the problem is partially in the DB. So, the DB tries to be friendly, but in doing so causes us to have a problem when we go to the "correct" way.19:13
jimbakerand see if we can get that package to generalize19:13
jimbakernot certain i follow19:13
thomasemIt's storing json.dumps("juno") and json.dumps(True), where you'll see a value_ like '"juno"' and 'true' stored.19:14
thomasemStrictly speaking, then, you should have to query for that value like SELECT * FROM variables WHERE value_='"juno"';19:14
thomasemAnd SELECT * FROM variables WHERE value_='true';19:15
thomasemBut, it tries to be friendly and interprets "juno" as '"juno"'.19:15
jimbakeryeah. no friendliness please19:15
thomasemFor TEXT columns19:15
jimbakerso we want pure JSON columns19:15
thomasemYeah, it's not always a good idea to be helpful. Winds up with bugs. :P19:15
thomasemYes.19:15
thomasemWhich means we need to go a stricter route19:15
thomasemwhere you query for '"juno"'19:16
thomasemso, you query for json.dumps(value_)19:16
thomasemessentially19:16
thomaseminstead of mixing JSON and strings and having the DB try to be helpful19:16
thomasemUsing strings will only work for if you're querying for a single value. If you're trying to do an IN (...), it won't work consistently unless you're strict about the values in the list being actual JSON.19:17
thomasemAnyway, I hope I explained that well enough. Happy to showcase in Vidyo, too.19:17
jimbakermakes sense to me19:18
thomasemOkay, good.19:19
thomasemSo, now I need to figure out how to make it work consistently!19:19
thomasemWant to maintain the status quo before adding things on top.19:19
jimbakerthomasem, and again, we will be ensuring that JSON types are stored as JSON columns19:21
jimbakerso this will stack on my alembic change as well19:21
jimbakerso instead of sqlalchemy_utils.types.json.JSONType, we use a real JSON type for that column19:23
thomasemjimbaker: yep, that's in my change on top of yours.19:23
thomasemNow, the question is, do we want to do that for all occurrences here, or just variables?19:23
thomasemI guess one will go away with your alembic change19:23
thomasem("roles")19:23
thomasemAssuming that's still in the change19:24
jimbakeryeah, roles is gone19:24
jimbakerthis is exactly why YAGNI needs to apply here :)19:24
thomasemvlans is the other one19:24
sulouff .. pymysql raises IntegrityError for duplicate entires19:24
jimbakerand that can stay for now19:24
thomasemfor network_devices19:24
thomasemjimbaker: do you mean that can stay sqlalchemy_utils.types.json.JSONType?19:25
jimbakersulo, does pymysql give you the cause?19:25
jimbakerno, json means json :)19:25
jimbakerno more text19:25
thomasemCool. Agreed. I'll address both, then.19:25
thomasembrb19:28
thomasemback19:41
*** VW has joined #craton19:57
*** VW has quit IRC19:59
*** VW has joined #craton19:59
*** VW_ has quit IRC20:01
*** VW has quit IRC20:03
*** VW has joined #craton20:04
*** VW has quit IRC20:06
*** VW has joined #craton20:06
openstackgerritIan Cordasco proposed openstack/python-cratonclient master: Add Betamax for testing  https://review.openstack.org/44216520:10
thomasemsaddyface /craton/lib/python3.5/site-packages/pymysql/cursors.py:166: Warning: (1235, "This version of MySQL doesn't yet support 'comparison of JSON in the IN operator'")20:50
openstackgerritIan Cordasco proposed openstack/python-cratonclient master: Add Betamax for testing  https://review.openstack.org/44216520:55
jimbakerthomasem, oh21:04
*** VW has quit IRC21:08
jimbakerthomasem, and you are running the latest release of PyMySQL right? do you have the full traceback?21:13
*** VW has joined #craton21:22
thomasemjimbaker: I think it's latest, I'll check. But, that error comes from MySQL 5.7 itself: | Warning | 1235 | This version of MySQL doesn't yet support 'comparison of JSON in the IN operator' |21:43
thomasemwhen I do SHOW WARNINGS21:43
thomasems/error/warning/21:43
thomasemPyMySQL==0.7.1021:44
thomasemthat's latest21:44
thomasemjimbaker: works fine when I switch to using OR, even though that's a performance hit (saddyface x2).21:54
thomasemjimbaker: I rebased and since your change is the parent of mine, I suspect it'll push a rebase up to your change. You cool w/ that?22:00
jimbakerthomasem, absolutely fine with that22:08
jimbakerand that makes sense, i was wondering where in PyMySQL it was generating that error22:09
jimbakerre performance hit - let's focus on functionality first. i think we can figure out any necessary optimizations22:09
jimbakerat the right time22:09
openstackgerritThomas Maddox proposed openstack/craton master: Move to MySQL 5.7 and SQLAlchemy>=1.1.0  https://review.openstack.org/44318622:12
openstackgerritThomas Maddox proposed openstack/craton master: WIP: Variable search for resources now uses resolved variables.  https://review.openstack.org/44092922:12
*** VW has quit IRC22:35
*** jovon has quit IRC22:42
openstackgerritThomas Maddox proposed openstack/craton master: Move to MySQL 5.7 and SQLAlchemy>=1.1.0  https://review.openstack.org/44318622:50
thomasemAlright, I hear the dinner bell. Have a lovely evening, everyone!22:50
thomasemjimbaker: Agreed. I don't want to hamstring us over unknown unknowns. :)22:51
*** david-lyle has quit IRC23:56

Generated by irclog2html.py 2.14.0 by Marius Gedminas - find it at mg.pov.lt!