BMI on Travis for CI #119
BMI on Travis for CI #119
Conversation
chemistry-sourabh
changed the title
|
Thanks @chemistry-sourabh, this is really awesome. Except for end of file new lines, I don't see much problems with this. But I would recommend this to be tried by @naved001 before we push forward. If not, I can give this a try later this week. |
|
Try what ? the travis check passing is proof that it works! @ravisantoshgudimetla |
Where is the travis run? |
|
It runs on Travis ? Check the build log for this Travis build, you will understand better |
|
@ravisantoshgudimetla Here are the travis logs. @chemistry-sourabh it's really awesome. |
|
Changed to Docker Hub as per long discussion with @ravisantoshgudimetla |
|
Fixed new lines also. So please approve if no other issues then we can merge this ASAP |
|
Forgot to add coverage and random execution of tests. Will update this PR later |
.travis.yml
Outdated
| - sudo docker run -d -v ceph:/etc/ceph -e MON_IP=172.17.0.2 -e CEPH_PUBLIC_NETWORK=172.17.0.0/24 --name ceph ceph/demo | ||
| - sleep 30 | ||
| - sudo docker run -d --name hil chemistrysourabh/hil | ||
| - sudo docker pull chemistrysourabh/bmi-ci |
There was a problem hiding this comment.
can we have generic names here?
There was a problem hiding this comment.
you mean the repo names ? Ya that is part of my agenda. I didnt knew docker would use my Docker ID as my repo name. Will change it.
|
@naved001 and @sirushtim added randomness plugin, changed repo to bmis instead of mine, Added comments to yaml also. Waiting on approval for coverage |
|
@CCI-MOC/bmi-breakers can I get a second +1, so that we can merge. I'll raise a second PR for coverage. |
|
Hey @knikolla would you have the time to review this PR? |
| @@ -0,0 +1,33 @@ | |||
| FROM ubuntu | |||
There was a problem hiding this comment.
@knikolla This is the file is used to build the docker image that is pulled in the yaml file. I'll add a comment on top saying the same.
.travis.yml
Outdated
| script: | ||
| # Run pep8 on all .py files in all subfolders | ||
| # ignore E402: module level import not at top of file | ||
| - find . -name \*.py -exec pep8 --ignore=E402 {} + | ||
|
|
||
| # Run Ceph Container and Wait for some time so that ceph runs | ||
| - sudo docker run -d -v ceph:/etc/ceph -e MON_IP=172.17.0.2 -e CEPH_PUBLIC_NETWORK=172.17.0.0/24 --name ceph ceph/demo |
There was a problem hiding this comment.
Please use the correct steps as defined here. https://docs.travis-ci.com/user/customizing-the-build#The-Build-Lifecycle
There was a problem hiding this comment.
I'll have a look
There was a problem hiding this comment.
@knikolla as per the build cycle, the pep8 check should go in the script section and running containers should go in before_install section. But it doesnt make sense to run the containers if pep8 failed right ? what do you think ?
|
Changes Unknown when pulling 78ce74c on chemistry-sourabh:Travis into ** on CCI-MOC:dev**. |
|
@chemistry-sourabh please refrain from adding changes once approval has been given unless they are related to change requested or else whoever has approved needs to come back and review it again. |
|
@ravisantoshgudimetla sorry about that. I am nearly done with this, will let you know when its ready. |
chemistry-sourabh
force-pushed
the
Travis
branch
4 times, most recently
from
ac081ee to
36c1441
Compare
|
@knikolla and @ravisantoshgudimetla please have a look. Added coverage also into this PR. |
| @@ -1,3 +1,5 @@ | |||
| [](https://travis-ci.org/CCI-MOC/ims) | |||
| [](https://coveralls.io/github/CCI-MOC/ims?branch=dev) | |||
There was a problem hiding this comment.
I guess, you need to separate the coverage part out as it is not part of BMI CI. You may be running CI to get coverage report but that wasn't explicitly mentioned in issue. So the steps would be to
- Update the existing issue to reflect this.
- Create 2 commits - One for BMI CI , one for coverage part.
|
Overall I am fine with the changes, I will leave it to @knikolla to provide his feedback. |
|
@naved001 @pgrosu changed ceph version and also changed OS to centos. The issue is with HIL, the current production is so old that the Dockerfile I wrote for it doesn't work. Also the docs for it are also not that detailed. @naved001 can you tell me the commit to which you will be upgrading production HIL to ? I'll change the docker script to install that instead. |
|
@chemistry-sourabh The current HIL master is what I'll use to upgrade. So it'll be this commit |
|
@knikolla are your concerns addressed? |
|
@chemistry-sourabh Thank you the fixes! I'm assuming the a177944 and 2a588f7 commits will pass with the HIL version that @naved001 recommended? If so, then let's match to what @naved001 is using as his commit for HIL. Over time I prefer we shift to a continuous integration, where the HIL instance to test against is the latest stable release - which will also be the deployed version in @naved001 This HIL commit will be the one installed in new |
|
Updated HIL also and it works!! |
Squashed commits: [639b713] Fixed tgt [57c9d09] test [2a588f7] Changed to centos [a177944] Added ceph tag [8b29689] CI Final Commit (+1 squashed commit) Squashed commits: [50097a9] Commented docker files (+1 squashed commit) Squashed commits: [e4c0f50] Added comments (+6 squashed commits) Squashed commits: [14e426c] Update images [876b6bd] Repo change [2ae8ab0] Added new lines [314830d] Changed to docker hub [51c5d61] Added build status [2faa0b0] Dockerfile for environment (+1 squashed commit) Squashed commits: [c338292] Travis
chemistry-sourabh
force-pushed
the
Travis
branch
from
22c7623 to
e673308
Compare
|
@chemistry-sourabh This is really great news and thank you for making this happen! I still need to review it a little bit, but had not had the chance today - I promise very soon. Many thanks! |
|
@ravisantoshgudimetla, ping |
|
@naved001 One thing to give you a heads up on, this PR contains hammer as the Ceph client and @djfinn14 and I are going to be testing a scenario soon, which might require this PR to be changed by using jewel as the Ceph client. This is not a scenario that would be easy to figure out if one assumes that Travis is working properly, when it might not. |
|
@pgrosu can you describe this scenario a bit more ? I tested BMI with Jewel and with worked fine. |
yeah, in that case we'll raise a PR to modify the changes introduced by this. |
|
@chemistry-sourabh, @naved001 Jewel would probably work but we need to still test on PRB in the new dev environment, but this PR is using Hammer. When we We did not want this merged yet as a separate on a passed PR could confuse people where the error is coming from, as Travis would also be used to confirm future submitted PRs. Such layered troubleshooting might or might not pass, causing confusion where the issue is coming from. Basically we need all merged PRs to ensure they pass all tests, of which ours is the final one. So it would be best to wait until we have completed each one properly. |
|
@pgrosu interesting! The weird question is why isn't it happening in Travis ? |
|
@chemistry-sourabh Because I don't believe we are running that control flow scenario of creating an image first, and then immediately importing it. @djfinn14 started going into the Ceph codebase, but before we drill too deep we wanted to confirm by replicating the scenario. If that is confirmed, then @apoorvemohan's minimum requirements list (#120 (comment)) would need to be updated as well. |
|
@pgrosu I understand what you are saying. My reason for going ahead and merging this was because this was sitting for too long and would start to rot just like how the install script is right now ( #60 ). We will continue to discover new bugs and patch them as we discover them instead of doing it in open PRs. And as for the ceph version (hammer vs jewel) we could open up new issue saying that our CI might need to be updated in the future; that way we can keep track of the issues we might have. If you'd prefer that we unmerge this PR, that's fine by me. You can merge this after you and @djfinn14 figure out the ceph issues. |
|
I'll prefer updating it with a future PR. Since we are not experiencing this issue in Travis, this future PR is not time critical. |
|
I'd recommend cutting a release before making the changes again. It should be something like: BMI 0.5 Version supports - TGT XX Version, Ceph YY Version etc. If not, it becomes difficult to manage. |
|
Though I understand, an unknown-known can still exist and we are beginning to shift as a group to a systematic analysis approach when testing or coding BMI, which includes PRs. That means analyzing all common scenario combinations, and how BMI handles them. Once the product is released we want to ensure surprises are minimized downstream, as we will need to focus on the redesign. |
|
@pgrosu so in conclusion, what do we want to do with this PR? |
|
@naved001 In conclusion we will run the acceptance test and report back the status, in case we require the change to jewel. So let's keep it as is but in the future, please ask before merging. |
Sure. The PR had 2 +1s which is why I merged it. In future, for any other PR that you don't want merged, just hit the Request Changes button with a comment. |
|
Better would be to add the DO NOT MERGE label |
Refer #117
Sets up 3 docker container instead of one as I was having issues with the former. Works perfectly well with all the test cases I have except the picasso test cases which need to be modified to start and stop picasso and einstein as per @naved001's suggestion. Also works with the state operation test cases in #111