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

Fixed 404 error when deleting disk from VM. #551

Merged
merged 1 commit into from
Apr 9, 2018

Conversation

bond95
Copy link
Contributor

@bond95 bond95 commented Apr 6, 2018

Fixes: #543


This change is Reviewable

@bond95 bond95 requested a review from jniederm April 6, 2018 11:58
Copy link
Contributor

@jniederm jniederm left a comment

Choose a reason for hiding this comment

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

code LGTM


please add short description to commit message. git log doesn't integrate well with github.

@bond95 bond95 force-pushed the error-disk-deleting branch from 0106a17 to 10e4074 Compare April 6, 2018 12:36
@bond95
Copy link
Contributor Author

bond95 commented Apr 6, 2018

@jniederm Done.

@mareklibra
Copy link
Contributor

@bond95 , thanks for the patch and commit message. The fix LGTM.

Anyway, it would be great if you can explain more deeply (in the commit message) the flow leading to 404 and the reason why canBeMissing=true is the proper fix.
It's clear now, but good commit message might help in the future when making related changes.

Bug: VM Portal show 404 error after deleting disk from VM. Problem in calling `callExternalAction` function, which has `canBeMissing` as last parameter. That parameter allow to get error as response from server without error message in VM Portal.
Fixes: oVirt#543
@bond95 bond95 force-pushed the error-disk-deleting branch from 10e4074 to 088fca4 Compare April 6, 2018 13:48
@bond95
Copy link
Contributor Author

bond95 commented Apr 6, 2018

@mareklibra Done.

@sjd78
Copy link
Member

sjd78 commented Apr 6, 2018

@bond95 Looks like your change really just tells the disk fetch to ignore fails. I think error is probably a timing issue. Remove the disk ... something asks for its details in future and gets a 404 since it has already been removed.

Take a look at src/components/VmDisks/sagas.js. The removeDisk() function kinda does that on purpose.

EDIT - Oops. That's what you fixed! Too many windows and tabs open at once today.

I suggest a commit description along the lines of, "The removeDisk() function polls the deleted disk and once the 404 is received it refreshes the disk list. The 404 is an expected response and no error needs to be handled and logged."

@mareklibra mareklibra merged commit 33a887a into oVirt:master Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants