Make ESX Ops idempotent #1084
Make ESX Ops idempotent #1084
Conversation
|
I guess few tests needs to be fixed as well. |
| # Return success since disk is anyway not attached | ||
| logging.warning("*** Detach disk={0} not found. VM={1}".format( | ||
| vmdk_path, vm.config.uuid)) | ||
| return None |
There was a problem hiding this comment.
this is where some tests failed. Please add affected tests as part of this PR.
There was a problem hiding this comment.
Fixed in latest commit.
There was a problem hiding this comment.
Test is also failing that the volume is in use when remove is issued without a detach. That path is not addressable as we can;t auto detach the volume. Or can we?
There was a problem hiding this comment.
Failing tests are fixed. Auto-detach is outside scope of this change.
| return err("File %s already exists" % vmdk_path) | ||
| # We are mostly here due to race or Plugin VMCI retry #1076 | ||
| logging.warning("File %s already exists", vmdk_path) | ||
| return None |
There was a problem hiding this comment.
Lets at least check that the volume opts in the KV match whats provided. Two create requests with say different VM sizes should be caught, unless the two volumes would be having the exact same properties.
There was a problem hiding this comment.
Good point. Actually if they are different we may want to reject the request. It's not really idempotent if requests are different :-)
There was a problem hiding this comment.
I think we should go ahead and merge (after fixing the tests) and open an issue to fix create with diff.params - it is super rear now (actually, never)
There was a problem hiding this comment.
Filed a new issue #1089. Yes, it's a rare case where Volume Create is requested from 2 separate nodes at the same time. Addressing it would require some code cleanup since some default options are set in KV (diskformat, fstype) but some are not set (e.g. size).
@govint Are you okay addressing this in separate issue?
There was a problem hiding this comment.
No problem if its going to take more changes anyway.
| @@ -1318,10 +1325,11 @@ def disk_detach(vmdk_path, vm): | |||
| if not device: | |||
| # Could happen if the disk attached to a different VM - attach fails | |||
| # and docker will insist to sending "unmount/detach" which also fails. | |||
There was a problem hiding this comment.
Not this change, Docker doesn't send unmount if mount fails anymore. But this service doesn't haven't to assume such behavior. Possibly remove all mention docker in the service code.
| vmdk_path, vm.config.uuid) | ||
| logging.warning(msg) | ||
| return err(msg) | ||
| # Or Plugin retrying operation due to socket errors #1076 |
There was a problem hiding this comment.
Just as a coding convention can skip mentioning PR number in code.
There was a problem hiding this comment.
we actually do refer to github issues and PRs in different places, so while it may be good or bad, it is certainly consistent (there are 30 cases like that in 17 files in our repo, including ./vendor). BTW , what/where is this coding convention asking to skip PR numbers ?
There was a problem hiding this comment.
I added PR# to give future reader some context why this special cases can occur. Alternatively, I can update function header comment without PR.
| @@ -245,9 +245,9 @@ def testPolicy(self): | |||
| volume_kv.DISK_ALLOCATION_FORMAT: unit[3]}) | |||
| self.assertEqual(err == None, unit[2], err) | |||
|
|
|||
| # clean up should fail if the created should have failed. | |||
| # clean up would succeed with #1084. | |||
There was a problem hiding this comment.
As mentioned PR/issue numbers can be skipped in comments.
Make ESX Ops idempotent to tolerate plugin retries in case of VMCI communication errors.
Testing Done: Changed Plugin code to retry every operation by injecting failure on 1st attempt. And then tried docker volume create/remove and docker run.
Fixes #1076