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

neonvm-controller: patch status if reconcile fails (#889) #1009

Closed

Conversation

agadelshin
Copy link
Contributor

@agadelshin agadelshin commented Jul 14, 2024

Added status update on fail. There is a way to just run it in a defer, but it's only 2 places so I guess it's more straightforward solution.

Also changed Update to Patch, added request of virtualmachine before to avoid conflicts.

And a last one - removed requeue after succesful reconcile, it's not required to reconcile if nothing happens, reconcile will be queued if there will be a change in a VirtualMachine or Pod with VirtualMachine owner. If there is another reason to keep it in a constant loop - please tell, will update it. Otherwise it'll save some CPU cycles and make logs more readable.

Tested it manually on a virtualmachine inside GCP, if there is a way to run autoscaling on Silicon Apple, I'd be happy to know (x86-64 emulation in QEMU is too slow, took hours to build everything).

@sharnoff sharnoff self-requested a review July 15, 2024 00:57
@agadelshin
Copy link
Contributor Author

More I think about it - if requeue is still required for some reason - it's better to increase duration between runs to some reasonable period like 10-30 seconds.

@agadelshin agadelshin force-pushed the agadelshin/patch-status branch from 929d943 to dda7cc2 Compare July 22, 2024 16:34
@agadelshin
Copy link
Contributor Author

agadelshin commented Jul 22, 2024

Removed requeAfter changes because it's already a part of #1016

@Omrigan
Copy link
Contributor

Omrigan commented Nov 19, 2024

Closed in favor of #1102 and #1103.

@agadelshin Thank you for the proactive proposal!

@Omrigan Omrigan closed this Nov 19, 2024
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