Support for external infra clusters#39
Conversation
|
|
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
- add InfraClusterSecretRef to KubevirtClusterSpec - add example template - add InfraCluster construct to incapsulate infra cluster client and context - move cluster management resource cleanup logic to `reconcileDelete` func in controllers - enhance logging Signed-off-by: Alex Gradouski <agradouski@apple.com>
davidvossel
left a comment
davidvossel
left a comment
There was a problem hiding this comment.
Looks pretty good, I had one kind of high level comment about how to differentiate the KubeVirtMachine's namespace from the underlying KubeVirt VM's namespace on the external cluster (more detailed comment is made in-line inside the review)
The suggestion of using a VMITemplate in our api might also help us in the future if we ever need to set special annotations/labels on the VMI which is independent of the KubeVirtMachine.
| if ctx.KubevirtMachine.Spec.ProviderID != nil { | ||
| ctx.Logger.Info("KubevirtMachine.Spec.ProviderID is set -- VM provisioning complete!") | ||
| // ensure ready state is set. | ||
| // This is required after move, because status is not moved to the target cluster. | ||
| ctx.KubevirtMachine.Status.Ready = true | ||
| conditions.MarkTrue(ctx.KubevirtMachine, infrav1.VMProvisionedCondition) | ||
| return ctrl.Result{}, nil | ||
| } else { | ||
| ctx.Logger.Info("KubevirtMachine.Spec.ProviderID is not set -- will work on bootstrapping machine...") |
There was a problem hiding this comment.
This change shouldn't be necessary, right? I think ctx.KubevirtMachine.Status.Ready will only be true once providerId and the VMProvisionedCondition are set.
| ctx.Logger.Info("Waiting for the Bootstrap provider controller to set bootstrap data") | ||
| infraClusterClient, infraClusterNamespace, err := r.InfraCluster.GenerateInfraClusterClient(ctx.ClusterContext()) | ||
| if err != nil { | ||
| ctx.Logger.Error(err, "Infra cluster client is NOT available.") |
There was a problem hiding this comment.
should this return an error as well as the log message?
I get that the next line below this will cause the function to return most likely in the case that infraClusterClient == nil. It just catches my eye when we attempt to use results from a function that returns an error.
| ctx.Logger.Info("Checking if VM already exists...") | ||
| // Create a helper for managing the KubeVirt VM hosting the machine. | ||
| externalMachine, err := kubevirthandler.NewMachine(ctx, r.Client) | ||
| externalMachine, err := kubevirthandler.NewMachine(ctx, infraClusterClient, infraClusterNamespace) |
There was a problem hiding this comment.
Right now, the namespace for the external KubeVirt VMs is inferred by introspecting the kubeconfig data for the external config. I'm unsure about that.
I think the issue we're trying to solve here is how to express the location of the underlying KubeVirt VM from the KubeVirtMachine's namespace.
One way to do this is to modify our KubeVirtMachine API to use a VMI template instead of a VMI Spec.
so, This
// KubevirtMachineSpec defines the desired state of KubevirtMachine.
type KubevirtMachineSpec struct {
VMSpec kubevirtv1.VirtualMachineInstanceSpec `json:"vmSpec,omitempty"`
Could change to this...
// KubevirtMachineSpec defines the desired state of KubevirtMachine.
type KubevirtMachineSpec struct {
VMITemplate kubevirtv1.VirtualMachineInstanceTemplateSpec `json:"vmiTemplate,omitempty"`
The advantage here is that the VMITemplate object has a ObjectMetadata field, where we can specify labels,annotations and even the namespace of the VMI independently of the KubeVirtMachine.
| func (r *KubevirtMachineReconciler) reconcileDelete(ctx *context.MachineContext) (ctrl.Result, error) { | ||
| infraClusterClient, infraClusterNamespace, err := r.InfraCluster.GenerateInfraClusterClient(ctx.ClusterContext()) | ||
| if err != nil { | ||
| ctx.Logger.Error(err, "Infra cluster client is not available.") |
There was a problem hiding this comment.
seems like we should return an error here.
|
|
||
| ctx.Logger.Info("Deleting VM bootstrap secret...") | ||
| if err := r.deleteKubevirtBootstrapSecret(ctx, infraClusterClient, infraClusterNamespace); err != nil { | ||
| ctx.Logger.Error(err, "Failed to delete bootstrap secret.") |
There was a problem hiding this comment.
another place where maybe a return is needed?
|
thanks for your review @davidvossel ! regarding the namespace for KubeVirt VMs, completely agree. this is a bit of a hack for time being, that I was looking to address in a future PR. in my mind it's blocked on #11, aligned with your thinking. we need that PR to land, in order to fix the issue with the namespace here, as well as some other internal issue that requires VMI labels to be set via the template. (today it's not possible, without VMI -> VM spec update.) completely agree with other comments regarding return statements, working on the fix. |
- return on errors, with retries Signed-off-by: Alex Gradouski <agradouski@apple.com>
k8s-ci-robot
added
the
needs-rebase
@agradouski yep, makes total sense. while #11 is being worked on, how do you feel about this external cluster PR? Do you think we should wait on #11 to land, or continue forward with the external cluster independently (sorting out the namespace issue later on)? I think this PR is fine as long as we're proceeding with the understanding that the namespace of the external VMs will be impacted by #11 later on, likely in a backwards incompatible way. |
|
@davidvossel yes, I'm okay with proceeding with external cluster work independently of #11 for now, with the understanding that it will need to be updated with the follow-up PR. Created issue to address that in the future: #51 |
k8s-ci-robot
added
size/XXL
k8s-ci-robot
added
size/XL
Signed-off-by: Alex Gradouski <agradouski@apple.com>
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: agradouski, cchengleo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
What this PR does / why we need it:
Currently, there is an assumption that both management resources and underlying infrastructure resources (Kubevirt VMs) are created on a single cluster. In other words, management cluster is the same as infrastructure cluster.
This change adds support for external infrastructure clusters. Meaning, that we now can decouple the management cluster that will only maintain the management resources for tenant cluster with actual infrastructure for tenant clusters.
This is achieved by injecting an external cluster kubeconfig into management cluster, and referencing it in the
KubevirtClusterresource. The logic will use that kubeconfig if it's provided and create all infra resources (VMs) in that cluster.This change is backward compatible in that if
infraClusterSecretRefis not provided inKubevirtCluster's spec, then the same cluster will be used for tenant cluster infra.Note, in order to use external cluster as infra, connectivity need to be ensured between the management cluster and that infra cluster.
Which issue this PR fixes: fixes #32
Special notes for your reviewer:
Release notes: