Bug "Copy Image Not Working #146" resolved. #147

nasibehteimouri · 2017-09-27T20:57:12Z

__get_ceph_image_name --> replaced with get_ceph_image_name_from_project
images is flattened

naved001 · 2017-09-27T21:11:35Z

slightly off-topic but, @chemistry-sourabh can we run travis in a way so that the build doen't stop right after pep8 failures?

jeremyfreudberg · 2017-09-27T21:27:15Z

@naved001 that is how before_script works in Travis. Anything in that section is assumed to be "setup" code that the script section relies on, so Travis thinks it is being smart by skipping everything else if setup fails

This is vaguely hinted at here: https://docs.travis-ci.com/user/customizing-the-build#Breaking-the-Build

naved001 · 2017-09-28T12:24:48Z

@nasibehteimouri

You could run pep8 locally on your machine.
pip install pep8 and then `pep8 <filename.py>'

Also, if you look at the Travis logs, you'll notice that the pep8 error is saying that the line is too long (over 79 characters) in certain places. So if you format it right and break it into multiple lines it should be fine.

Other than that, you could setup your text editor to remove white spaces, warn you when you go over the character limit per line, and also convert your tabs to spaces. Search for "your-text-editor for python development"

nasibehteimouri · 2017-09-28T15:38:25Z

pep8 does not complain in my local machine, but still complaining here.

naved001 · 2017-09-28T15:45:15Z

hmm, that's weird. running it like how the ci runs it find . -name \*.py -exec pep8 --ignore=E402 {} + should point out the errors.

naved001 · 2017-10-02T15:15:34Z

tests/integration/einstein/test_operations.py

@@ -29,6 +29,7 @@
 NEW_SNAP_NAME = _cfg.tests.new_snap_name
 NOT_EXIST_IMG_NAME = _cfg.tests.not_exist_img_name
 NOT_EXIST_SNAP_NAME = _cfg.tests.not_exist_snap_name
+ING2 = _cfg.tests.IMAGE2_NAME_PARAMETER


should be IMG2, and also you would need to put this in the config file if you are doing _cfg.tests.something.

But I think for the purpose of this test, you can get rid of this constant from here, and just generate a random string in the setup of your test. Something like this would work

import uuid img2 = uuid.uuid4()

nasibehteimouri · 2017-10-02T23:27:50Z

@naved001 @chemistry-sourabh Can somebody please take a look at the TestCopyImage and let me know what I missed in SETUP or TearDown?

I think RunTest is correct. Manually (using the command bmi cp srcPrj img1 desPrj img2), I can make a copy of img1 and use that to provision a node.

Appreciate it in advance.

naved001 · 2017-10-03T20:19:36Z

tests/integration/einstein/test_operations.py

+        time.sleep(constants.HIL_CALL_TIMEOUT)
+
+    def tearDown(self):
+        self.good_bmi.deprovision(NODE_NAME, NETWORK, NIC)


no need to deprovision because you didn't provision in the setup()

naved001 · 2017-10-03T20:22:21Z

tests/integration/einstein/test_operations.py

+        self.good_bmi = BMI(CORRECT_HIL_USERNAME, CORRECT_HIL_PASSWORD,
+                            PROJECT)
+        self.good_bmi.import_ceph_image(EXIST_IMG_NAME)
+        img2 = uuid.uuid4()


this should be self.img2 = uuid.uuid4(), then it will be available in runTest and tearDown. And to call it you need to write self.img2

@chemistry-sourabh suggests that you should also do a check in ceph to make sure that the copied image is present.

naved001 · 2017-10-03T20:25:43Z

tests/integration/einstein/test_operations.py

+            if img2 == image:
+                exists_image = True
+        self.assertTrue(exists_image)
+        time.sleep(constants.HIL_CALL_TIMEOUT)


you can also remove this because we aren't provisioning.

Provisioning and deprovisioning make calls to hil that can take some time, which is why we have that sleep call.

naved001 · 2017-10-03T20:31:04Z

tests/integration/einstein/test_operations.py

+        self.db.project.delete_with_name(PROJECT)
+        self.db.close()
+        self.good_bmi.shutdown()
+        time.sleep(constants.HIL_CALL_TIMEOUT)


time.sleep not required.

@naved001 @chemistry-sourabh
Thanks,
I applied your comments, except checking the ceph to see if the image exists. I'll do it soon and let you know the result.

naved001 · 2017-10-03T20:31:56Z

Hopefully the comments we made should fix the test!

nasibehteimouri · 2017-10-15T03:52:06Z

@chemistry-sourabh @apoorvemohan
As far as I see from the log, it seems that there is a problem with db.image.copy_image, or the way that it is called in copy_image (in operations.py). It seems that the EXIST_IMG is not found in db.

P.S) please ignore the pep8 error.

Can somebody please take a look, thanks!

chemistry-sourabh · 2017-10-16T16:49:09Z

@nasibehteimouri you need to fix the pep8 so that the tests run in Travis. Please fix the pep8 so that we can look into this.

chemistry-sourabh

Small Nits

Could you please add a docstring to the copy_image function also.

Also don't forget to squash the commits

chemistry-sourabh · 2017-10-23T05:12:11Z

tests/integration/einstein/test_operations.py

@@ -2,6 +2,7 @@

 import time
 import unittest
+import uuid


I guess you can remove the import

chemistry-sourabh · 2017-10-23T05:12:43Z

tests/integration/einstein/test_operations.py

+        self.good_bmi = BMI(CORRECT_HIL_USERNAME, CORRECT_HIL_PASSWORD,
+                            PROJECT)
+        self.good_bmi.import_ceph_image(EXIST_IMG_NAME)
+        self.img2 = 'img2'


Can you make this a constant similar to PROJECT, EXIST_IMG_NAME, etc.

not sure about it. I think it must be defined in CLI. Following is the list of variables available in CLI. We should ask them to add a new variable like IMG2 for us.

picasso_url

correct_hil_username

correct_hil_password

incorrect_hil_password

node_name

nic

project

network

exist_img_name

new_snap_name

not_exist_snap_name

not_exist_img_name

chemistry-sourabh · 2017-10-23T05:15:20Z

tests/integration/einstein/test_operations.py

+                      _cfg.iscsi.password) as fs:
+            img_id = self.good_bmi.get_ceph_image_name_from_project(
+                self.img2, PROJECT)
+            fs.get_image(img_id)


I am not 100% sure this will work. What if the image is not found ? As far as I know the test won't fail. You will need to fail the test if an exception is raised.

I will recommend testing it first. Try deleting the image before the get_image and see if the test fails. If it does then no issues.

Tried and failed. So, there is no issue about it.

chemistry-sourabh · 2017-10-29T21:49:22Z

tests/integration/einstein/test_operations.py

@@ -13,6 +13,8 @@
 from ims.database.database import Database
 from ims.einstein.operations import BMI

+IMG2 = 'img2'
+


Could you please move this to the chunk where EXIST_IMG_NAME, etc constants are defined ?

You didnt address this

chemistry-sourabh

You forgot to add the docstring to the copy_image function

chemistry-sourabh

nits

chemistry-sourabh · 2017-10-31T21:12:16Z

ims/einstein/operations.py

@@ -560,17 +560,31 @@ def get_node_ip(self, node_name):

    @log
    def copy_image(self, img1, dest_project, img2=None):
+        """
+        Create a flatten copy of a src image in DB


Create a deep copy of src image

chemistry-sourabh · 2017-10-31T21:12:46Z

ims/einstein/operations.py

+        Create a flatten copy of a src image in DB
+
+        :param img1: Name of src image
+        :param dest_project: where the destination image will be created


name of the project where the destination image will be created

naved001

LGTM. note to whoever merges this, please squash and merge.

naved001 · 2017-11-21T16:51:12Z

@chemistry-sourabh ping

chemistry-sourabh · 2017-11-23T06:10:41Z

ims/einstein/operations.py

            self.fs.snap_image(ceph_name, constants.DEFAULT_SNAPSHOT_NAME)
-            self.fs.snap_protect(ceph_name, constants.DEFAULT_SNAPSHOT_NAME)


@nasibehteimouri I think this line is the issue.

Since the image's snapshot is not protected it is failing as the remove image is not able to unprotect the snapshot

chemistry-sourabh

@nasibehteimouri looks like the previous edit fixed it.

Just do the small nits and squash. Then I'll Approve

chemistry-sourabh · 2017-11-24T22:31:36Z

ims/einstein/operations.py

            self.fs.snap_image(ceph_name, constants.DEFAULT_SNAPSHOT_NAME)
            self.fs.snap_protect(ceph_name, constants.DEFAULT_SNAPSHOT_NAME)
+


Remove this

chemistry-sourabh · 2017-11-24T22:31:49Z

tests/integration/einstein/test_operations.py

@@ -13,6 +13,8 @@
 from ims.database.database import Database
 from ims.einstein.operations import BMI

+IMG2 = 'img2'
+


You didnt address this

apoorvemohan · 2018-02-07T00:18:27Z

@chemistry-sourabh @nasibehteimouri We need this urgently. Could guys work on merging this?

chemistry-sourabh · 2018-02-08T20:06:07Z

@nasibehteimouri can you please address the required changes so that we can merge this for @apoorvemohan ?

Also I think you will need to rebase this

nasibehteimouri · 2018-02-08T21:50:38Z

@chemistry-sourabh you asked for "img2" to be defined like other constant. If so, it is impossible or at least I cannot do this. please check out here (https://github.com/CCI-MOC/ims/blob/master/docs/testing.md). It says that "The current values that are available in our CI are bellow and Let us know if your test cases need additional variables". (IMG2 must be added in CI and I cannot do this).

picasso_url
correct_hil_username
correct_hil_password
incorrect_hil_password
node_name
nic
project
network
exist_img_name
new_snap_name
not_exist_snap_name
not_exist_img_name
"

chemistry-sourabh · 2018-02-09T18:39:22Z

@nasibehteimouri sorry to confuse you. I meant to move IMG2 to the constants section in the same file (in the area below config.get() )

Also I think you screwed up the rebase. You did a merge instead of rebase. So first undo the merge and rebase. @xuhang57 can you please help her with this also.

nasibehteimouri · 2018-02-09T23:08:10Z

@chemistry-sourabh @apoorvemohan thanks for making it more clear. I relocate img2 and now it is defined in the constant part of the file.

sorry for the merge! Did un-do it and rebased.

nasibehteimouri · 2018-02-09T23:22:44Z

@chemistry-sourabh @apoorvemohan

I REALLY do not know why CI fails, all tests pass on my local machine without any failures. I observed the same here and not know why still CI fails!

nothing has changed compared to the last time which successfully finished. Now, I only defined img2 in line 33 instead of 16! (ims/tests/integration/einstein/test_operation.py).

chemistry-sourabh

Everything else LGTM!

Fix CI and you should be good

chemistry-sourabh · 2018-02-10T07:22:45Z

tests/integration/einstein/test_operations.py

@@ -29,7 +30,7 @@
 NEW_SNAP_NAME = _cfg.tests.new_snap_name
 NOT_EXIST_IMG_NAME = _cfg.tests.not_exist_img_name
 NOT_EXIST_SNAP_NAME = _cfg.tests.not_exist_snap_name
-
+IMG2 = 'img2'



There should be one more blank line here that is why CI failed in pep8.

nasibehteimouri · 2018-02-10T16:08:05Z

@chemistry-sourabh Thank. I think it is ready now.

chemistry-sourabh · 2018-02-10T19:12:41Z

@apoorvemohan you can press the magic button whenever you want!

@nasibehteimouri congratulations on your first PR

naved001 · 2018-02-10T19:38:37Z

@chemistry-sourabh github says this branch is out-of-date with the base branch. Should this be rebased before merging? There's also the "Update branch" button

chemistry-sourabh · 2018-02-10T19:39:54Z

Its fine. There is no conflict, so rebase is not required.

naved001 mentioned this pull request Sep 27, 2017

run pep8 along with other scripts rather than before_script #148

Merged

naved001 reviewed Oct 2, 2017

View reviewed changes

naved001 suggested changes Oct 3, 2017

View reviewed changes

chemistry-sourabh suggested changes Oct 23, 2017

View reviewed changes

chemistry-sourabh reviewed Oct 29, 2017

View reviewed changes

chemistry-sourabh suggested changes Oct 29, 2017

View reviewed changes

chemistry-sourabh suggested changes Oct 31, 2017

View reviewed changes

naved001 approved these changes Nov 15, 2017

View reviewed changes

apoorvemohan assigned apoorvemohan and nasibehteimouri and unassigned apoorvemohan Nov 15, 2017

nasibehteimouri force-pushed the master branch from 6de8349 to b44141a Compare November 22, 2017 03:00

chemistry-sourabh reviewed Nov 23, 2017

View reviewed changes

chemistry-sourabh suggested changes Nov 24, 2017

View reviewed changes

nasibehteimouri force-pushed the master branch from 34ffd20 to 42cc2b7 Compare February 9, 2018 23:01

chemistry-sourabh approved these changes Feb 10, 2018

View reviewed changes

Bug "Copy Image Not Working CCI-MOC#146" resolved.

8839eb7

nasibehteimouri force-pushed the master branch from 1e7e911 to 8839eb7 Compare February 10, 2018 15:56

apoorvemohan merged commit 8a334df into CCI-MOC:master Feb 10, 2018

apoorvemohan mentioned this pull request Feb 10, 2018

Revert "Bug "Copy Image Not Working #146" resolved." #160

Closed

		self.fs.snap_image(ceph_name, constants.DEFAULT_SNAPSHOT_NAME)
		self.fs.snap_protect(ceph_name, constants.DEFAULT_SNAPSHOT_NAME)

Bug "Copy Image Not Working #146" resolved. #147

Bug "Copy Image Not Working #146" resolved. #147

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment