Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-04-12
14:10:58 dansmith I guess it's the unix "everything is a file" philosophy :)
14:11:43 openstackgerrit Stephen Finucane proposed openstack/nova master: conf: Undeprecate and rename the 'dhcp_domain' option https://review.openstack.org/480616
14:16:02 stephenfin mriedem: I had to move that option in order to add it to the metadata file, so I also took the opportunity to rename it (since nova-net should be going away this cycle). Not entirely happy with the name so open to suggestions
14:21:22 mriedem stephenfin: why not just leave the option name as dhcp_domain?
14:23:53 mriedem couple more comments
14:24:02 mriedem i'm not sure what you mean by "its been repurposed"
14:26:24 kashyap stephenfin: Bike-shedding: if the new name is "domain", beware that Nova frequently uses that term in context of "libvirt domains"
14:26:24 stephenfin mriedem: It seemed odd, given that it's not really anything specific to DHCP as used here
14:26:25 melwitt dansmith: could you take a look at the question I asked on the discover_hosts collision patch warn? I'm getting confused about whether I should return 0 if strict=False https://review.openstack.org/651947 and then also tssurya pointed out that map_instances skips DBDuplicateEntry
14:26:49 stephenfin kashyap: Yeah, it also easy interpret as the domain of the metadata service or something. Not a fan
14:27:10 stephenfin Eh, I haven't got a better idea so back to dhcp_domain it is
14:28:15 kashyap stephenfin: fqdn_name?
14:28:24 mriedem just leave it dhcp_domain
14:28:33 kashyap Yeah
14:28:50 kashyap (Also: the 'n' in 'fqdn' is name)
14:29:36 artom RAS syndrome!
14:32:12 dansmith melwitt: done
14:32:18 openstackgerrit Stephen Finucane proposed openstack/nova master: conf: Undeprecate and move the 'dhcp_domain' option https://review.openstack.org/480616
14:32:27 melwitt dansmith: ty
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 :)

Earlier   Later