)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"e05ced5beceb97ac0bc7900cd0b7d2f3048d2c9b","unresolved":false,"context_lines":[{"line_number":14,"context_line":"filter the host. But oversubscription ratio is computed using the"},{"line_number":15,"context_line":"volume size and if it\u0027s less than max oversubscription ratio, the"},{"line_number":16,"context_line":"host is an eligible candidate and there is no need to compare"},{"line_number":17,"context_line":"virtual free capacity with volume size."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Change-Id: Ifaff40dc87ec1e041c00614d78191a393eed3125"},{"line_number":20,"context_line":"Closes-Bug: #1462606"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3afb71cf_331e0064","line":17,"updated":"2015-06-08 12:09:52.000000000","message":"I think you can add what your patch does, since all the above part is just trying explaining why.","commit_id":"399f90157ae52e6d52671dc358a42edb82cb6736"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"5c3189111c61dc5a6f722827c71fa1f7b13e6b19","unresolved":false,"context_lines":[{"line_number":14,"context_line":"filter the host. But oversubscription ratio is computed using the"},{"line_number":15,"context_line":"volume size and if it\u0027s less than max oversubscription ratio, the"},{"line_number":16,"context_line":"host is an eligible candidate and there is no need to compare"},{"line_number":17,"context_line":"virtual free capacity with volume size."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Change-Id: Ifaff40dc87ec1e041c00614d78191a393eed3125"},{"line_number":20,"context_line":"Closes-Bug: #1462606"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3afb71cf_f3e17e9b","line":17,"in_reply_to":"3afb71cf_331e0064","updated":"2015-06-08 15:41:20.000000000","message":"Can you add a line to summarize your code change?","commit_id":"399f90157ae52e6d52671dc358a42edb82cb6736"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"df739932678c5ff470a76880472d41cfa68201c0","unresolved":false,"context_lines":[{"line_number":14,"context_line":"filter the host. But oversubscription ratio is computed using the"},{"line_number":15,"context_line":"volume size and if it\u0027s less than max oversubscription ratio, the"},{"line_number":16,"context_line":"host is an eligible candidate and there is no need to compare"},{"line_number":17,"context_line":"virtual free capacity with volume size."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"Change-Id: Ifaff40dc87ec1e041c00614d78191a393eed3125"},{"line_number":20,"context_line":"Closes-Bug: #1462606"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3afb71cf_77ffb367","line":17,"in_reply_to":"3afb71cf_f3e17e9b","updated":"2015-06-08 18:02:39.000000000","message":"Done.\n\nAlso added Xing\u0027s reply related to free virtual formula correctness.","commit_id":"399f90157ae52e6d52671dc358a42edb82cb6736"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"f6d85bd71c0918cc42314c08690a57efe17d1216","unresolved":false,"context_lines":[{"line_number":30,"context_line":"decide the eligibility of a host if thin provisioning is enabled"},{"line_number":31,"context_line":"and provisioning ratio is less than or equal to max oversubscription"},{"line_number":32,"context_line":"ratio."},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"Change-Id: Ifaff40dc87ec1e041c00614d78191a393eed3125"},{"line_number":35,"context_line":"Closes-Bug: #1462606"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"ba3cc151_1678c8c3","line":33,"updated":"2015-06-29 19:06:39.000000000","message":"Shouldn\u0027t there be a DocImpact since we are changing the way the filter works?\n\nBefore this patch the filter would enforce reserved percentage both in real free space as well as in virtual free space, but with this patch it will only enforce the first.","commit_id":"eb5101e4903ad26cb4e939e693868af0194b943e"}],"cinder/scheduler/filters/capacity_filter.py":[{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"e05ced5beceb97ac0bc7900cd0b7d2f3048d2c9b","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":1,"id":"3afb71cf_53229c02","line":104,"updated":"2015-06-08 12:09:52.000000000","message":"If total\u003d10G, host_state.provisioned_capacity_gb\u003d8G, host_state.max_over_subscription_ratio\u003d1.5, free\u003d2G, volume_size\u003d5G. It will pass line 92 but fail line 104. Could you explain more clearly why line 104 is not needed?","commit_id":"399f90157ae52e6d52671dc358a42edb82cb6736"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"5c3189111c61dc5a6f722827c71fa1f7b13e6b19","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":1,"id":"3afb71cf_d0ef9885","line":104,"in_reply_to":"3afb71cf_53229c02","updated":"2015-06-08 15:41:20.000000000","message":"max_over_subscription_ratio should be measured against total capacity rather than free capacity. The formula on line 104 on the left is not correct. The correct formula should be: \"free_virtual\u003dtotal*max_over_subscription_ratio-provisioned_capacity-volume_size\"\n\nand then \"return free_virtual \u003e\u003d 0\".\n\nWe should not be deducting volume_size from free_virtual for thin  provisioning that supports oversubscription.\n\nWhen we are in the else block on line 103, free_virtual is always \u003e\u003d 0. So it should just return \"True\".","commit_id":"399f90157ae52e6d52671dc358a42edb82cb6736"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"df739932678c5ff470a76880472d41cfa68201c0","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":1,"id":"3afb71cf_b7640b22","line":104,"in_reply_to":"3afb71cf_d0ef9885","updated":"2015-06-08 18:02:39.000000000","message":"I used current free virtual in the updated commit message.","commit_id":"399f90157ae52e6d52671dc358a42edb82cb6736"},{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"602d61c5b0be068ced35f20f5f8936de9862179e","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afb71cf_c3d9f023","line":104,"updated":"2015-06-09 09:25:11.000000000","message":"If we delete the test on `free` space here. The `reserved` ratio won\u0027t be taken into consideration before return True/False.","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"5ceb7c1d69b58deca43b97a7999d9cc842557d93","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afb71cf_7728ba86","line":104,"updated":"2015-06-09 07:22:54.000000000","message":"Sorry maybe this problem is not exactly related to this bug, I would like to discuss it here.\nShould reserved capacity(reserved_percentage) is considered when counting provisioned_ratio?","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"70b553eff60fd9e459859b78ed8b68d81e81d150","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa1b9901_e07336b3","line":104,"in_reply_to":"1a4dcd0f_9d56b25c","updated":"2015-08-17 11:04:16.000000000","message":"Winston, Any comments? Based on this discussion, it looks like we need to fix capacity weigher. Do you agree?","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"15a595cba7671c4e1848d89fcb347bedd1ffa636","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_beaa126f","line":104,"in_reply_to":"3afb71cf_0a02ff0b","updated":"2015-06-15 11:14:58.000000000","message":"\u003e So it is safer to keep the original formula. We could just change the variable name from \"free_virtual\" to \"adjusted_free_virtual\" and add some comments (to avoid confusion hopefully), but still keep the original calculations on the left hand side.\nThoughts?\n\nBut do we end up with the same problem addressed in https://review.openstack.org/#/c/188031/","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":2759,"name":"Huang Zhiteng","email":"winston.d@gmail.com","username":"zhiteng-huang"},"change_message_id":"eaa1237cdf6c8ebcd7525706da7ad0be61640f57","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_0081b78e","line":104,"in_reply_to":"3afb71cf_0a02ff0b","updated":"2015-06-18 09:35:29.000000000","message":"I like the example Xing had given here.  Let us verify current version and so far agreed change with slightly modified example:  total\u003d100G, provisioned \u003d 100G, used \u003d 94 G, free \u003d 6G, reserved \u003d 0.05, max_overratio \u003d 2, volume_size \u003d 90G\n\nprovisioned ratio \u003d (100+90)/100 \u003d 1.9 \u003c 2 (max prov ratio)\n\ncurrent version: free_virtual \u003d 6*2 \u003c 12, return False\n\nso far agreed version: adjusted_free \u003d 6-(100*0.05) \u003d 1 \u003e 0, return True\n\nSo the question becomes should we allow a new 90G volume be created in the backend with only 1 GB actual useable physical capacity?  My personal vote would be \u0027No\u0027.  And I\u0027d like to hear feedback before we move on.","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"23a3f25a8f536e88d6e60ed8c66bdf89c5553ba2","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afb71cf_7564a7e4","line":104,"in_reply_to":"3afb71cf_0a02ff0b","updated":"2015-06-11 18:32:14.000000000","message":"Thanks for the clarification Xing!\n\nThat does make sense, but I do have some concerns that the reserved space would now be ignored if thin provisioning is enabled. It seems like we still want to check somewhere along the line if \"free_space - math.floor(total * reserved)\" aka \u0027free\u0027 ever reached zero or went negative, right? In that case we would not want to schedule on this backend, even if the ratio is still less than the max allowed.","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"89622c4620a30da5924e50fcf35a5d66819a41a0","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afb71cf_0a02ff0b","line":104,"in_reply_to":"3afb71cf_56631542","updated":"2015-06-11 17:53:51.000000000","message":"You can never \"fix this once and for all\":).  Let\u0027s NOT go back to the endless spec review route!\n\nLet\u0027s think about this carefully.\n\nFirst of all, there\u0027s some confusion regarding the formula on line 104 on the left hand side:\nfree_virtual \u003d free * host_state.max_over_subscription_ratio\n\nThis way of calculating free virtual capacity is not right. The correct formula should be:\nfree_virtual \u003d total*max_over_subscription_ratio - provisioned_capacity\n\nIf we change the code to:\n\nfree_virtual \u003d total*host_state.max_over_subscription_ratio\n\nreturn free_virtual\u003e\u003dvolume_size\n\nfree_virtual is always greater than or equal to the volume_size here in the else block.  So it will always return True.  So \"return free_virtual\u003e\u003dvolume_size\" is equivalent to \"return True\" here.  If you could come up with numbers that show this could return False, please give an example.\n\nWith the above change, thin provisioning with max_over_subscription_ratio\u003e1 will be completely controlled by the over subscription ratio.  Free physical capacity will not be part of the evaluation at all.  That is the reason to change it to \"return True\" here.\n\nSo what is the changed in behavior with this patch?\n\nI think the original code on line 104 on the left is actually looking at the adjusted free capacity. So it does look at the physical free space but also take into account how much oversubscription can be done. It puts the cap of free physical capacity there but adjusted it for thin.\n\nSee this example: \ntotal\u003d10G, provisioned\u003d15G, used\u003d8G, free\u003d2G. Assume reserved\u003d0.\nmax_over_subscription_ratio \u003d 2.\nvolume_size \u003d 5\n\n(provisioned + volume_size)/total \u003d (15+5)/10 \u003d 2\nIt goes to else block:\nfree_virtual\u003d2*2\u003d4\n4\u003c5, so it will return False.\nHowever with this patch, it will return True.\n\nSo it is safer to keep the original formula. \nWe could just change the variable name from \"free_virtual\" to \"adjusted_free_virtual\" and add some comments (to avoid confusion hopefully), but still keep the original calculations on the left hand side.\n\nThoughts?","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"42ac06620eb8cc732ef79631a333c4b4d6026f99","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afb71cf_9b61b8d5","line":104,"in_reply_to":"3afb71cf_676665bc","updated":"2015-06-11 03:20:51.000000000","message":"That part of the spec is no longer accurate because of this bug fix:\nhttps://review.openstack.org/#/c/185764/\n\nBecause of the above bug fix, we no longer use physical capacity to evaluate a backend that supports thin provisioning with max_over_subscription_ratio \u003e 1.  Reserved capacity is physical capacity.  free_capacity is also physical capacity.  Reserved and free capacity are used for thick provisioning only. That\u0027s why we should not use them for thin provisioning with max_over_subscription_ratio \u003e1 any more.\n\nFor thin provisioning with max_over_subscription_ratio \u003e 1, we should only use over_subscription_ratio to evaluate.","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"b67f46e265fc02f02cb657a116e79b8309dc0fcc","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afb71cf_f6b2de05","line":104,"in_reply_to":"3afb71cf_7564a7e4","updated":"2015-06-11 19:17:20.000000000","message":"I think that makes sense too. So we could change it to:\n\nreturn free\u003e0\n\n\"free\" already takes into account \"reserved\".  So this will still preserve what was proposed in the original spec.","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"8baf25d84891c6a05ac26a30073adbf6aaff56e0","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afb71cf_3298a21b","line":104,"in_reply_to":"3afb71cf_9b61b8d5","updated":"2015-06-11 07:10:58.000000000","message":"Based on your comments, I agree with the update.","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"7c119c8112f2c1c485eb4fdb9206fc44035ac4a3","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afb71cf_56631542","line":104,"in_reply_to":"3afb71cf_9b61b8d5","updated":"2015-06-11 05:08:16.000000000","message":"I\u0027m not sure I am understanding the point about not using physical capacity at all to evaluate a backend. We should be, to an extent... its just that we modify the amount of virtual_free space based on the oversubscription ratio, right?\n\nI suppose the question is can we end up in a state where free * max_over_subscription_ratio is actually less than volume size AND the provisoioned_ratio is less than max_over_subscription_ratio? Do we care?\n\nEither way we have the ability to:\n- Create a volume with space larger than what is free on the array\n- Create volumes that have a combined size larger than free space on the array \n\nBoth of those features being under the assumption that volumes would be under utilized and/or additional capacity can be added later. So in that regard I\u0027m pretty happy with either approach... but I do lean towards leaving the formula in there. And I say that with more of the \u0027if it ain\u0027t broke don\u0027t fix it\u0027 mindset.\n\nOne thing I\u0027m not clear on is what exactly does this change fix? Is there actually a bug here? This is a behavior change and not just removing some \u0027unnecessary\u0027 or dead code (unless I\u0027m totally missing something).... is this just another round of folks trying to grok the oversubscription spec/code and not agreeing with how it works?\n\nI think if its the later case, we should maybe put up a patch to revise the spec first and make sure we really nail down the details of what behavior we want and fix this once and for all. It kind of seems like we all have a little bit different view on how it should work... I don\u0027t know if there is a good way to solve that, but maybe its worth a little bit of time to consider at least.","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"988b03cd0c03ac2a1d75f7bc20e467f5a042f144","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afb71cf_cac66e99","line":104,"in_reply_to":"3afb71cf_c3d9f023","updated":"2015-06-10 07:28:05.000000000","message":"Please see the comment from Xing:\n\n\u003etotal*reserved should not be deducted from free_virtual because reserved is for the physical capacity\n\n\nAnother way to look at it is, we have over-subscription for thin provisioning and we haven\u0027t reached the max limit (over-subscription ratio \u003c\u003d max over-subscription ratio), so it is OK to provision more.","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"3f88c2e295139945c9b911053baa330600691b5a","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afb71cf_676665bc","line":104,"in_reply_to":"3afb71cf_cac66e99","updated":"2015-06-11 01:09:02.000000000","message":"Copy the link here: \nhttps://github.com/openstack/cinder-specs/blob/master/specs/kilo/over-subscription-in-thin-provisioning.rst\n\nMy understanding is that reserved capacity is used to protect from running out of read capacity. \nSo when free_capacity  is less than reserved capacity, it should not allow to create volumes.\n\nPlease refer the two cases:\nCase 1. \nTotal_space(total real capacity): 100G\nreserved_percentage: 50\nprovisioned_capacity(virtual capacity): 80G\nmax_over_provision: 2\nfree_capacity: 49G  (already allocated real capacity 51G )\n\nIn this case, from the updated code, a new volume (50G ) can be allocated. \nBut it should not.\n\nCase 2.\nTotal_space(total real capacity): 100G\nreserved_percentage: 50\nprovisioned_capacity(virtual capacity): 80G\nmax_over_provision: 2\nfree_capacity: 60G  (already allocated real capacity 40G )\n\nIn this case, if to create a volume whose virtual capacity is 50G, it should allow.\n\nMy opinion: \n1. if free_capacity is less than reserved capacity, no volume can be created.\n2. if free_capacity is not less than reserved capacity, just consider provision_rate \u003c max_over_provision. \nprovision_rate \u003d (provisioned_capacity + new_volume_size)/total\n\nSo except your update here, I think it needs to add a check in line 79.\nif free\u003c\u003d0:\nreturn false","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"02a4fab0d6267bcb615152cbcedeae03fbbae7c6","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_74279712","line":104,"in_reply_to":"3afb71cf_f6b2de05","updated":"2015-06-16 09:59:27.000000000","message":"Done","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"15a595cba7671c4e1848d89fcb347bedd1ffa636","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_5eee668c","line":104,"in_reply_to":"3afb71cf_f6b2de05","updated":"2015-06-15 11:14:58.000000000","message":"So the conclusion is, if provisioned_ratio \u003c\u003d max_over_subscription_ratio, we can provision provided \u0027free\u0027 is positive.","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"cdf565ab572aeafa4507f37a777e1fa4ffadb4f1","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"3afb71cf_9cf50bcd","line":104,"in_reply_to":"3afb71cf_f6b2de05","updated":"2015-06-11 20:16:23.000000000","message":"Sounds good to me, I\u0027ll put my -1 back on then until we update to a check like you proposed.","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"469ca89edefba96c160716cdfcf399f7b3e1f2d2","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa1b9901_9ee4043c","line":104,"in_reply_to":"fa1b9901_e07336b3","updated":"2015-08-17 20:57:32.000000000","message":"I don\u0027t think capacity weigher needs to be \"fixed\".  The formula there doesn\u0027t have to be exactly the same as it is here.  We are evaluating in multiple steps in the capacity filter.  In the capacity weigher, we can only use one formula to do ranking.  We can rename \"free_virtual\" here to be \"adjusted_free_virtual\" to be more accurate.  I\u0027d like to see what Winston thinks as well.","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"6a1fdc7eac64d5672aa9758f21634d7a6583815a","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_165021d3","line":104,"in_reply_to":"fa32b979_0081b78e","updated":"2015-06-18 10:04:46.000000000","message":"small correction in your calculation:\n\ncurrent version:\n\nfree_virtual \u003d (6 - 100*.05) * 2 \u003d 2\n\nreturn False since 2 \u003c 90","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"eb367c57e19386cbc01b75bfc6598a521caf311e","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_d6a719bb","line":104,"in_reply_to":"fa32b979_165021d3","updated":"2015-06-18 10:12:34.000000000","message":"Another example to consider:\n\ntotal\u003d100G, provisioned \u003d 10G, used \u003d 10 G, free \u003d 90G, reserved \u003d 0.05, max_overratio \u003d 1.01, volume_size \u003d 91G\n\nprovisioned_ratio \u003d (10 + 91)/100 \u003d 1.01 \u003c\u003d 1.01\n\ncurrent version:\n\nfree_virtual \u003d (90 - 100*.05) * 1.01 \u003d 85.85\n\nreturn False since 85.85 \u003c 91\n\n\nnew version:\n\nfree \u003d 90 - 100 * 0.05 \u003d 85\n\nreturn True since 85 \u003e 0\n\nDo we want to discard the host when it has 90G free with thin provisioning enabled and the volume size is only 91 G?\n\nIrrespective of the formula we use, I feel comparing free virtual with volume size is not the right way if thin provisioning is enabled? We may end up not utilizing thin provisioning properly.","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"f9bf7cfc162df3f7830d0dd07a422fd5b72f532f","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_615cde4b","line":104,"in_reply_to":"fa32b979_5eee668c","updated":"2015-06-15 15:13:04.000000000","message":"Yea, I think that would solve the concerns everyone has","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"3f72354c7fe0ec402c85f34442bbb91af2a43e35","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"1a4dcd0f_9d56b25c","line":104,"in_reply_to":"fa32b979_841869d4","updated":"2015-08-12 09:51:32.000000000","message":"Winston, We both have examples to favor our reasoning. But one thing to note is, the formula here to compute free_virtual is different from the one used in capacity weigher. So it is clear that it is wrong in one of these places.\n\nAssume that we have two hosts h1 and h2.\n\nh1: total\u003d100G, provisioned \u003d 100G, used \u003d 94 G, free \u003d 6G, reserved \u003d 0.05, max_overratio \u003d 2\n\nh2: total\u003d100G, provisioned \u003d 60G, used \u003d 50 G, free \u003d 50G, reserved \u003d 0.05, max_overratio \u003d 1.5\n\nCurrently the capacity weight for h1 is 95 and h2 is 85. But h1 has only 1G free whereas h2 has 45G free.","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":2759,"name":"Huang Zhiteng","email":"winston.d@gmail.com","username":"zhiteng-huang"},"change_message_id":"eb30db99b6a42abcfae0d5dee88b9f5a38647d16","unresolved":false,"context_lines":[{"line_number":101,"context_line":"                     \"host\": host_state.host})"},{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                return True"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"        if free \u003c volume_size:"},{"line_number":107,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_841869d4","line":104,"in_reply_to":"fa32b979_d6a719bb","updated":"2015-06-18 15:18:35.000000000","message":"I think your question should be \u0027do we want to discard the host when it has 85G free capacity (not 90G) with thin provisioning enabled, but overprovision ratio is limited to 1.01, and the volume size is 91G?\u0027","commit_id":"c2bad0f6c360b2ec9c95e30560add2a7a0f845e6"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"b888bd284161ab823e21620132a3dc9161f80755","unresolved":false,"context_lines":[{"line_number":102,"context_line":"                return False"},{"line_number":103,"context_line":"            else:"},{"line_number":104,"context_line":"                # We can provision only if free space is non-zero."},{"line_number":105,"context_line":"                return free \u003e 0"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        if free \u003c volume_size:"},{"line_number":108,"context_line":"            LOG.warning(_LW(\"Insufficient free space for volume creation \""}],"source_content_type":"text/x-python","patch_set":4,"id":"ba3cc151_c0c953ae","line":105,"updated":"2015-06-29 08:13:18.000000000","message":"Hi, as you are updating the bug, it is better to put a warning here if returns false.\nAs in other places of this function, there are warnings when returning  false.","commit_id":"eb5101e4903ad26cb4e939e693868af0194b943e"}],"cinder/tests/unit/scheduler/test_host_filters.py":[{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"e05ced5beceb97ac0bc7900cd0b7d2f3048d2c9b","unresolved":false,"context_lines":[{"line_number":296,"context_line":"                                    \u0027service\u0027: service})"},{"line_number":297,"context_line":"        self.assertFalse(filt_cls.host_passes(host, filter_properties))"},{"line_number":298,"context_line":""},{"line_number":299,"context_line":"    @mock.patch(\u0027cinder.utils.service_is_up\u0027)"},{"line_number":300,"context_line":"    def test_filter_reserved_thin_thick_true_passes(self, _mock_serv_is_up):"},{"line_number":301,"context_line":"        _mock_serv_is_up.return_value \u003d True"},{"line_number":302,"context_line":"        filt_cls \u003d self.class_map[\u0027CapacityFilter\u0027]()"}],"source_content_type":"text/x-python","patch_set":1,"id":"3afb71cf_f3c74810","line":299,"updated":"2015-06-08 12:09:52.000000000","message":"May I know why you deleted this test case? Since each test case specifies a behavior, which there may be other code relying on. Delete a test case may potentially break the behavior, thus breaks the code relying on it.","commit_id":"399f90157ae52e6d52671dc358a42edb82cb6736"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"5c3189111c61dc5a6f722827c71fa1f7b13e6b19","unresolved":false,"context_lines":[{"line_number":296,"context_line":"                                    \u0027service\u0027: service})"},{"line_number":297,"context_line":"        self.assertFalse(filt_cls.host_passes(host, filter_properties))"},{"line_number":298,"context_line":""},{"line_number":299,"context_line":"    @mock.patch(\u0027cinder.utils.service_is_up\u0027)"},{"line_number":300,"context_line":"    def test_filter_reserved_thin_thick_true_passes(self, _mock_serv_is_up):"},{"line_number":301,"context_line":"        _mock_serv_is_up.return_value \u003d True"},{"line_number":302,"context_line":"        filt_cls \u003d self.class_map[\u0027CapacityFilter\u0027]()"}],"source_content_type":"text/x-python","patch_set":1,"id":"3afb71cf_f3d73eeb","line":299,"in_reply_to":"3afb71cf_f3c74810","updated":"2015-06-08 15:41:20.000000000","message":"I think that is because this test no longer fails after the code change.\n\nVipin,\n\nCan you adjust the numbers to keep this failed test?","commit_id":"399f90157ae52e6d52671dc358a42edb82cb6736"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"df739932678c5ff470a76880472d41cfa68201c0","unresolved":false,"context_lines":[{"line_number":296,"context_line":"                                    \u0027service\u0027: service})"},{"line_number":297,"context_line":"        self.assertFalse(filt_cls.host_passes(host, filter_properties))"},{"line_number":298,"context_line":""},{"line_number":299,"context_line":"    @mock.patch(\u0027cinder.utils.service_is_up\u0027)"},{"line_number":300,"context_line":"    def test_filter_reserved_thin_thick_true_passes(self, _mock_serv_is_up):"},{"line_number":301,"context_line":"        _mock_serv_is_up.return_value \u003d True"},{"line_number":302,"context_line":"        filt_cls \u003d self.class_map[\u0027CapacityFilter\u0027]()"}],"source_content_type":"text/x-python","patch_set":1,"id":"3afb71cf_bad19259","line":299,"in_reply_to":"3afb71cf_f3d73eeb","updated":"2015-06-08 18:02:39.000000000","message":"Done.\n\nI think test_filter_reserved_thin_thick_true_fails2 also tests the same code path.","commit_id":"399f90157ae52e6d52671dc358a42edb82cb6736"},{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"11b6602388103b60cad07740e857d60589042613","unresolved":false,"context_lines":[{"line_number":387,"context_line":"        self.assertFalse(filt_cls.host_passes(host, filter_properties))"},{"line_number":388,"context_line":""},{"line_number":389,"context_line":"    @mock.patch(\u0027cinder.utils.service_is_up\u0027)"},{"line_number":390,"context_line":"    def test_filter_reserved_thin_thick_true_fails3(self, _mock_serv_is_up):"},{"line_number":391,"context_line":"        _mock_serv_is_up.return_value \u003d True"},{"line_number":392,"context_line":"        filt_cls \u003d self.class_map[\u0027CapacityFilter\u0027]()"},{"line_number":393,"context_line":"        filter_properties \u003d {\u0027size\u0027: 100,"}],"source_content_type":"text/x-python","patch_set":4,"id":"fa32b979_b4c3aaec","line":390,"updated":"2015-06-17 09:27:19.000000000","message":"The \"fails2\", \"fails3\" are not very meaningful. Since they are not brought into by your commit, but changing them will be pretty favorable.","commit_id":"eb5101e4903ad26cb4e939e693868af0194b943e"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"01b8598ccff4083678c8076a5426c395921a345b","unresolved":false,"context_lines":[{"line_number":387,"context_line":"        self.assertFalse(filt_cls.host_passes(host, filter_properties))"},{"line_number":388,"context_line":""},{"line_number":389,"context_line":"    @mock.patch(\u0027cinder.utils.service_is_up\u0027)"},{"line_number":390,"context_line":"    def test_filter_reserved_thin_thick_true_fails3(self, _mock_serv_is_up):"},{"line_number":391,"context_line":"        _mock_serv_is_up.return_value \u003d True"},{"line_number":392,"context_line":"        filt_cls \u003d self.class_map[\u0027CapacityFilter\u0027]()"},{"line_number":393,"context_line":"        filter_properties \u003d {\u0027size\u0027: 100,"}],"source_content_type":"text/x-python","patch_set":4,"id":"fa32b979_8fd52f1d","line":390,"in_reply_to":"fa32b979_b4c3aaec","updated":"2015-06-17 09:41:04.000000000","message":"Sure, will do it in a separate patch.","commit_id":"eb5101e4903ad26cb4e939e693868af0194b943e"}]}