-
Notifications
You must be signed in to change notification settings - Fork 33
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
Update to CAPI 1.5 #342
base: main
Are you sure you want to change the base?
Update to CAPI 1.5 #342
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-cloudstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #342 +/- ##
==========================================
- Coverage 25.45% 25.38% -0.08%
==========================================
Files 59 59
Lines 5555 5535 -20
==========================================
- Hits 1414 1405 -9
+ Misses 4002 3990 -12
- Partials 139 140 +1 ☔ View full report in Codecov by Sentry. |
I haven't tested it, but in theory we must upgrade to CAPI 1.5 or whatever may be the latest. I would want to see an easily v0.5 release. /lgtm |
cc @shwstppr we'll need that CI fixed soon 🙏 |
/run-e2e -c 4.18 |
/retest |
@g-gaston Invalid command: /run-e2e -c 4.18
The command to run e2e test for CAPC. Usage: /run-e2e [-k Kubernetes_Version] [-c CloudStack_Version] [-h Hypervisor] [-i Template/Image] [-f Kubernetes_Version_Upgrade_From] [-t Kubernetes_Version_Upgrade_To]
Examples:
|
/run-e2e -c 4.18 |
@rohityadavcloud a jenkins job has been kicked to run test with following paramaters:
|
/test capi-provider-cloudstack-presubmit-e2e-smoke-test |
Hi @g-gaston @rohityadavcloud I ran the e2e suit manually to test the CAPI changes. And got a successful run ✅ Not sure why the presubmit is failing, |
/test capi-provider-cloudstack-presubmit-e2e-smoke-test |
/run-e2e -c 4.18 |
@rohityadavcloud a jenkins job has been kicked to run test with following paramaters:
|
@@ -182,6 +183,7 @@ func (r *CloudStackMachineReconciliationRunner) SetFailureDomainOnCSMachine() (r | |||
name = *r.CAPIMachine.Spec.FailureDomain | |||
r.ReconciliationSubject.Spec.FailureDomainName = *r.CAPIMachine.Spec.FailureDomain | |||
} else { // Not a control plane machine. Place randomly. | |||
rand := rand.New(rand.NewSource(time.Now().Unix())) // #nosec G404 -- weak crypt rand doesn't matter here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: should we make this a field in the reconciler? It helps with testing
if err != nil { | ||
r.Log.Error(err, "GetOrCreateVMInstance returned error") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we log the error if we are already returning it? Is it being silenced somewhere?
@@ -298,6 +301,9 @@ func (r *CloudStackMachineReconciliationRunner) AddToLBIfNeeded() (retRes ctrl.R | |||
// GetOrCreateMachineStateChecker creates or gets CloudStackMachineStateChecker object. | |||
func (r *CloudStackMachineReconciliationRunner) GetOrCreateMachineStateChecker() (retRes ctrl.Result, reterr error) { | |||
checkerName := r.ReconciliationSubject.Spec.InstanceID | |||
if checkerName == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good addition. However, i would prefer if we only included in this pr changes that are strictly required for the capi upgrade. Else, it makes very difficult to keep track of the rationale behind changes once the review context of this pr fades away.
@@ -298,6 +301,9 @@ func (r *CloudStackMachineReconciliationRunner) AddToLBIfNeeded() (retRes ctrl.R | |||
// GetOrCreateMachineStateChecker creates or gets CloudStackMachineStateChecker object. | |||
func (r *CloudStackMachineReconciliationRunner) GetOrCreateMachineStateChecker() (retRes ctrl.Result, reterr error) { | |||
checkerName := r.ReconciliationSubject.Spec.InstanceID | |||
if checkerName == nil { | |||
return ctrl.Result{}, errors.New(CSMachineStateCheckerCreationFailed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we return an error that actually says that instance id is nil?
WithEventFilter( | ||
predicate.Funcs{ | ||
UpdateFunc: func(e event.UpdateEvent) bool { | ||
// Avoid reconciling if the event triggering the reconciliation is related to incremental status updates | ||
// for CloudStackMachine resources only | ||
if _, ok := e.ObjectOld.(*infrav1.CloudStackMachine); !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this new code or coming from somewhere else? I can't find it sorry
Asking bc i don't really understand why is this needed
K8sClient client.Client | ||
CSClient cloud.Client | ||
Recorder record.EventRecorder | ||
WatchFilterValue string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this new field for?
if it's not used by the base reconciler, can we put it where it's used?
|
||
// CloudStackClusterToCloudStackMachines is a handler.ToRequestsFunc to be used to enqeue requests for reconciliation | ||
// of CloudStackMachines. | ||
func CloudStackClusterToCloudStackMachines(c client.Client, obj runtime.Object, scheme *runtime.Scheme, log logr.Logger) (handler.MapFunc, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add unit test for these 2 new methods?
/uncc @chrisdoherty4 |
New changes are detected. LGTM label has been removed. |
Signed-off-by: Hans Rakers <h.rakers@global.leaseweb.com>
Signed-off-by: Hans Rakers <h.rakers@global.leaseweb.com>
Signed-off-by: Hans Rakers <h.rakers@global.leaseweb.com>
Signed-off-by: Hans Rakers <h.rakers@global.leaseweb.com>
PR updated for CAPI v1.5.8. Sorry for the delay. |
if err != nil { | ||
return errors.Wrap(err, "failed to create CloudStackClusterToCloudStackMachines mapper") | ||
} | ||
//requeueCloudStackMachinesForUnpausedCluster := reconciler.requeueCloudStackMachinesForUnpausedCluster(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented code.
PR needs rebase. 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-sigs/prow repository. |
This PR has some conflicts and some comments needs to be addressed/resolved. Changing milestone for this PR to v0.6.0 for now. |
Hi, I'm sorry I haven't been able to dedicate much time on this lately cause I'm swamped in launching our k8s product at work. I will update the PR when i have some time. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
Issue #, if available:
#285
#253
Description of changes:
This PR updates the cluster-api dependency to v1.5.5. It also updates to Go 1.20, updates golang-ci and addresses lint issues caught by the new version (mostly unused variables in function definitions)
Testing performed:
make test
test with kind
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.