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

Create and delete machines via controller #36

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

mhmxs
Copy link
Contributor

@mhmxs mhmxs commented Dec 1, 2023

I open this PR for transparency. I'm still testing, but i think it is ready to discuss :).

This change doing reconciliation loop for Linode Machines. Only create and delete has been implemented. You can find some buxfixes and code cleanups too, i tried to push the scope as low as possible.

Fixes: #25
Fixes: #11

@mhmxs
Copy link
Contributor Author

mhmxs commented Dec 1, 2023

@luthermonson @zliang-akamai I'm a bit afraid of re-queuing every instance we wait for, maybe it makes sense to retry only X times and dropping the event instead of filling workqueue and waste reconciliation threads WDYT?:

if lastKnownState != linodego.InstanceRunning {
			logger.Info("Instance is not ready, wait for running...", "state", lastKnownState)

			res = ctrl.Result{RequeueAfter: reconciler.DefaultMachineControllerWaitForRunningDelay}
		}

@mhmxs mhmxs marked this pull request as ready for review December 5, 2023 13:35
@luthermonson
Copy link
Collaborator

@luthermonson @zliang-akamai I'm a bit afraid of re-queuing every instance we wait for, maybe it makes sense to retry only X times and dropping the event instead of filling workqueue and waste reconciliation threads WDYT?:

if lastKnownState != linodego.InstanceRunning {
			logger.Info("Instance is not ready, wait for running...", "state", lastKnownState)

			res = ctrl.Result{RequeueAfter: reconciler.DefaultMachineControllerWaitForRunningDelay}
		}

want to extend the back off as it gets older? if these dont end up running we could be in a way worse case for the data center so eventually stopping at 10 or 15min might be plent?

@mhmxs
Copy link
Contributor Author

mhmxs commented Dec 13, 2023

@luthermonson It is ready to merge. I couldn't test the change, because of the missing other parts of the project, but verified the behavior based on other well known provider.

Copy link
Contributor

@avestuk avestuk left a comment

Choose a reason for hiding this comment

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

LGTM as is. Comments are nits/prompts for futher discussion we can have after merge if that's desirable.

return ctrl.Result{}, nil
default:
match := false
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about using the slices package for this?

		if !slices.ContainsFunc(linodeMachine.OwnerReferences, func(or metav1.OwnerReference) bool {
			return or.UID == machine.UID
		}) {
			log.Info("Failed to find the referenced owner machine, skipping reconciliation", "references", linodeMachine.OwnerReferences, "machine", machine.ObjectMeta)
		}
			return ctrl.Result{}, nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@avestuk in my personal opinion is slices package breaks one of the principles of Golang: simplicity. I would be the last who suggest adopting it :D But of course the question is what we think about his teamwise.

}
}()

// Delete
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about breaking the delete/update/create parts of this function out into their own functions? I don't mind if we do it later in another PR (I'm happy to take a crack at it for instance) but I think the function would be more easily understood if we did so.

cloud/scope/cluster.go Outdated Show resolved Hide resolved
@luthermonson luthermonson merged commit f9ca95d into linode:main Dec 18, 2023
@luthermonson
Copy link
Collaborator

good to see some progress! well done @mhmxs and thanks for the help @avestuk

cbzzz pushed a commit to cbzzz/cluster-api-provider-linode that referenced this pull request Feb 6, 2024
cbzzz pushed a commit to cbzzz/cluster-api-provider-linode that referenced this pull request Feb 6, 2024
cbzzz pushed a commit to cbzzz/cluster-api-provider-linode that referenced this pull request Feb 6, 2024
amold1 pushed a commit that referenced this pull request May 17, 2024
* Create and delete machines via controller

* Update cloud/scope/cluster.go

Co-authored-by: Alex Vest <[email protected]>

---------

Co-authored-by: Richard Kovacs <[email protected]>
Co-authored-by: Alex Vest <[email protected]>
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.

Implement business logic to create Linode instances Define API for LinodeMachine
3 participants