Make OBMd connection info required. #991
Conversation
|
Suspicions about site-layout.json were correct. I pushed a fix for the depolyment tests. It looks like there may also be some failures in the migration tests; will investigate more soon. |
zenhack
force-pushed
the
obmd-api-changes
branch
2 times, most recently
from
2b3c022
to
4e97d79
Compare
|
Still tracking down some places where stuff needed to change, but should be working soon. This is ready for review; there aren't going to be any big structural changes. The amount of time a type system would have saved me on this one is insane. |
zenhack
force-pushed
the
obmd-api-changes
branch
from
561ce26
to
eed2b8b
Compare
|
Ok, CI is passing. |
|
this can be rebased now |
zenhack
force-pushed
the
obmd-api-changes
branch
from
5ba4101
to
d0bfc08
Compare
|
@naved001, done. |
Pull Request Test Coverage Report for Build 1839
💛 - Coveralls |
|
Travis failure seem to be because of a new pycodestyle release, as of today; everything is explained by the release notes: https://pypi.python.org/pypi/pycodestyle/2.4.0 I can fix these, but it might be better to do so separately, as unrelated style changes will make the diff harder to follow. |
After this PR is approved, you could push one last commit to fix pylint errors before we merge it. Besides the pylint errors, there are a bunch of line-too-long errors. Edit: Dont know why I kept thinking it was pylint that got upgraded. |
|
I think the errors are from pycodestyle, not pylint, but sounds good.
The line too long errors are for bits of docstring that were already
there; see the release notes, apparently there was a bug where it was a
bit more lax before.
Quoting Naved Ansari (2018-04-11 10:23:14)
… I can fix these, but it might be better to do so separately, as
unrelated style changes will make the diff harder to follow.
After this PR is approved, you could push one last commit to fix pylint
errors before we merge it.
Besides the pylint errors, there are a bunch of line-too-long errors.
--
You are receiving this because you were assigned.
Reply to this email directly, [1]view it on GitHub, or [2]mute the
thread.
Verweise
1. #991 (comment)
2. https://github.com/notifications/unsubscribe-auth/AA18Prawp4mbds-w8_GpeIxrb_l9ZIE8ks5tnhHSgaJpZM4TLVHs
|
| register all of your existing nodes with OBMd. | ||
| * Ensure that both the extensions ``hil.ext.obm.ipmi`` and | ||
| ``hil.ext.obm.mock`` are listed in ``hil.cfg``. | ||
| * Run ``hil-admin db create``. |
There was a problem hiding this comment.
I might have lost context, but could you remind me why are we creating the tables for the mock obm if it didn't originally exist?
There was a problem hiding this comment.
I don't think it was actually discussed; some of this is future-tense for a migration script that's not part of this pr; I started hacking on this by deleting the old drivers and writing migration scripts, but decided this was a better ordering, since that basically required me to do everything at once, and I didn't want to burden you guys with reviewing everything at the same time. But I left the release notes in there.
Here's the migration script:
zenhack@e0767f1#diff-3801d3778ab81ddb9e26cf48f8663114R1
Per the comment at the top, the reason we need both drivers loaded is because the script "merges" their branches into the branch for hil core, so they have to exist.
docs/UPGRADING.rst
Outdated
|
|
||
| * HIL now depends on OBMd to manage obms; the built in driver support has | ||
| been removed. Upgrading requires several steps: | ||
| 1. set up OBMd. |
There was a problem hiding this comment.
maybe add a link to the OBMd repo?
Also, how are we going to run the Go server? Run the go webapp directly, or have apache in front of it?
There was a problem hiding this comment.
Added a link. Go's net/http package is a production ready web server, so it doesn't need another server in front of it, but you could do so if e.g. you have other web stuff on the same machine and want to share a port.
There was a problem hiding this comment.
I opened an issue CCI-MOC/obmd#15 to better document the OBMd deployment process.
tests/unit/client.py
Outdated
| C.node.register("dummy-node-04", | ||
| "http://obmd.example.com/node/dummy-node-02", | ||
| "secret", | ||
| # Non-existant subtype. |
There was a problem hiding this comment.
lgtm. Though I think we should let naved and Ian @Izhmash to give the +2 since they are more familiar with obmd.
Also, it seems that obmd will become a hard dependency in 0.4 release. Should we create a separate branch for 0.3 release?
| def node_register(node, obmtype, hostname, username, password): | ||
| def node_register(node, | ||
| obmd_uri, | ||
| obmd_admin_token, |
There was a problem hiding this comment.
Bit of a high-level question, how are you envisioning users will keep track of their obmd admin tokens? My first thought is that the admin token should be added to the CLI command automatically somehow since it's long and would require a lot of copy and pasting if you need to type a lot of commands in a hurry.
There was a problem hiding this comment.
Same for the obmd URI actually, I wonder if that could be inserted from a config file. Or perhaps we leave that to the user?
There was a problem hiding this comment.
The combination is probably comparable to the current one, no? between obmtype, hostname, username, and password, node_register already requires a fair amount of typing. Remember obmd_uri & admin_token are eventually going to replace those, so I think it will end up being about the same.
There was a problem hiding this comment.
Ah right, I forgot that the other options are going away.
|
@xuhang57, yeah, obmd will be a hard dependency as of 0.4. This is why we made a point of getting another release out (0.3) before then, with the necessary stuff to upgrade. 0.3 has already been tagged and released, so I'm not sure exactly what you're suggesting? |
|
@zenhack I was thinking of creating a new branch based on 0.3, but it seems unnecessary since we have tagged/released 0.3 version? The only advantage that I could think of about having a 0.3 branch is that someone who does not nee obmd can use HIL by switching the branch. Not sure whether it makes sense or not. |
|
Quoting Lucas H. Xu (2018-04-16 21:27:27)
The only advantage that I could think of about having a 0.3 branch is
that someone who does not nee obmd can use HIL by switching the branch.
Not sure whether it makes sense or not.
You can check out tags too. I don't think there's any need for a branch.
|
|
Not a big deal enough for me to take the check away, but there might be some documentation that'll need to change, like at the bottom of testing.md for example. |
|
I pushed another commit to fix most of the pycodestyle errors. Per the commit message I left a few of them out, which are in regexes for the drivers -- if not done carefully we could introduce breakage, so I think it's worth deferring to a separate pr. In general we should be using raw strings for regexes, so we don't have to think about two levels of escape sequences. |
|
I don't see the regexes in the failures? I think we are just concatenating strings which happen to contain a backslash. |
|
Switching to so-called raw strings should fix it. The error message for W605 should be clearer, but it means to say that because "\(" is not an actual escape sequence, the backslash itself needs to be escaped (otherwise you are relying on an implementation detail that "invalid" escape sequences aren't interpolated, rather than cause an error) |
zenhack
force-pushed
the
obmd-api-changes
branch
from
55750e7
to
98d84f5
Compare
Both in the database and in node_register. They are still not actually used. Most of the work in this patch is updating the tests, mostly boring adding of obmd parameters everywhere. There are a few things worth calling out: * TestCorrectRegisterObm in tests/unit/apy/main.py now checks a different condition: it verifies the correctness of the obmd fields, rather than the obm driver fields. * Release notes have been added for 0.4. Not everything described there is present in the code yet, but it reflects our final destination. * an environment variable is used in the test suite to work around the need to manually migrate obm info; it is referenced in the migration script and set in .travis.yml * The parameters must also now be specified in site-layout.json
oops.
* Bad indentation (pycodestyle) * Missing import.
Folks need to have run this before upgrading to this version of the code, since otherwise they'd get errors about NULL values. Accordingly the test was failing. We'll end up ripping out the script as well at some point, but let's keep the PR as small as we can.
I'll probably end up fishing all of this out of the history when I start doing some more things, but for right now there aren't any actual tests, and trying to call these is making CI fail.
Missed this before.
This fixes a few missing changes to the Node creation code. Additionally, this factors out the test-specific logic into its own migration script. We're having a problem where the changes we're making to the obmd fields are not being seen by the script that marks those fields as NOT NULL. This was done in an attempt to fix that, but it didn't work. It still seems cleaner to me though, so I'm leaving it.
This seems to get the next migration script to see the changes. We're now getting an error later in an INSERT, when trying to build from a fresh database.
This is built on top of #990, which should be merged first. Only the most recent commit is specific to this PR; see the commit message there for details.
We should wait until 0.3 has been tagged to merge this, since it involves changes that can't go into effect until after users have had the change to run our migration helper script.
We should also run the deployment tests for this one. I made an attempt at updating the deployment tests with the relevant changes (site-layout.json now needs to include the obmd info as well), but this needs testing.
Also, I noticed while I was editing the example site-layout.json that it's format seems to differ from what the code wants; it has a top-level "ipmi" field, but the code references an "obm" field. I have a hunch that this is bitrotten -- @naved001, can you compare against the site-layout.json that we've been using?