)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"4b975e10aba3aa699ca9ad9ea21377c8db8daa79","unresolved":false,"context_lines":[{"line_number":15,"context_line":"is 1 TB, then the virtual capacity is 1.2 TB. If 200 GBs"},{"line_number":16,"context_line":"are then allocated *and written*, then \"free\" (physical)"},{"line_number":17,"context_line":"will be 800 GB and free virtual will be 1 TB,"},{"line_number":18,"context_line":"but free * osr (800 * 1.2) will be 960 GB - incorrect."},{"line_number":19,"context_line":"The function caculate_virtual_free_capacity is made"},{"line_number":20,"context_line":"for the correct calculation."},{"line_number":21,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1f1a1f67_22a0c78f","line":18,"range":{"start_line":18,"start_character":4,"end_line":18,"end_character":26},"updated":"2017-07-19 15:59:00.000000000","message":"This is not how it works.\n\nfree \u003d free_space - math.floor(total * reserved)\n\n\"free\" does not take into account the space written.  It only deducts the space reserved.","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"},{"author":{"_account_id":22683,"name":"Arnon Yaari","email":"arnony@infinidat.com","username":"arnony"},"change_message_id":"2c60b4ad2c93ea5bab79b394a797e65041193d54","unresolved":false,"context_lines":[{"line_number":15,"context_line":"is 1 TB, then the virtual capacity is 1.2 TB. If 200 GBs"},{"line_number":16,"context_line":"are then allocated *and written*, then \"free\" (physical)"},{"line_number":17,"context_line":"will be 800 GB and free virtual will be 1 TB,"},{"line_number":18,"context_line":"but free * osr (800 * 1.2) will be 960 GB - incorrect."},{"line_number":19,"context_line":"The function caculate_virtual_free_capacity is made"},{"line_number":20,"context_line":"for the correct calculation."},{"line_number":21,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1f1a1f67_a4ebc210","line":18,"range":{"start_line":18,"start_character":4,"end_line":18,"end_character":26},"in_reply_to":"1f1a1f67_22a0c78f","updated":"2017-07-19 17:09:10.000000000","message":"xing-yang this example is from _current_ behavior. I state that it is incorrect in this line. If it is wrong then that means there is something to fix :)\nI don\u0027t deal with \"reserved\" in the examples, but the function I want to use, caculate_virtual_free_capacity, does. In the example I\u0027m assuming reserved is 0 for simplicity. The patch isn\u0027t about \"reserved\" but rather about \"max_over_subscription_ratio\" - what do we multiply, before taking into account the reserved space? do we multiply \"free physical space\" * max_osr to get \"free_space\"? Or do we multiply \"total physical space\" * max_osr and then subtract \"used\" space to get \"free_space\"?  caculate_virtual_free_capacity does the latter, apparently that one may be correct.","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"},{"author":{"_account_id":2759,"name":"Huang Zhiteng","email":"winston.d@gmail.com","username":"zhiteng-huang"},"change_message_id":"da80333375c8a940115536d5f4f32182b7e51402","unresolved":false,"context_lines":[{"line_number":15,"context_line":"is 1 TB, then the virtual capacity is 1.2 TB. If 200 GBs"},{"line_number":16,"context_line":"are then allocated *and written*, then \"free\" (physical)"},{"line_number":17,"context_line":"will be 800 GB and free virtual will be 1 TB,"},{"line_number":18,"context_line":"but free * osr (800 * 1.2) will be 960 GB - incorrect."},{"line_number":19,"context_line":"The function caculate_virtual_free_capacity is made"},{"line_number":20,"context_line":"for the correct calculation."},{"line_number":21,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1f1a1f67_8a93f25c","line":18,"range":{"start_line":18,"start_character":4,"end_line":18,"end_character":26},"in_reply_to":"1f1a1f67_a4ebc210","updated":"2017-07-21 05:33:17.000000000","message":"\u003e\u0027we multiply \"total physical space\" * max_osr and then subtract \"used\" space to get \"free_space\"\u0027\ncalculate_virtual_free_capacity does this:\n   \u0027free\u0027 \u003d total * max_osr - provisioned (note it doesn\u0027t used used capacity).\n\nThere is significant difference between provisioned capacity and actual used capacity.  What current behavior does is:\n   \u0027free\u0027 \u003d total * max_osr - used * max_osr \u003d (total - used) * max_osr\n\nDoes it make sense to count used capacity more than it is actually consumed like we are doing right now?  I think so.\n\nLet\u0027s look deeper into max_osr, and find out where it should be applied:\n  1) total / provisioned \u003c\u003d max_osr\n  2) used / provisioned \u003c\u003d 1/max_osr\n\n1) is so obvious that we all agree; 2) is not mentioned in earlier discussions but it\u0027s actually quite intuitive too.  Because max_osr basically is percentage that admin thinks (at certain confident level) that on average consumption ratio of all volumes don\u0027t go beyond/over, otherwise he/she would be in trouble.  So 1/max_osr \u003d max_consumption_ratio.  Now, let\u0027s calculate \u0027free\u0027 capacity with both 1) and 2) in mind we have:\n\n  3) \u0027free\u0027 \u003c\u003d total * max_osr - provisioned\n  4) \u0027free\u0027 * max_consumption_ratio \u003c\u003d total - used\n4) can be changed to\n  5) \u0027free\u0027 \u003c\u003d (total - used) * max_osr\n\n\nThe meaningful \u0027free\u0027 should be the lower value calculated from 3) and 5), and that\u0027s exactly what we are doing now.","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"}],"cinder/scheduler/filters/capacity_filter.py":[{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"d092aa326562378b86dbc3a777ac6e7e55953a4f","unresolved":false,"context_lines":[{"line_number":154,"context_line":"                    backend_state.max_over_subscription_ratio,"},{"line_number":155,"context_line":"                    backend_state.reserved_percentage,"},{"line_number":156,"context_line":"                    backend_state.thin_provisioning_support)"},{"line_number":157,"context_line":"                res \u003d virtual_free \u003e\u003d requested_size"},{"line_number":158,"context_line":"                if not res:"},{"line_number":159,"context_line":"                    msg_args \u003d {\"available\": virtual_free,"},{"line_number":160,"context_line":"                                \"size\": requested_size,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_0adf1869","line":157,"updated":"2017-07-19 20:42:09.000000000","message":"This code path will always return \"True\".  Your patch works actually exactly the same as this old patch:\n\nhttps://review.openstack.org/#/c/189261/3/cinder/scheduler/filters/capacity_filter.py\n\nIn utils.calculate_virtual_free_capacity(), free_virtual \u003d (total * max_over_subscription_ratio - provisioned_capacity - math.floor(total * reserved)).  This is actually already evaluated by lines 128-130.  Let\u0027s assume reserved\u003d0 for simplicity.  We are comparing whether total*max_ratio - provisioned \u003e\u003d requested in this code path.\n\nIn the if block starting from line 130, we compare:\n(provisioned+requested )/total vs max_ratio\nThis is the same as:\n(provisioned + requested) vs total * max_ratio\nThis is same as:\nrequested vs total * max_ratio - provisioned\nSo if requested \u003c total*max_ratio-provisioned, it goes to the else block here and total*max_ratio-provisioned will of course \u003e\u003d requested.\n\nTherefore, your change can be simplified to:\n\nelse:\n    return True\n\nThis is exactly the same proposal as https://review.openstack.org/#/c/189261/3/cinder/scheduler/filters/capacity_filter.py which was abandoned after the lengthy review discussion.","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"f101831ef58747bde0c88aa88164eacf32a98c78","unresolved":false,"context_lines":[{"line_number":154,"context_line":"                    backend_state.max_over_subscription_ratio,"},{"line_number":155,"context_line":"                    backend_state.reserved_percentage,"},{"line_number":156,"context_line":"                    backend_state.thin_provisioning_support)"},{"line_number":157,"context_line":"                res \u003d virtual_free \u003e\u003d requested_size"},{"line_number":158,"context_line":"                if not res:"},{"line_number":159,"context_line":"                    msg_args \u003d {\"available\": virtual_free,"},{"line_number":160,"context_line":"                                \"size\": requested_size,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_eaf5fccb","line":157,"in_reply_to":"1f1a1f67_0adf1869","updated":"2017-07-19 20:47:46.000000000","message":"Let me give an example from that old patch set I referenced.\n\ntotal\u003d100G, provisioned \u003d 100G, used \u003d 94 G, free \u003d 6G, reserved \u003d 0.00, max_ratio \u003d 2, volume_size \u003d 90G\n\nprovisioned ratio \u003d (100+90)/100 \u003d 1.9 \u003c 2 (max_ratio)\n\nIt goes to the else block:\nExisting code: adjusted_free_virtual \u003d 6*2 \u003d 12 \u003c 90, return False\n\nYour proposed code: free_virtual\u003d100*2-100 \u003d 100 \u003e 90, return True\n\nBased on the discussion from the old patch set, the existing code is safer.  That\u0027s why we decided to keep the original code and just rename free_virtual to adjusted_free_virtual.\n\nWe could probably add more explanation and add a concrete example here to avoid future confusion.","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"},{"author":{"_account_id":22683,"name":"Arnon Yaari","email":"arnony@infinidat.com","username":"arnony"},"change_message_id":"bd1f0d0644a6c917bf6804593763b4da7623be3d","unresolved":false,"context_lines":[{"line_number":154,"context_line":"                    backend_state.max_over_subscription_ratio,"},{"line_number":155,"context_line":"                    backend_state.reserved_percentage,"},{"line_number":156,"context_line":"                    backend_state.thin_provisioning_support)"},{"line_number":157,"context_line":"                res \u003d virtual_free \u003e\u003d requested_size"},{"line_number":158,"context_line":"                if not res:"},{"line_number":159,"context_line":"                    msg_args \u003d {\"available\": virtual_free,"},{"line_number":160,"context_line":"                                \"size\": requested_size,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_6a368c05","line":157,"in_reply_to":"1f1a1f67_0adf1869","updated":"2017-07-19 21:23:51.000000000","message":"That would be correct (we could replace this with \"return True\"), except _here_ we get to the point where \"reserved\" matters and should be accounted for (like the comment says). The comments on the bug you referenced were also about taking \"reserved\" into account here.","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"1fc52591f4a1c08fe3b5f2f43c3b842a19e9df2d","unresolved":false,"context_lines":[{"line_number":154,"context_line":"                    backend_state.max_over_subscription_ratio,"},{"line_number":155,"context_line":"                    backend_state.reserved_percentage,"},{"line_number":156,"context_line":"                    backend_state.thin_provisioning_support)"},{"line_number":157,"context_line":"                res \u003d virtual_free \u003e\u003d requested_size"},{"line_number":158,"context_line":"                if not res:"},{"line_number":159,"context_line":"                    msg_args \u003d {\"available\": virtual_free,"},{"line_number":160,"context_line":"                                \"size\": requested_size,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_edccf689","line":157,"in_reply_to":"1f1a1f67_6a368c05","updated":"2017-07-19 21:28:22.000000000","message":"Are you saying \"reserved\" is not accounted for?  See line 105: free \u003d free_space - math.floor(total * reserved)","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"aa23d211620a929802252a2dc43f6c696c2a7e67","unresolved":false,"context_lines":[{"line_number":154,"context_line":"                    backend_state.max_over_subscription_ratio,"},{"line_number":155,"context_line":"                    backend_state.reserved_percentage,"},{"line_number":156,"context_line":"                    backend_state.thin_provisioning_support)"},{"line_number":157,"context_line":"                res \u003d virtual_free \u003e\u003d requested_size"},{"line_number":158,"context_line":"                if not res:"},{"line_number":159,"context_line":"                    msg_args \u003d {\"available\": virtual_free,"},{"line_number":160,"context_line":"                                \"size\": requested_size,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_6ff21672","line":157,"in_reply_to":"1f1a1f67_ad1a7ec2","updated":"2017-07-20 03:03:43.000000000","message":"In the filter we can do evaluations in more granularity so we do this in several steps. In the if block, we compared (requested) vs (total * max_ratio - provisioned).  In the else block, we used (free - reserved) * max_ratio.\n\nIn weigher, we have to use one formula to capture all so we can rank them.  So we have virtual free \u003d total * max_ratio - provisioned - reserved which is in utils.calculate_virtual_free_capacity().","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"0a4358b68e350e39c5ca54198b52171e60ba8eb8","unresolved":false,"context_lines":[{"line_number":154,"context_line":"                    backend_state.max_over_subscription_ratio,"},{"line_number":155,"context_line":"                    backend_state.reserved_percentage,"},{"line_number":156,"context_line":"                    backend_state.thin_provisioning_support)"},{"line_number":157,"context_line":"                res \u003d virtual_free \u003e\u003d requested_size"},{"line_number":158,"context_line":"                if not res:"},{"line_number":159,"context_line":"                    msg_args \u003d {\"available\": virtual_free,"},{"line_number":160,"context_line":"                                \"size\": requested_size,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_66fe85af","line":157,"in_reply_to":"1f1a1f67_b4cf35af","updated":"2017-07-21 13:24:33.000000000","message":"Hi Winston,\n\nAre you suggesting to change the formula in utils.calculate_virtual_free_capacity() to something like this?\n\n        free_virtual \u003d (total-used-reserved) * max_over_subscription_ratio\n\nI think it makes sense but it is a significant change.  Not only that it changes the ranking but also that the results from utils.calculate_virtual_free_capacity have already been published through Ceilometer for admin to view.  So this will be a user visible change and we need to have a release note for it.","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"1fc52591f4a1c08fe3b5f2f43c3b842a19e9df2d","unresolved":false,"context_lines":[{"line_number":154,"context_line":"                    backend_state.max_over_subscription_ratio,"},{"line_number":155,"context_line":"                    backend_state.reserved_percentage,"},{"line_number":156,"context_line":"                    backend_state.thin_provisioning_support)"},{"line_number":157,"context_line":"                res \u003d virtual_free \u003e\u003d requested_size"},{"line_number":158,"context_line":"                if not res:"},{"line_number":159,"context_line":"                    msg_args \u003d {\"available\": virtual_free,"},{"line_number":160,"context_line":"                                \"size\": requested_size,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_0d8052f4","line":157,"in_reply_to":"1f1a1f67_ca2c80ad","updated":"2017-07-19 21:28:22.000000000","message":"As I said, utils.calculate_virtual_free_capacity() is already taken account for by the if block.  This calculation here is redundant and will always return True.","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"aa23d211620a929802252a2dc43f6c696c2a7e67","unresolved":false,"context_lines":[{"line_number":154,"context_line":"                    backend_state.max_over_subscription_ratio,"},{"line_number":155,"context_line":"                    backend_state.reserved_percentage,"},{"line_number":156,"context_line":"                    backend_state.thin_provisioning_support)"},{"line_number":157,"context_line":"                res \u003d virtual_free \u003e\u003d requested_size"},{"line_number":158,"context_line":"                if not res:"},{"line_number":159,"context_line":"                    msg_args \u003d {\"available\": virtual_free,"},{"line_number":160,"context_line":"                                \"size\": requested_size,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_2f977e3c","line":157,"in_reply_to":"1f1a1f67_ca2c80ad","updated":"2017-07-20 03:03:43.000000000","message":"If more space is actually consumed (written), it should affect how much user can virtually provision further.  We should add that protection.  That\u0027s what line 150 on the left (the original code) is for.","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"},{"author":{"_account_id":2759,"name":"Huang Zhiteng","email":"winston.d@gmail.com","username":"zhiteng-huang"},"change_message_id":"da80333375c8a940115536d5f4f32182b7e51402","unresolved":false,"context_lines":[{"line_number":154,"context_line":"                    backend_state.max_over_subscription_ratio,"},{"line_number":155,"context_line":"                    backend_state.reserved_percentage,"},{"line_number":156,"context_line":"                    backend_state.thin_provisioning_support)"},{"line_number":157,"context_line":"                res \u003d virtual_free \u003e\u003d requested_size"},{"line_number":158,"context_line":"                if not res:"},{"line_number":159,"context_line":"                    msg_args \u003d {\"available\": virtual_free,"},{"line_number":160,"context_line":"                                \"size\": requested_size,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_b4cf35af","line":157,"in_reply_to":"1f1a1f67_ca2c80ad","updated":"2017-07-21 05:33:17.000000000","message":"The problem with calculate_virtual_free_capacity() is that it does the calculation *without* consideration of actual used capacity when thin provision is enabled, which can be too optimistic at least.\n\nYour example also leaves alone the used capacity out of equation.  Total\u003d100G, max_over_subscription_ratio\u003d2, and provisioned\u003d1, and consider these two extreme case: 1) used\u003d0G, 2) used\u003d100G. Your proposal treats them without any difference at all.  In case 2), the cluster is already full occupied/consumed, what\u0027s the point allowing a new 100G volume being provisioned onto such a cluster?  \n\nAnd I don\u0027t think current behavior is confusing at all. Scheduler makes every decision based on what it learns from backend at that moment it\u0027s being invoked.  Used capacity is dynamic and scheduler should to take this (alone with other \u0027static\u0027/dynamic factors) into account.  I find no reason that we should leave this very important fact alone.\n\nI think we should fix calculate_virtual_free_capacity() instead.","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"},{"author":{"_account_id":22683,"name":"Arnon Yaari","email":"arnony@infinidat.com","username":"arnony"},"change_message_id":"bd1f0d0644a6c917bf6804593763b4da7623be3d","unresolved":false,"context_lines":[{"line_number":154,"context_line":"                    backend_state.max_over_subscription_ratio,"},{"line_number":155,"context_line":"                    backend_state.reserved_percentage,"},{"line_number":156,"context_line":"                    backend_state.thin_provisioning_support)"},{"line_number":157,"context_line":"                res \u003d virtual_free \u003e\u003d requested_size"},{"line_number":158,"context_line":"                if not res:"},{"line_number":159,"context_line":"                    msg_args \u003d {\"available\": virtual_free,"},{"line_number":160,"context_line":"                                \"size\": requested_size,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_ca2c80ad","line":157,"in_reply_to":"1f1a1f67_eaf5fccb","updated":"2017-07-19 21:23:51.000000000","message":"I\u0027m still in the opinion that if we say max_over_subscription_ratio is 2.0 and total\u003d100G, then we should allow 200 GB subscription. if we provisioned 100 GB, we should have 90 GB left to provision...\nThe number \"12\" calculated here is meaningless: if we can provision a 12 GB volume, we\u0027ll still have 2 GB that can\u0027t actually be written so we don\u0027t really protect the user against this (and I don\u0027t see why the user needs protection here - we\u0027re practically disabling over provisioning feature if we do)... Also, this number (12) would vary wildly based on used space. If I allocate that 100 GB volume and write just 1 GB, then the calculation here will be 99*2\u003d198. So I would be able to allocate another 90 GB volume successfully. Later when I write more data I suddenly can\u0027t...? This is very confusing and doesn\u0027t make any sense to me. Even more so because it\u0027s not consistent with how max_over_subscription_ratio is used in calculate_virtual_free_capacity.\nIf this really was the decision then I think it\u0027s worth reopening the discussion.","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"},{"author":{"_account_id":22683,"name":"Arnon Yaari","email":"arnony@infinidat.com","username":"arnony"},"change_message_id":"c71e8c6c58a65e1f7640face708f3cefd3e44e00","unresolved":false,"context_lines":[{"line_number":154,"context_line":"                    backend_state.max_over_subscription_ratio,"},{"line_number":155,"context_line":"                    backend_state.reserved_percentage,"},{"line_number":156,"context_line":"                    backend_state.thin_provisioning_support)"},{"line_number":157,"context_line":"                res \u003d virtual_free \u003e\u003d requested_size"},{"line_number":158,"context_line":"                if not res:"},{"line_number":159,"context_line":"                    msg_args \u003d {\"available\": virtual_free,"},{"line_number":160,"context_line":"                                \"size\": requested_size,"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_ad1a7ec2","line":157,"in_reply_to":"1f1a1f67_edccf689","updated":"2017-07-19 21:37:29.000000000","message":"no no. I\u0027m saying that both the current code and the new code should take \"reserved\" into account. The current code accounts for reserved by using \"free\" in line 105. The new code that I am proposing should also do that, and I\u0027m doing that by using calculate_virtual_free_capacity which takes \"reserved\" into account inside. I *can\u0027t* replace it with \"return True\" exactly for this reason. It will not always return True.\nThe \"if\" from before *doesn\u0027t* take reserved into account, so we have to do it here (that\u0027s why we currently do that, too).","commit_id":"2e219dbfc252953632aaca59ce71b53c8b8084f2"}]}