[merged] [WIP] Cpd 126 config

[mbarnes]

Current status:

• CLI parsing is more or less done.
• Bootstrap playbook integration is duct taped together well enough I think to preserve current functionality and near-term expected usage patterns, but further discussion and rework necessary. We're allowing for more configuration flexibility than we support.
• Next to-do is come up with a new model class for all the stuff we fetch through cli_etcd_or_default and integrate it into the storage handler framework.
• Still need tests and docs for all this.

We should also define the register_store_handler kwargs a little better. It doesn't have to be (and probably shouldn't be) a straight pass-through to the kube or etcd client API. For instance, would it be better for the UX to take a URI string for the remote storage address or keep arguments split into host/port/protocol components?

[ashcrow]

For instance, would it be better for the UX to take a URI string for the remote storage address or keep arguments split into host/port/protocol components?

Agreed. I started to use the config.$THING portion for the Kubernetes annotation backend. We could follow that or we could send the entire config in and have the StoreHandler itself choose what to read. Thoughts?

[ashcrow]

It's looking good so far. My main concern is the same as yours in terms of standardizing the data going into the StoreHandler.

[mbarnes]

Agreed. I started to use the config.$THING portion for the Kubernetes annotation backend. We could follow that or we could send the entire config in and have the StoreHandler itself choose what to read. Thoughts?

It looks like you're already sending the entire config to the StoreHandler, which I think is the right thing to do. The handler classes are where we'd translate human-friendly config inputs to whatever form the relevant API expects.

Slight preference for reading URLs from the config file / CLI and letting a StoreHandler parse and validate it, just because that's what I see most other things under /etc doing. But in any case, I think my point was we need to document these options.

[ashcrow]

Slight preference for reading URLs from the config file / CLI and letting a StoreHandler parse and validate it, just because that's what I see most other things under /etc doing.

:+1: How about this ....

• Config gets updated and no longer uses urlparse itself. That will now be done at the handler level.
• Define each sub config. For instance, Config.etcd should have known attributes that can be looked up.
• register_store_handlers config parameter should expect only its specific section of the config. For etcd it should expect Config.etcd.

[mbarnes]

Define each sub config. For instance, Config.etcd should have known attributes that can be looked up.

Well our JSON configuration allows for doing weird things we don't currently support like defining multiple Kubernetes handlers for different sets of model types. I have no idea why you'd want to do that, but it's possible with our format. Whereas the Config class assumes one etcd and/or one kubernetes service.

For that reason I recall I was working toward eliminating the Config class, but I think I left off trying to address KubeContainerManager which still takes a Config instance.

I suppose a quick and dirty hack is to explicitly limit the configuration to one handler definition per backend (i.e. one etcd handler and one kubernetes handler).

register_store_handlers config parameter should expect only its specific section of the config. For etcd it should expect Config.etcd.

I think we have that already, though the config object is just a plain dictionary.

[ashcrow]

Ok. If we remove Config from the equation and deprecate how would this look? I think I'm not understanding :smile:.

[mbarnes]

Ok. If we remove Config from the equation and deprecate how would this look? I think I'm not understanding

Short answer: I'll let you know when I finish this PR. :smiley:

Perhaps have KubernetesStoreHandler create a KubeContainerManager, passing it the same config dictionary received from register_store_handler. Just need to work out how to hand it off to the investigator process.

[ashcrow]

Short answer: I'll let you know when I finish this PR. :smiley:

:laughing: I follow now. I'll leave you to it :smile:

[mbarnes]

@ashcrow I think the code for this is 95% done. Still got lots of docs and tests to write next week, but I'd appreciate at least a preliminary review to make sure we're in agreement on all this stuff.

[ashcrow]

@mbarnes sounds good. Going through it now.

[mbarnes]

@ashcrow Getting that weird pickle error in the Travis build again, but this is ready for review and all tests pass here.

[ashcrow]

@mbarnes the last build seemed to work OK in Travis. It's showing a few pep8 issues though.

src/commissaire/jobs/investigator.py:26:1: F401 'commissaire.containermgr.kubernetes.KubeContainerManager' imported but unused
src/commissaire/store/__init__.py:19:1: E302 expected 2 blank lines, found 1
src/commissaire/store/etcdstorehandler.py:50:13: E129 visually indented line with same indent as next logical line
src/commissaire/store/kubestorehandler.py:56:13: E129 visually indented line with same indent as next logical line

[mbarnes]

Oh wow, my eyes must have just skipped over that part of the report.

Also, I wasn't able to reproduce those errors until I updated flake8 from 3.0.0 to 3.0.3 just now. Starting to think I should have a cron job to keep these requirements up-to-date.

I'll push some fixups in a bit.

[mbarnes]

@ashcrow :arrow_up:

[ashcrow]

@mbarnes will review shortly!

[ashcrow]

@rh-atomic-bot try

[rh-atomic-bot]

:hourglass: Trying commit 52fa0e7 with merge 88a0ecb...

[rh-atomic-bot]

:sunny: Test successful - travis State: approved= try=True

[ashcrow]

This looks good to me @mbarnes. Did you do any manual testing to make sure that you could boostrap into kube cluster?

[mbarnes]

I haven't tested that but I can shortly.

[ashcrow]

@mbarnes perfect. That's the only question about the changes I have. Everything looks good!

I'll do a few tests myself.

[ashcrow]

@mbarnes I found one issue that I think is OK to push to another set of work:

If there is no etcdstorehandler defined flanneld is unable to get it's configuration data. I'll backlog a card for us to come back and fix that. In the mean time, in your documentation, please note that etcd is still required for flanneld.

I'll wait for your test results before moving forward but this gets a big :+1: from me!

[mbarnes]

Another minor thing I just bumped into is logging configuration has to be local now because logging gets set up before store handlers. Commissaire complained at me because I don't have an /etc/commissaire/logger.json defined nor did I happen to be in the git repo directory.

For ease of use "out of the box" I think the conf/logger.json content should be built-in, and maybe have a --dump-logger-json CLI option which dumps a default logger.json as a starting point for further customization.

Would be one less annoyance when trying out Commissaire for the first time.

[ashcrow]

[multi-host-mgr] Decouple flanneld configuration from etcd

[mbarnes]

This is the first time I've tried the new Kubernetes integration, so I'm fumbling around a bit. I created a new Kuberenetes-type cluster and added a host to it, but after bootstrapping the host status says "host_only" instead of "kubernetes". Trying to investigate that.

But the configuration seems to be working at least.

[ashcrow]

@mbarnes the host_only thing is a bug. I'll fix it in a different PR.

[mbarnes]

Another configuration got'cha: with the wildcard support in the models list, Commissaire needs to catch model types accidentally getting registered for multiple store handlers. Especially since the default pattern is '*'. That's another card.

[ashcrow]

[ashcrow]

@rh-atomic-bot r+

[rh-atomic-bot]

:pushpin: Commit 52fa0e7 has been approved by ashcrow

[rh-atomic-bot]

:hourglass: Testing commit 52fa0e7 with merge 7249d05...

[rh-atomic-bot]

:sunny: Test successful - travis Approved by: ashcrow Pushing 7249d05f18490c012def22cefe632fe0c8c537da to master...