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

Add a cleanup loop for failed vm #40

Conversation

SchSeba
Copy link
Collaborator

@SchSeba SchSeba commented Jul 1, 2019

Before this PR we have an issue if the vm creation fails because there
is an validation error from the virt-api we didn't clean the allocation.

This PR introduce a cleanup loop into the system.
When we hit the create vm mutating webhook we create the regular
allocation but we also configure the vm in a wait map.
We have a waiting look the will check if the object is removed from the
map and if is not after 3-6 second we assume the object wasn't saved
into the etcd (no controller event) so we release the object.

Before this PR we have an issue if the vm creation fails because there
is an validation error from the virt-api we didn't clean the allocation.

This PR introduce a cleanup loop into the system.
When we hit the create vm mutating webhook we create the regular
allocation but we also configure the vm in a wait map.
We have a waiting look the will check if the object is removed from the
map and if is not after 3-6 second we assume the object wasn't saved
into the etcd (no controller event) so we release the object.
@SchSeba SchSeba requested a review from phoracek July 1, 2019 10:28
@SchSeba
Copy link
Collaborator Author

SchSeba commented Jul 1, 2019

should fix #37

@SchSeba
Copy link
Collaborator Author

SchSeba commented Jul 2, 2019

we should use a persistent storage for this because the kubemacpool can stop after the mutating webhook but before the controller ack on the request.

delete(p.vmCreationWaiting, vmName)
}

// This function check if there are virtual machines that hits the create
Copy link
Member

Choose a reason for hiding this comment

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

Nice documentation 👍 !

@@ -96,5 +96,6 @@ func (r *ReconcilePolicy) Reconcile(request reconcile.Request) (reconcile.Result
return reconcile.Result{}, err
}

r.poolManager.MarkVMAsReady(fmt.Sprintf("%s/%s", request.Namespace, request.Name))
Copy link
Member

Choose a reason for hiding this comment

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

maybe it would be nicer to pass namespace and name as arguments and join them inside the function

@@ -82,6 +83,7 @@ func (p *PoolManager) AllocateVirtualMachineMac(virtualMachine *kubevirt.Virtual
}

virtualMachine.Spec.Template.Spec.Domain.Devices.Interfaces = copyVM.Spec.Template.Spec.Domain.Devices.Interfaces
p.vmCreationWaiting[vmNamespaced(virtualMachine)] = 0
Copy link
Member

Choose a reason for hiding this comment

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

maybe map[string]bool would look better, so you do p.vmCreationWaiting[...] = true, makes more sense IMO. But even better would be to have a helper function

@@ -113,6 +115,7 @@ func (p *PoolManager) ReleaseVirtualMachineMac(virtualMachineName string) error
}

delete(p.vmToMacPoolMap, virtualMachineName)
delete(p.vmCreationWaiting, virtualMachineName)
Copy link
Member

Choose a reason for hiding this comment

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

can't you use MarkVMAsReady function?

@SchSeba
Copy link
Collaborator Author

SchSeba commented Aug 27, 2019

replaced by #59

@SchSeba SchSeba closed this Aug 27, 2019
@SchSeba SchSeba deleted the fix_reapplying_invalid_yaml branch August 27, 2019 09:46
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.

2 participants