[Digital Ocean] Handle logic for kops edit/update cluster #9116
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @srikiz. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/L
k8s-ci-robot
added
the
area/provider/digitalocean
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
pkg/resources/digitalocean/cloud.go
Outdated
| volumes, err := getAllVolumesByRegion(c, c.RegionName) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("error describing volumes: %v", err) |
There was a problem hiding this comment.
Maybe use a error message closer to the function name we called and include the region name, e.g.:
return nil, fmt.Errorf("failed to get all volumes by region from %s: %v", c.RegionName, err)
pkg/resources/digitalocean/cloud.go
Outdated
| var etcdClusterSpec *etcd.EtcdClusterSpec | ||
|
|
||
| for _, myTag := range volume.Tags { | ||
| klog.V(2).Infof("findEtcdStatus status (from cloud): %v", myTag) |
There was a problem hiding this comment.
Perhaps the message should be more about the tag, for instance:
"findEtcdStatus status (from cloud): checking if volume with tag %q belongs to cluster", myTag"
pkg/resources/digitalocean/cloud.go
Outdated
| var etcdClusterSpec *etcd.EtcdClusterSpec | ||
|
|
||
| for _, myTag := range volume.Tags { | ||
| klog.V(2).Infof("findEtcdStatus status (from cloud): %v", myTag) |
There was a problem hiding this comment.
Should this be at a higher log level to decrease verbosity, especially because other log statements in this function is a verbosity of 10?
There was a problem hiding this comment.
Sure, changed to verbosity of 10.
pkg/resources/digitalocean/cloud.go
Outdated
| // check if volume belongs to this cluster. | ||
| // tag will be in the format "KubernetesCluster:dev5-k8s-local" (where clusterName is dev5.k8s.local) | ||
| clusterName := strings.Replace(cluster.Name, ".", "-", -1) | ||
| if strings.Contains(myTag, fmt.Sprintf("%s, %s", TagKubernetesClusterNamePrefix, ":", clusterName)) { |
There was a problem hiding this comment.
Am I misreading this one, or does the call to fmt.Sprintf use 3 variables but only 2 placeholders?
There was a problem hiding this comment.
That was a miss - I updated it as part of the future commit.
pkg/resources/digitalocean/cloud.go
Outdated
| for _, volumeTag := range volume.Tags { | ||
| if strings.Contains(volumeTag, TagKubernetesClusterIndex) { | ||
| if len(strings.Split(volumeTag, ":")) < 2 { | ||
| return nil, fmt.Errorf("error splitting the volume tag %q on volume %q", volumeTag, volume) |
There was a problem hiding this comment.
Can we make the message more specific about the fact that we have too few components in the tag?
pkg/resources/digitalocean/cloud.go
Outdated
| if len(strings.Split(volumeTag, ":")) < 2 { | ||
| return nil, fmt.Errorf("error splitting the volume tag %q on volume %q", volumeTag, volume) | ||
| } | ||
| dropletIndex := strings.Split(volumeTag, ":")[1] |
There was a problem hiding this comment.
nit: if we call strings.Split() first and check the length on the outcome, we only need a single call to strings.Split(). That is,
volumeTagParts := strings.Split(volumeTag, ":")
if len(volumeTagParts) < 2 {
<error handling>
}
dropletIndex := volumeTagParts[1]There was a problem hiding this comment.
Good catch - thanks, updated.
| // tag will be in the format "KubernetesCluster:dev5-k8s-local" (where clusterName is dev5.k8s.local) | ||
| clusterName := strings.Replace(cluster.Name, ".", "-", -1) | ||
| if strings.Contains(myTag, fmt.Sprintf("%s:%s", TagKubernetesClusterNamePrefix, clusterName)) { | ||
| klog.V(10).Infof("findEtcdStatus cluster comparison matched for tag: %v", myTag) |
There was a problem hiding this comment.
Wondering if 10 could be too high. AFAIK, level 8 is where Kubernetes tends to log very specific details at the HTTP request level. I'm not too familiar with how kops handles log levels in general, so not a super strong opinion here.
There was a problem hiding this comment.
I will keep it with 10 for now. There are lot places I had seen 10 before, and that's how I continued to use that. I'll keep a note to change this to 8 for all DO work as a separate PR at a later point.
| dropletIndex := strings.Split(volumeTag, ":")[1] | ||
| etcdClusterSpec, err = c.getEtcdClusterSpec(volume.Name, dropletIndex) | ||
| klog.V(10).Infof("findEtcdStatus etcdClusterSpec: %v", fi.DebugAsJsonString(etcdClusterSpec)) | ||
| if err != nil { |
There was a problem hiding this comment.
The error should be handled before we log the spec in the line right above.
pkg/resources/digitalocean/cloud.go
Outdated
| } | ||
| } | ||
|
|
||
| var status []kops.EtcdClusterStatus |
There was a problem hiding this comment.
This can be improved slightly performance-wise since we know the target size of the slice:
status := make([]kops.EtcdClusterStatus, 0, len(statusMap))
|
Thanks @srikiz! /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rdrgmnzs, srikiz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Added logic for FindCluster Status
Also fixed load balancer logic to only create LB if it doesn't exist.
Tested with kops edit cluster by adding new Instance group and was able to see the cluster updated as expected.