{"action":"created","issue":{"url":"https://api.github.com/repos/os-autoinst/openQA/issues/1536","repository_url":"https://api.github.com/repos/os-autoinst/openQA","labels_url":"https://api.github.com/repos/os-autoinst/openQA/issues/1536/labels{/name}","comments_url":"https://api.github.com/repos/os-autoinst/openQA/issues/1536/comments","events_url":"https://api.github.com/repos/os-autoinst/openQA/issues/1536/events","html_url":"https://github.com/os-autoinst/openQA/pull/1536","id":280650968,"number":1536,"title":"`limit_assets`: remove asset that breaks limit, not next one","user":{"login":"AdamWill","id":916551,"avatar_url":"https://avatars1.githubusercontent.com/u/916551?v=4","gravatar_id":"","url":"https://api.github.com/users/AdamWill","html_url":"https://github.com/AdamWill","followers_url":"https://api.github.com/users/AdamWill/followers","following_url":"https://api.github.com/users/AdamWill/following{/other_user}","gists_url":"https://api.github.com/users/AdamWill/gists{/gist_id}","starred_url":"https://api.github.com/users/AdamWill/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AdamWill/subscriptions","organizations_url":"https://api.github.com/users/AdamWill/orgs","repos_url":"https://api.github.com/users/AdamWill/repos","events_url":"https://api.github.com/users/AdamWill/events{/privacy}","received_events_url":"https://api.github.com/users/AdamWill/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2017-12-08T23:51:21Z","updated_at":"2017-12-09T00:05:47Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/os-autoinst/openQA/pulls/1536","html_url":"https://github.com/os-autoinst/openQA/pull/1536","diff_url":"https://github.com/os-autoinst/openQA/pull/1536.diff","patch_url":"https://github.com/os-autoinst/openQA/pull/1536.patch"},"body":"At present, when looping through the assets for a given job\r\ngroup, we decide whether to make the asset a deletion candidate\r\n*before* we count its size against the limits. So effectively\r\nwe keep the minimum amount of assets needed to *exceed* the\r\nsize limit, rather than the maximum amount of assets that can\r\nfit within it. This is clearly incorrect. We can fix it by\r\ncounting the asset's size against the limits *before* we decide\r\nwhether it should be a deletion candidate.\r\n\r\nThis is an alternative to #1532 by @coolo - I think this way\r\nof fixing it is clearer and better. It also fixes the tests.\r\n@coolo couldn't figure out what was going on with the tests,\r\nand I don't blame him, because it was quite tricky. In fact\r\nwhat happened is that my understanding of the interaction\r\nbetween the code and the assets used in the tests was slightly\r\nwrong, but ironically, *this very bug* happened to exactly\r\n'offset' my faulty understanding, so the tests I wrote happened\r\nto pass. I have added some comments to hopefully explain this\r\ncorrectly and in detail.\r\n\r\nSigned-off-by: Adam Williamson <awilliam@redhat.com>"},"comment":{"url":"https://api.github.com/repos/os-autoinst/openQA/issues/comments/350403377","html_url":"https://github.com/os-autoinst/openQA/pull/1536#issuecomment-350403377","issue_url":"https://api.github.com/repos/os-autoinst/openQA/issues/1536","id":350403377,"user":{"login":"AdamWill","id":916551,"avatar_url":"https://avatars1.githubusercontent.com/u/916551?v=4","gravatar_id":"","url":"https://api.github.com/users/AdamWill","html_url":"https://github.com/AdamWill","followers_url":"https://api.github.com/users/AdamWill/followers","following_url":"https://api.github.com/users/AdamWill/following{/other_user}","gists_url":"https://api.github.com/users/AdamWill/gists{/gist_id}","starred_url":"https://api.github.com/users/AdamWill/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AdamWill/subscriptions","organizations_url":"https://api.github.com/users/AdamWill/orgs","repos_url":"https://api.github.com/users/AdamWill/repos","events_url":"https://api.github.com/users/AdamWill/events{/privacy}","received_events_url":"https://api.github.com/users/AdamWill/received_events","type":"User","site_admin":false},"created_at":"2017-12-09T00:05:46Z","updated_at":"2017-12-09T00:05:46Z","author_association":"CONTRIBUTOR","body":"So a fun note on the question of *why* we count against the limit at the end: here's how the `while` loop looked in a very early version of the function looked (after commit a2b851785):\r\n\r\n        while (my $a = $assets->next) {\r\n            next if $a->get_column('asset_type') ne 'iso';\r\n            my $file = sprintf(\"%s/%s/%s\", $OpenQA::Utils::assetdir,$a->get_column('asset_type'),$a->get_column('asset_name'));\r\n            my @st = stat($file);\r\n            if (@st && $st[7] > 0) {\r\n                $keep{$a->asset_id} = 1;\r\n                $sizelimit -= $st[7];\r\n            }\r\n            # check after keeping - so we can be sure to keep at least one even if sizelimit too strict\r\n            last if $sizelimit < 0;\r\n        }\r\n\r\nAnd that was written by....@coolo ;) So at least if I understand this correctly, @coolo specifically put the decrement of `$sizelimit` after the initial 'keep' decision, but included the `last if` check, with the intent that we'd usually remain within the size limit (thanks to the `last if` check), *but* if there was only one asset, and it exceeded the size limit, it'd be kept. Simply decrementing `$sizelimit` before the check would've meant that a single asset that exceeded the size limit would be removed.\r\n\r\nHowever, the `last if` check dropped out in commit 104bf719e211073cc6a38072d31bedcbc9c02b9e (also by @coolo ), and this has been wrong ever since.\r\n\r\nSo we could actually put that `last if` back in, more or less, if we still wanted the behaviour @coolo originally designed. But from the other PR:\r\n\r\n\"I think this breaks user's expectations - as a limit of 0 in my book means 0 assets and not one asset, no matter what size it is.\"\r\n\r\nit sounds like 2017-model @coolo thinks 2015-model @coolo is an idiot ;)"}}
{"actor":{"display_login":"AdamWill"}}