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

machine actuator implementation #16

Closed
jichenjc opened this issue Apr 15, 2019 · 6 comments
Closed

machine actuator implementation #16

jichenjc opened this issue Apr 15, 2019 · 6 comments
Assignees

Comments

@jichenjc
Copy link
Contributor

/kind feature
we need add Create/Update/Delete/Exists functions to machine so that we can make it able to be consumed by cluster-api Reconcile function

we can create several patches to add those functions

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

@jichenjc
Copy link
Contributor Author

jichenjc commented Apr 18, 2019

This can be enhanced to be

  • Create
  • Delete
  • Update
  • Exists
  • GetIP
  • GetKubeConfig

let's add one by one

@jichenjc jichenjc self-assigned this Apr 18, 2019
@jichenjc
Copy link
Contributor Author

before that, we need something like
ClusterStatusFromProviderStatus
ClusterSpecFromProviderSpec

so we can consume the status and spec from cluster-api.....

@gyliu513
Copy link
Contributor

@xunpan will work for update.

FYI @jichenjc

@xunpan xunpan mentioned this issue May 9, 2019
@xunpan
Copy link
Contributor

xunpan commented May 9, 2019

Discussion about @jichenjc 's comments
https://github.com/kubernetes-sigs/cluster-api-provider-ibmcloud/pull/128/files/8d9a595c053465a9bb00cffbfbd47c6dc5edf32d#r282150120

why we have pending status? if you see line 90 this means for already created instance you create it again
you will mark as pending? I think just creating is enough ? if you agree I can create a PR to update it @xunpan

Common Case

Currently, the behavior is:
If a new machine is created, there should not be a VM instance already. So that API can create a new VM instance with bootstrap configuration.

Existing Case

I guess there has logic issue here, if there has an exist VM instance. We should:

  • report error: we will not update a existing machine. Then we should delete this 'Pending' status.
  • Pending in status is needed. It tells the Machine is just created. We can use update operation to delete existing VM instance (better to check if it meets our spec before deletion) and create a new one.

I think option 2 should be better. Because we create name of vm instance which should be not duplicated in cloud. Even if it is duplicated (e.g. Machine transfer case, or the VM is created manually, or by other cluster-api instances), we should identify this machine is managed by cluster-api (better to know current cluster instance ID I guess) or not.
However, there is still many things need to be investigation. And it is my thought for the behavior of Existing Case. Any commnets for this @jichenjc @gyliu513

If update is needed, refer below thoughts

Update

I'm considering how to implement update. This is logic in my mind:

  1. support compute node
    • check if anything changed for VM, delete and recreate if exists
    • check if K8s status is ok, if not, try to run start up scripts in the machine after a TTL.
  2. support master node
    • we need related operations as compute node
    • further checkes and with cluster check to reflect K8s configuration change if needed.

@jichenjc
Copy link
Contributor Author

jichenjc commented May 9, 2019

I agree with the approach on the update flow (delete and recreate) ,I think we can make it as it is now, but we'd better leave this open to track the TODO items to verify and agree on the behavior

for this:
Pending in status is needed. It tells the Machine is just created. We can use update operation to delete existing VM instance (better to check if it meets our spec before deletion) and create a new one.

I think I need more time to try and have more input here, basically I am not against add pending status but we need make sure it's useful and we can have full picture on the state transition for guest we created
otherwise it will bring confusion to enduser/admin

so let's keep this issue open and provide more input here after more test

@jichenjc
Copy link
Contributor Author

jichenjc commented Jun 5, 2019

close this as all things completed

@jichenjc jichenjc closed this as completed Jun 5, 2019
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

No branches or pull requests

3 participants