Discard and replace new VMs if they never join the cluster #123
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
✅ Deploy Preview for kubernetes-sigs-cluster-api-cloudstack ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
k8s-ci-robot
added
the
size/M
Codecov Report
@@ Coverage Diff @@
## main #123 +/- ##
==========================================
- Coverage 29.00% 28.90% -0.11%
==========================================
Files 27 27
Lines 2179 2204 +25
==========================================
+ Hits 632 637 +5
- Misses 1473 1493 +20
Partials 74 74
Continue to review full report at Codecov.
|
| func (s *CloudStackMachineStatus) TimeSinceLastStateChange() time.Duration { | ||
| if s.InstanceStateLastUpdated.IsZero() { | ||
| return time.Duration(-1) | ||
| } | ||
| return time.Since(s.InstanceStateLastUpdated.Time) | ||
| } |
There was a problem hiding this comment.
could we start adding some unit tests here?
There was a problem hiding this comment.
Good idea. I just added some.
|
Just a reminder that this is still waiting for approval. |
|
|
||
| // capiTimeout indicates that a new VM is running, but it isn't reachable due to a network issue or a misconfiguration. | ||
| // When this happens, the machine should be deleted or else the cluster won't ever recover. | ||
| capiTimeout := csRunning && !capiRunning && csTimeInState > 5*time.Minute |
There was a problem hiding this comment.
should this be configurable instead of hardcoded? Maybe default to 5 minutes, unless some env var is set or something?
There was a problem hiding this comment.
I'd love to have a good mechanism for making it configurable. I wish we could put something in the EKS-A cluster definition file, but nothing there passes through nicely. Is there a good way to set environment variables on the VMs in bulk?
There was a problem hiding this comment.
Let's create a GitHub issue and move forward without this configuration for now, since it's not essential.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maxdrib, mrog 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
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Issue #, if available:
Description of changes:
When a new VM is added to a cluster, it's possible that the VM isn't reachable on the network due to a misconfiguration or network failure. This causes CAPC to wait forever, and the cluster is never completed.
This change makes it so VMs are deleted if they don't become ready in CAPI within 5 minutes of being ready in CloudStack. The VM will then be replaced, and cluster creation can continue. Note that this change only affects newly created VMs that have not yet successfully joined a cluster.
Testing performed:
I tested it manually using EKS-Anywhere. I verified that it works for self-managed clusters, management clusters, and workload clusters. I tested with control plane nodes and worker nodes.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.