Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IPMI network allocations API with pool support #513

Merged
merged 18 commits into from
Apr 6, 2017

Conversation

byxorna
Copy link
Contributor

@byxorna byxorna commented Mar 6, 2017

We have a bit of an odd IPMI network topology, and I have been wanting to add an endpoint to allow IPMI details to be generated separate from asset creation. This diff enables multiple OOB networks to be managed, and gives users the flexibility to generate IPMI details based on information collected at induction time. For example, one could imagine an induction process in Genesis that:

  • creates the asset without IPMI
  • performs LLDP collection
  • call #ipmi_allocate(tag, pool: ...) based on the connectivity discovered and specify a specific IPMI address pool

Ive updated collins-shell and collins-client with this functionality as well.

cc @michaeljs1990 @defect @roymarantz @alex-laties @vhp @qx-xp

Fixes #512, docs in #514

@michaeljs1990
Copy link
Contributor

michaeljs1990 commented Mar 6, 2017

Just to keep everything in the PR. I had an oob (lol i couldn't help myself) conversation with gabe that since you can generate IPMI at asset creation time it would be cool if you could also pass it a pool to allocate from since you could have gotten LLDP info before asset creation.

I have also run into the issue of people manually creating an asset for JBODs that need to go into a specific pool and then they message me later to correctly allocate the ip address for it.

@byxorna
Copy link
Contributor Author

byxorna commented Mar 6, 2017

@michaeljs1990 ive updated diff to support #create!('tag', ipmi_pool: "FOO"), as well as the documentation update that goes along with it. Tests are broken for a few reasons:

  1. i bumped the gem dependencies for collins-shell (as it requires the latest collins-client), but that version doesnt exist in rubygems yet
  2. flaky scala solr tests bailed :( idk how to tell travis to rerun though...

@defect review appreciated!

@michaeljs1990
Copy link
Contributor

michaeljs1990 commented Mar 7, 2017

Was messing around with the docker build and I believe I ran into the 500 error you were talking about. I dug into it a bit and tracked it down to app/collins/util/Tattler.scala:83 which I believe is trying to grab a single asset to use for logging. Since it's currently not set I think it's returning whatever is set as the multicollins asset in the config and trying to log to that.. which also doesn't exist as an asset in collins.

# production.conf
features {
  syslogAsset = tumblrtag1
}

Needs to be set so we can use the super special logger. I should make a task for documenting this and then maybe just having it log to a central place eventually. The error is also a little vague since you can have it set but to a non valid asset and it will throw neither features.syslogAsset or multicollins.thisInstance were specified

@byxorna
Copy link
Contributor Author

byxorna commented Mar 7, 2017

@michaeljs1990 so, i think this is a race in startup with solr initial indexing; ive seen this before, and the tumblrtag1 asset (inserted here https://github.com/tumblr/collins/blob/master/conf/evolutions/collins/2.sql#L61 by evolutions) is not always indexed by solr. Manually hitting /admin/solr to reindex assets sorts the problem out. We should make a new issue to debug if the issue I already had open in #403 is different.

@@ -1,12 +1,12 @@
FROM java:8-jre
FROM openjdk:8-jre
MAINTAINER Gabe Conradi <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably should change the MAINTAINER to [email protected]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that alias accessible from outside the company? I thought there was a public maintainers mailing list for collins?

MAINTAINER Gabe Conradi <[email protected]>

# Solr cores should be stored in a volume, so we arent writing stuff to our rootfs
VOLUME /opt/collins/conf/solr/cores/collins/data

COPY . /build/collins
RUN apt-get update && \
apt-get install --no-install-recommends -y openjdk-8-jdk zip unzip ipmitool && \
apt-get install --no-install-recommends -y openjdk-8-jdk openjdk-8-jdk-headless zip unzip ipmitool && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't openjdk-8-jdk-headless already installed in the base image?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this is the JRE image. We install only the JDK for the duration of the build, then remove it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, thats only JRE. We need JDK for the build

@@ -1,12 +1,12 @@
FROM java:8-jre
FROM openjdk:8-jre
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you though of using openjdk:8-jre-alpine ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses muscl instead of glibc, and didnt want to take a chance of breaking something. Plus, for downstream consumers, they expect to be able to apt-get install stuff for collins to hook into. We could maintain 2 different Dockerfiles that produces a debian and alpine build, but probably outside the scope of this diff

@byxorna
Copy link
Contributor Author

byxorna commented Mar 13, 2017

@roymarantz @defect thumbs bump?

@byxorna byxorna requested a review from roymarantz March 14, 2017 16:48
@byxorna
Copy link
Contributor Author

byxorna commented Mar 16, 2017

@roymarantz @defect second bump! I would like to land this soon, along with the already approved #514 :)

@byxorna byxorna merged commit 3dba29d into tumblr:master Apr 6, 2017
# ~~~~~

include "dev_base.conf"
include "test_base.conf"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aha! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, sorry! I figured i had enough of changing things for dev and accidentally breaking tests -_-

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants