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

Drop requirement for DBRs #85

Merged
merged 11 commits into from
Oct 5, 2021
Merged

Drop requirement for DBRs #85

merged 11 commits into from
Oct 5, 2021

Conversation

ibrokethecloud
Copy link
Contributor

What this PR does / why we need it:
Gargantua has a complex logic to try static and dynamic binds.
This results in a lot of dynamic bind requests being created, when something like a slow api server is preventing the object processing to be completed in time.
The session server already specifies the VirtualMachineClaims to be dynamic in nature.
This PR cleans up the VirtualMachineClaim controller logic to handle vm interaction and reconcillation within the VMC controller.

Which issue(s) this PR fixes:

@ibrokethecloud ibrokethecloud marked this pull request as ready for review September 21, 2021 23:44
@ebauman
Copy link
Member

ebauman commented Sep 22, 2021

Tested this locally. Works fine when provisioning VMs.
However after clicking Finish my garg logs were spammed with

E0922 18:14:29.141361       1 util.go:181] virtualmachine.hobbyfarm.io "vmc-dp733qqzhp-2267913e" not found
E0922 18:14:29.141383       1 util.go:181] virtualmachine.hobbyfarm.io "vmc-dp733qqzhp-2267913e" not found
E0922 18:14:29.141404       1 util.go:181] virtualmachine.hobbyfarm.io "vmc-dp733qqzhp-2267913e" not found

@ebauman
Copy link
Member

ebauman commented Sep 22, 2021

might we also want to issue a new version of the virtualmachineclaim crd and remove the reference to dynamicbindrequest from the status?

@ibrokethecloud
Copy link
Contributor Author

Tested this locally. Works fine when provisioning VMs.
However after clicking Finish my garg logs were spammed with

E0922 18:14:29.141361       1 util.go:181] virtualmachine.hobbyfarm.io "vmc-dp733qqzhp-2267913e" not found
E0922 18:14:29.141383       1 util.go:181] virtualmachine.hobbyfarm.io "vmc-dp733qqzhp-2267913e" not found
E0922 18:14:29.141404       1 util.go:181] virtualmachine.hobbyfarm.io "vmc-dp733qqzhp-2267913e" not found

@ebauman the new commit addresses this. I have changed the logging behavior in util.VerifyVM method.

…er immediately reaps finished sessions. Also dropped needed for reaping dbr's when session is finished
@jggoebel
Copy link
Member

jggoebel commented Sep 28, 2021

When having restirected bind disabled on a SE we are receiving a
"incorrect number of dbc matching sessionName found" in vmclaimcontroller line 436.
Likely this is the result of the label selector in line 427 which does not fit for dbc with restrictedbind="false".
No DBC for this event is found and the provisioning of vms is not possible.

When restrictedbind is enabled this works very nice! Good Job!
We are testing this with heavy load in the next days and will likely provide more feedback!

@ibrokethecloud
Copy link
Contributor Author

@jggoebel i will check out the scenario with restricted bind disabled as well.. should be an easy fix.

@jggoebel
Copy link
Member

jggoebel commented Sep 29, 2021

I have not tested the new commit yet but it looks promising!

Unfortunately though we have found another issue with this PR.
When having "on demand" disabled a SE will preprovision the VMs.
However with the additions added by this PR a new VM is created when a user starts a scenario. The preprovisioned VMs do not get used.

Another issue i found is that the "on demand" setting can not be updated when editing an event. This is not an issue with this PR but could be fixed as well. When having "on demand" disabled the admin UI also shows "on demand" as enabled while editing the event.

Edit: i fixed the other issue regarding the update of these settings in
gargantua #87
and the admin-ui hobbyfarm/admin-ui#91

@ibrokethecloud
Copy link
Contributor Author

Currently disabling on-demand provisioning is mostly useless. It provisions the extra compute which is never used.

Even without these changes, the sessionserver generates a VirtualMachineClaim which is by default DynamicCapable.

As a result of this the vmclaimcontroller spins up infra to handle this request. Since the sessionserver never checks the nature of the scheduledevent, we always defaulting to on-demand provisioning.

We don't plan to have static pre-provisioning in the future anyways, so i see no reason to handle this scenario, when the virtualmachineclaim types are always going to be dynamic capable.

@jggoebel
Copy link
Member

I tested this again on the current master state.
Yes, a VMC is created but it refers to a pre-provisioned machine. No new VMs are created and the user receives the scheduled VM. No unused VM is created, exactly like we want it.

I think it is a valid option to preprovision machines. Sometimes provisioning a machine with a large image takes some minutes. So the option to preprovision them bevore the event starts is useful to prevent long waiting times for the user.

@ebauman ebauman merged commit 47a9c68 into hobbyfarm:master Oct 5, 2021
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.

3 participants