Implements delete cluster and toolbox dumper command #54
Conversation
abrarshivani
requested review from
msterin,
luomiao,
SandeepPissay and
prashima
|
After deleting cluster A, the state store of cluster A should be deleted too. |
| @@ -329,6 +329,91 @@ func (c *VSphereCloud) FindVMUUID(vm *string) (string, error) { | |||
| return vmResult.Config.Uuid, nil | |||
| } | |||
|
|
|||
| func (c *VSphereCloud) VirtualMachines(args []string) ([]*object.VirtualMachine, error) { | |||
There was a problem hiding this comment.
What is this method doing? It's not clear from the name of this method.
Also, please add relevant comments for methods.
There was a problem hiding this comment.
Good Catch! Will add the comment
| } | ||
|
|
||
| type vsphereListFn func() ([]*ResourceTracker, error) | ||
|
|
||
| func (c *AwsCluster) listResourcesVSphere() (map[string]*ResourceTracker, error) { |
There was a problem hiding this comment.
Why not use something generic like KubernetesCluster, or VSphereCluster if every cloud provider needs to have its own type name?
If possible, please address this in current PR. AwsCluster inside delete_cluster_vsphere.go doesn't look intuitive. Otherwise we can declare a new type that inherits AwsCluster.
There was a problem hiding this comment.
Agreed, the type name AwsCluster doesn't look good in delete_cluster_vsphere. This will be changed in upstream. They have todo comment which says that.
| return nil, nil | ||
| vsphereCloud := c.Cloud.(*vsphere.VSphereCloud) | ||
| if c.Region == "" { | ||
| c.Region = vsphereCloud.Cluster |
There was a problem hiding this comment.
Can you please explain where is this getting used?
There was a problem hiding this comment.
It is used not anywhere for now.
There was a problem hiding this comment.
Let's not set it, if it's not getting used anywhere.
| d.listVMs, | ||
| } | ||
|
|
||
| for _, fn := range listFunctions { |
There was a problem hiding this comment.
Can you please explain what's the use of putting a single function in list and iterating over that list?
It would have made sense if we were trying to list VMs using multiple clusterDiscoveryVSphere instances. But with current usage, I am not able to see benefit of iterating over listFunctions.
There was a problem hiding this comment.
This will help adding functions in future.
| if _, ok := err.(*find.NotFoundError); !ok { | ||
| return nil, err | ||
| } | ||
| glog.Warning(err) |
There was a problem hiding this comment.
Let's discuss this offline
There was a problem hiding this comment.
We didn't discuss this one. Can you please comment?
| func (d *clusterDiscoveryVSphere) listVMs() ([]*ResourceTracker, error) { | ||
| c := d.vsphereCloud | ||
|
|
||
| var trackers []*ResourceTracker |
There was a problem hiding this comment.
Nit: please move the declaration closer to usage, at line#93.
| return nil | ||
| } | ||
|
|
||
| func DumpVM(r *ResourceTracker) (interface{}, error) { |
There was a problem hiding this comment.
What is this method dumping?
Can you please explain why is the return type interface{}, where as the type of the 'data' variable is map[string]interface{}?
There was a problem hiding this comment.
This method is dumping VM info.
There was a problem hiding this comment.
Resource tracker requires this signature func (r *ResourceTracker) (interface{}, error) for function DumpVM. We are returning multiple values so we need a map or slice to store it. When this function returns map[string]interface{} datatype it gets converted into interface type.
Note: any type in golang can be converted into interface type.
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| vm := r.obj.(*object.VirtualMachine) |
There was a problem hiding this comment.
Can you please explain what this line is doing?
There was a problem hiding this comment.
This typecast obj to struct object.VirtualMachine pointer
| } | ||
|
|
||
| // passing empty cd-rom name so that the first one gets returned | ||
| cdrom, err := devices.FindCdrom("") |
There was a problem hiding this comment.
Can this method be modified to get all cdrom devices and check backing.FileName for all of them. One that matches the iso name regex, that is being created by vSphere attachiso task, will get deleted.
This way there won't be necessity for this assumption that first cdrom should be the one that has the cloud-init's iso attached. Just in case user goes and messes up with the VM.
|
@luomiao the cluster state and kubeconfig gets deleted. So we don't need separate PR |
|
LGTM. |
abrarshivani
force-pushed
the
remote-delete_cluster
branch
from
110eb2d to
81c2a35
Compare
This PR implements delete cluster and toolbox dump command for kops.
Fixes #14