Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CA-128095 #238

Closed
wants to merge 2 commits into from
Closed

CA-128095 #238

wants to merge 2 commits into from

Conversation

kostaslambda
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.24%) when pulling 27a7502 on kostaslamda:CA-128095 into a11482a on xapi-project:master.

@kostaslambda
Copy link
Contributor Author

Test this please

@germanop
Copy link
Contributor

Acked-by: Germano Percossi germano.percossi@citrix.com

@germanop germanop changed the title CA-28095 CA-128095 Dec 10, 2015
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 28.625% when pulling def4e5d on kostaslamda:CA-128095 into 40c7963 on xapi-project:master.

@germanop
Copy link
Contributor

germanop commented Apr 4, 2017

OK, back on this after quite some time :-)

The raise in there is too drastic.
We need just to fix the case where we leave the (huge and horrible) try block.
There are too many things that can fail in there, for different reasons and not all of them should
make the entire load fail.

Just check that at the end of the block we have at least one session and if not, raise a generic iscsi failure.

I think this is the simplest and lest invasive thing we can do

@@ -153,6 +153,7 @@ def load(self, sr_uuid):
self.session.xenapi.PBD.set_device_config(pbd, dconf)
except:
util.logException("LVHDoISCSISR.load")
raise
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raising here has many unwanted effects.

Let's just raise if we did not manage to have any valid session (the reason why the next indexing fails).

There is a big try/except block in load() that does not handle all possible
exceptions. If an exception is not handled in place, it is logged and the
function tries to continue immediately after the try/except block. Some
errors are recoverable further down, but not all.

When an exception is thrown, log it and save it; if immediatelly after the
try/except block an 'IndexError' exception is raised, catch it and throw
an 'SROSError' one instead.

Signed-off-by: Kostas Ladopoulos <konstantinos.ladopoulos@citrix.com>
Reviewed-by: Germano Percossi <germano.percossi@citrix.com>
@kostaslambda kostaslambda force-pushed the CA-128095 branch 3 times, most recently from a96b9ea to 585583b Compare May 24, 2017 14:46
Signed-off-by: Kostas Ladopoulos <konstantinos.ladopoulos@citrix.com>
Reviewed-by: Germano Percossi <germano.percossi@citrix.com>
@MarkSymsCtx
Copy link
Contributor

Superseded by PR-358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants