Report out-of-space, not patch-invalid #1598

Closed
wants to merge 6 commits into
from

Projects

None yet

2 participants

@mcclurmc

If there isn't enough space on the filesystem to upload a patch (which can take
2-3 times the size of the patch, because of multiple copies and gpg signature
checking), we should fail with an Out_of_space exception. Before, if the gpg
check failed because there wasn't enough space to write the unsigned cleartext,
we would erroneously report patch_invalid. Now we check that there is enough
free space to signature check the patch (we make a guess of 2 * size of the
patch), and catch Unix.ENOSPC exceptions and handle them accordingly.

CA-124008

Signed-off-by: Mike McClurg mike.mcclurg@citrix.com

Mike McClurg Report out-of-space, not patch-invalid
If there isn't enough space on the filesystem to upload a patch (which can take
2-3 times the size of the patch, because of multiple copies and gpg signature
checking), we should fail with an Out_of_space exception. Before, if the gpg
check failed because there wasn't enough space to write the unsigned cleartext,
we would erroneously report patch_invalid. Now we check that there is enough
free space to signature check the patch (we make a guess of 2 * size of the
patch), and catch Unix.ENOSPC exceptions and handle them accordingly.

CA-124008

Signed-off-by: Mike McClurg <mike.mcclurg@citrix.com>
412b60b
@thomassa
Xapi Project member

The patch calls assert_space_available before read_in_and_check_patch in pool_patch_upload_handler but not in get_patch_to_local, so it only checks on the master. We should check on slaves too.
(Maybe move the call to assert_space_available to inside get_patch_to_local.)

@mcclurmc

Hi @thomassa. I double checked the code, and found out that get_patch_to_local uploads the patch to the slaves through pool_patch_upload_handler, so the free-space check is being performed on the slaves as well, without modification to this pull request. See:

https://github.com/mcclurmc/xen-api/blob/412b60bea61397a77a1c799077d2bcda97fd60b0/ocaml/xapi/xapi_pool_patch.ml#L508

Mike McClurg added some commits Jan 31, 2014
Mike McClurg Whitespace fix for get_patch_to_local
Signed-off-by: Mike McClurg <mike.mcclurg@citrix.com>
da0ea34
Mike McClurg assert_space_available takes optional multiplier arg
Signed-off-by: Mike McClurg <mike.mcclurg@citrix.com>
76fb912
Mike McClurg get_patch_to_local checks filesystem space on slave
Signed-off-by: Mike McClurg <mike.mcclurg@citrix.com>
6ad1df6
Mike McClurg mkdir -p /var/patch, which might not be there
Signed-off-by: Mike McClurg <mike.mcclurg@citrix.com>
948fea1
Mike McClurg whitespace: spaces to tabs
Signed-off-by: Mike McClurg <mike.mcclurg@citrix.com>
4cb25e0
@mcclurmc

Okay, I triple checked the code now :) Sorry @thomassa, you were right about the get_patch_to_local function - it was calling a different http handler, and I just didn't notice the difference in URLs. I've updated this branch with a couple whitespace changes, and a fix to get_patch_to_local so that it calls assert_space_available before downloading the patch.

@thomassa
Xapi Project member

If assert_space_available finds insufficient space it logs a warning message, but this message states the required space without the multiplier. It should state the value after multiplication. (The log call wasn't updated when the multiplier parameter was added to the function.)

@thomassa
Xapi Project member

I've fixed that in pull-req #1678 which supersedes this one: closing this.

@thomassa thomassa closed this Apr 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment