Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-04-12
14:33:03 stephenfin mriedem: back to dhcp_domain it is ^
14:37:48 melwitt I was thinking about it in the inverse, keep running under zero is returned, not keep running until nonzero is returned. no wonder I was confused xD
14:43:30 melwitt *until
14:43:47 mriedem i also commented because everyone loves a good bike shed, not sure how my comments align with others
14:44:02 dansmith melwitt: well, I'm just going based on the existing behavior, not necessarily the right behavior
14:44:14 melwitt I see, yeah
14:45:48 dansmith mriedem: my preference is to just highlight the error and stop processing records, not skip and continue
14:45:59 dansmith which is why I think we should bail,
14:46:16 dansmith but I think the existing return code behavior means we return 1 until we find nothing else to do, then return 0
14:46:27 dansmith and in that case, if we bail, we should return 1 so that something knows that there may be more stuff to do
14:47:37 mriedem the existing return code behavior depends on the --strict option
14:48:00 dansmith ...right
14:48:03 mriedem if you don't specify --strict it returns 0 unless something blows up
14:48:32 dansmith ...right, are you saying you want to preserve the non-strict nonzero behavior on error?
14:49:04 dansmith I thought she was specifically asking about what to do if --strict was passed
14:49:09 melwitt wait, I thought we return 0 until there's nothing more to do, and then return 1 when there's nothing to do?
14:49:14 mriedem if i'm running this on a cron or something and not specifying --strict, i don't want it to error out if it got a duplicate entry error because it either did the job or there wasn't anything to do, so i don't care
14:49:55 dansmith mriedem: but the job may very well be unfinished
14:50:13 dansmith melwitt: not currently, AFAICT
14:50:16 mriedem if we just blow up and stop processing sure
14:50:30 dansmith melwitt: [07:46:16] <dansmith>but I think the existing return code behavior means we return 1 until we find nothing else to do, then return 0
14:50:32 tssurya dansmith: what I meant was to *not* stop if we found duplicates, I don't want my periodic job stopping and complaining that it already found a host was already mapped and now it doesn't want to proceed
14:50:40 tssurya but it could also just be me :)
14:50:47 mriedem tssurya: i think you and i are saying the same thing
14:50:48 kashyap alex_xu: When you're about, the more I think about this, the more I'm not sure if we should do this: https://review.openstack.org/#/c/642030/
14:51:00 kashyap alex_xu: I'll think more and write on the change next week.
14:51:06 tssurya mriedem: yea just replied to your comment
14:51:14 tssurya (on the patchset)
14:51:20 dansmith tssurya: okay, I just don't see the point of having two threads which are clearly fighting continue to fight, processing all the records in the db
14:52:28 dansmith in the case of there only being a few new nodes, it's not a problem,
14:52:47 mriedem stephenfin: more comments
14:53:10 dansmith but if you're turning on several thousand and running this in parallel for each node you added (which I think was the original assertion), you could have thousands of threads, running across many controller nodes, processing every record, and all but one are losing
14:53:17 stephenfin efried: I've made a horrible mistake trying to bump hacking
14:53:19 stephenfin FML
14:53:27 stephenfin Oh, wait
14:53:36 efried stephenfin: It's educational at worst
14:53:55 finucannot Running tox -e pep8 and it just keeps on scrolling...
14:54:09 finucannot I've no idea how this wasn't picked up locally before either. Very strange
14:54:18 fried_rice finucannot: I've got 1314 lines of it
14:54:20 tssurya dansmith: fair enough
14:54:56 finucannot Hopefully most of them are W504 (which can be disabled because it's W503 or W504, choose one)
14:54:58 finucannot Still, ugh
14:55:12 fried_rice finucannot: I put how many of each in my last comment.
14:55:23 fried_rice 1055 of them are W504
14:55:37 tssurya dansmith: yeah you have a valid point, its just that the behaviour of the command seemed weird when its all about discovering hosts
14:59:31 dansmith tbh, I think what melwitt has is probably what I would want.. we retain the existing behavior of returning nonzero for the duplicate case, we just get a nice message about why
14:59:39 dansmith no behavioral change at all, just easier for the humans
14:59:57 dansmith we can change the behavior too, but that means deployment people likely have to change their stuff
15:01:00 tssurya yea return code wise no behaviour change, just that now the command stops because it found an already discovered host :D
15:01:01 melwitt yeah, that's what I was thinking with return 1, was today it traces and returns nonzero, so keep the behavior, just warn about it instead of tracing
15:01:14 dansmith tssurya: it stopped before
15:01:14 tssurya but I guess I am okay with it considering we run it as a periodic task
15:01:25 melwitt I hadn't been thinking about the backwards (from the way my brain works) way that it currently returns values
15:01:28 dansmith tssurya: cern runs it as a periodic?
15:01:43 tssurya dansmith: no
15:01:47 dansmith tssurya: okay good :D
15:01:51 tssurya I meant in general :)
15:01:55 dansmith gotcha
15:02:15 dansmith melwitt: yeah, I think the return codes are a bit wonky, but what you have is an improvement for humans
15:02:30 melwitt tssurya: yeah, today the command stops because it raises an exception all the way up
15:03:37 dansmith melwitt: does this also continue to raise an exception for the periodic?
15:03:54 dansmith melwitt: because I thought we were going to catch and log a warning for the periodic case to avoid the trace in the logs
15:04:36 melwitt it would raise HostMappingExists yeah. I missed the point about catching and logging for periodic only
15:04:38 dansmith or did you want to do that in a separate patch
15:04:39 dansmith ?
15:05:03 dansmith don't you think that's better than a trace? LOG.warning('This should only be enabled when ...')
15:05:09 tssurya melwitt, dansmith: yeah ok thanks I am aboard the plan then
15:06:17 melwitt dansmith: yeah I think that's better, I just missed it and need to add it. what do you mean about "this should only be enabled when" though?
15:06:26 tssurya melwitt: let's also add something in the nova-manage docs like a fat note
15:06:43 dansmith like "this should only be enabled on a single scheduler" or something
15:06:57 melwitt dansmith: I'm concerned about mschuppert's comment here though https://bugzilla.redhat.com/show_bug.cgi?id=1698630#c7 does this mean he expects it to return zero in the dupe case?
15:06:58 openstack bugzilla.redhat.com bug 1698630 in openstack-tripleo-heat-templates "deployment fails running nova_cellv2_discover_hosts process with a duplicate key" [High,New] - Assigned to owalsh
15:07:10 melwitt dansmith: ack got it
15:07:42 melwitt tssurya: I think that docstring gets put in the docs right? let me check actually
15:08:07 dansmith melwitt: I dunno what of that you think is related to his expectation of the return value
15:08:29 dansmith melwitt: but if he's expecting something that isn't in line with the current behavior, he's probably expecting the wrong thing :)
15:08:34 melwitt "With the proposed change to nova to just report a warning we'll not fail in that case."
15:08:46 dansmith *my* concern about that comment is that he's talking about running this *on* the computes
15:09:13 owalsh dansmith: on one compute
15:09:16 dansmith melwitt: oh is he talking about your proposed change/
15:09:27 dansmith owalsh: doesn't matter.. any compute :)
15:09:29 owalsh melwitt: he's assuming rc==0, yes
15:09:33 melwitt I assumed so. I had linked it on the BZ
15:09:42 owalsh dansmith: yea... not how I did it initially but reasons...
15:10:23 dansmith owalsh: that's unfortunate.. giving computes credentials to the api DB just to run this thing the controllers should do.. do those creds persist on the compute?
15:10:47 dansmith melwitt: okay I hadn't read it that way.. so he's just misunderstanding what your patch is doing you mean'?
15:11:09 melwitt I'm concerned that he is, yeah
15:11:43 melwitt tssurya: you're right, I need to add to the docs separately, the docstrings don't go in there automatically http://logs.openstack.org/47/651947/2/check/openstack-tox-docs/d6d7085/html/cli/nova-manage.html
15:11:48 dansmith melwitt: but he's quoting from owalsh who says they'll use a retry loop, which is fine yeah?
15:12:49 melwitt yeah, I guess so. I was hyperfocusing on the reply and not the quote
15:13:03 owalsh dansmith: not sure where the creds are coming from now actually... previously when I moved this to the computes I embedded them in the script and unlink the script as soon as it was run to avoid leaving creds around
15:13:16 dansmith owalsh: okay
15:13:31 owalsh dansmith: but it's been rewritten since, I'll take a look
15:15:50 dansmith owalsh: "talking to the api database" is something we generally expect a compute not to ever do.. I guess in this case you're using it as the admin node since you're just taking action on that one,
15:16:06 dansmith but in ansible lingo, I would "delegate" the discovery task to one of the controller nodes
15:16:47 owalsh dansmith: yea, the reasons for this were that the controller nodes were being excluded from tripleo scale outs to speed things up, so it had to be on a compute... there isn't anything else
15:17:49 dansmith right, because tripleo/heat reasons yeah? If it was pure ansible delegating a single admin command to one controller node after you're done would be easy, but I know that heat probably makes that hard
15:18:25 owalsh might be possible now that tripleo == heat generating ansible playbook, but not sure if it would backport
15:27:28 fried_rice finucannot: a little sphinx help?
15:27:35 finucannot shoot

Earlier   Later