Skip to content

Commit

Permalink
controller: remove IPAMClaim finalizers on VMI delete events
Browse files Browse the repository at this point in the history 
This way, we ensure the CNI only returns the IP addresses to the pool
after the VMI has been deleted on VM foreground deletes.

Without it, the IPAMClaim would be deleted by the Kubernetes garbage
collector before the VMI was deleted, which would cause the IP address
to be released to the pool (by the CNI) prematurely. More details on
[0].

[0] - https://kubernetes.io/docs/concepts/architecture/garbage-collection/#foreground-deletion

Signed-off-by: Miguel Duarte Barroso <[email protected]>
  • Loading branch information
@maiqueb
maiqueb committed May 17, 2024
1 parent 5623761 commit baecdd1
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 32 deletions.
38 changes: 31 additions & 7 deletions pkg/vmnetworkscontroller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (

controllerruntime "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/predicate"

Expand Down Expand Up @@ -55,9 +56,11 @@ func (r *VirtualMachineReconciler) Reconcile(
defer cancel()
err := r.Client.Get(ctx, request.NamespacedName, vmi)
if apierrors.IsNotFound(err) {
r.Log.Error(err, "Error retrieving VMI")
// Error reading the object - requeue the request.
return controllerruntime.Result{}, err
r.Log.Info("could not retrieve VMI - will cleanup its IPAMClaims")
if err := r.Cleanup(request.NamespacedName); err != nil {
return controllerruntime.Result{}, fmt.Errorf("error removing the IPAMClaims finalizer: %w", err)
}
return controllerruntime.Result{}, nil
}

var ownerInfo metav1.OwnerReference
Expand Down Expand Up @@ -88,9 +91,7 @@ func (r *VirtualMachineReconciler) Reconcile(
Namespace: vmi.Namespace,
OwnerReferences: []metav1.OwnerReference{ownerInfo},
Finalizers: []string{kubevirtVMFinalizer},
Labels: map[string]string{
virtv1.VirtualMachineLabel: vmi.Name,
},
Labels: ownedByVMLabel(vmi.Name),
},
Spec: ipamclaimsapi.IPAMClaimSpec{
Network: netConfigName,
Expand Down Expand Up @@ -145,7 +146,7 @@ func onVMPredicates() predicate.Funcs {
return true
},
DeleteFunc: func(event.DeleteEvent) bool {
return false
return true
},
UpdateFunc: func(updateEvent event.UpdateEvent) bool {
return false
Expand Down Expand Up @@ -201,3 +202,26 @@ func (r *VirtualMachineReconciler) vmiNetworksClaimingIPAM(
}
return vmiNets, nil
}

func (r *VirtualMachineReconciler) Cleanup(vmiKey apitypes.NamespacedName) error {
ipamClaims := &ipamclaimsapi.IPAMClaimList{}
if err := r.Client.List(context.Background(), ipamClaims, ownedByVMLabel(vmiKey.Name)); err != nil {
return fmt.Errorf("could not get list of IPAMClaims owned by VM %q: %w", vmiKey.String(), err)
}

for _, claim := range ipamClaims.Items {
removedFinalizer := controllerutil.RemoveFinalizer(&claim, kubevirtVMFinalizer)
if removedFinalizer {
if err := r.Client.Update(context.Background(), &claim, &client.UpdateOptions{}); err != nil {
return client.IgnoreNotFound(err)
}
}
}
return nil
}

func ownedByVMLabel(vmiName string) client.MatchingLabels {
return map[string]string{
virtv1.VirtualMachineLabel: vmiName,
}
}
96 changes: 71 additions & 25 deletions pkg/vmnetworkscontroller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ type testConfig struct {
existingIPAMClaim *ipamclaimsapi.IPAMClaim
expectedError error
expectedResponse reconcile.Result
expectedIPAMClaims []ipamclaimsapi.IPAMClaimSpec
expectedIPAMClaims []ipamclaimsapi.IPAMClaim
}

var _ = Describe("vm IPAM controller", Serial, func() {
Expand Down Expand Up @@ -96,6 +96,14 @@ var _ = Describe("vm IPAM controller", Serial, func() {
initialObjects = append(initialObjects, config.existingIPAMClaim)
}

if vmiKey.Namespace == "" && vmiKey.Name == "" {
// must apply some default for the VMI DEL scenarios ...
vmiKey = apitypes.NamespacedName{
Namespace: namespace,
Name: vmName,
}
}

ctrlOptions := controllerruntime.Options{
Scheme: scheme.Scheme,
NewClient: func(_ *rest.Config, _ client.Options) (client.Client, error) {
Expand All @@ -121,20 +129,26 @@ var _ = Describe("vm IPAM controller", Serial, func() {

if len(config.expectedIPAMClaims) > 0 {
ipamClaimList := &ipamclaimsapi.IPAMClaimList{}
Expect(mgr.GetClient().List(context.Background(), ipamClaimList, &client.ListOptions{
Namespace: config.inputVMI.Namespace,
})).To(Succeed())
Expect(ipamClaimsSpecExtractor(ipamClaimList.Items...)).To(ConsistOf(config.expectedIPAMClaims))

Expect(mgr.GetClient().List(context.Background(), ipamClaimList, ownedByVMLabel(vmName))).To(Succeed())
Expect(ipamClaimsCleaner(ipamClaimList.Items...)).To(ConsistOf(config.expectedIPAMClaims))
}
},
Entry("when the VM has an associated VMI pointing to an existing NAD", testConfig{
inputVM: dummyVM(nadName),
inputVMI: dummyVMI(nadName),
inputNAD: dummyNAD(nadName),
expectedResponse: reconcile.Result{},
expectedIPAMClaims: []ipamclaimsapi.IPAMClaimSpec{
expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{
{
Network: "goodnet",
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("%s.%s", vmName, "randomnet"),
Namespace: namespace,
Finalizers: []string{kubevirtVMFinalizer},
Labels: ownedByVMLabel(vmName),
OwnerReferences: []metav1.OwnerReference{{Name: vmName}},
},
Spec: ipamclaimsapi.IPAMClaimSpec{Network: "goodnet"},
},
},
}),
Expand Down Expand Up @@ -162,17 +176,27 @@ var _ = Describe("vm IPAM controller", Serial, func() {
},
}),
Entry("the VMI does not exist on the datastore - it might have been deleted in the meantime", testConfig{
expectedError: &errors.StatusError{
ErrStatus: metav1.Status{
Status: "Failure",
Message: "virtualmachineinstances.kubevirt.io \"\" not found", // no name printed since we're not passing a VMI
Reason: "NotFound",
Details: &metav1.StatusDetails{
Name: "",
Group: "kubevirt.io",
Kind: "virtualmachineinstances",
expectedResponse: reconcile.Result{},
}),
Entry("the VMI was deleted, thus the existing IPAMClaims finalizers must be removed", testConfig{
expectedResponse: reconcile.Result{},
existingIPAMClaim: &ipamclaimsapi.IPAMClaim{
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("%s.%s", vmName, "randomnet"),
Namespace: namespace,
Finalizers: []string{kubevirtVMFinalizer},
Labels: ownedByVMLabel(vmName),
},
Spec: ipamclaimsapi.IPAMClaimSpec{Network: "doesitmatter?"},
},
expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{
{
ObjectMeta: metav1.ObjectMeta{
Name: "vm1.randomnet",
Namespace: "ns1",
Labels: ownedByVMLabel(vmName),
},
Code: 404,
Spec: ipamclaimsapi.IPAMClaimSpec{Network: "doesitmatter?"},
},
},
}),
Expand Down Expand Up @@ -205,23 +229,46 @@ var _ = Describe("vm IPAM controller", Serial, func() {
UID: dummyUID,
},
},
Labels: ownedByVMLabel(vmName),
Finalizers: []string{kubevirtVMFinalizer},
},
Spec: ipamclaimsapi.IPAMClaimSpec{Network: "doesitmatter?"},
},
expectedResponse: reconcile.Result{},
expectedIPAMClaims: []ipamclaimsapi.IPAMClaimSpec{
expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{
{
Network: "doesitmatter?",
ObjectMeta: metav1.ObjectMeta{
Name: "vm1.randomnet",
Namespace: "ns1",
Labels: ownedByVMLabel(vmName),
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: "v1",
Kind: "virtualmachines",
Name: "vm1",
UID: dummyUID,
},
},
Finalizers: []string{kubevirtVMFinalizer},
},
Spec: ipamclaimsapi.IPAMClaimSpec{Network: "doesitmatter?"},
},
},
}),
Entry("a lonesome VMI (with no corresponding VM) is a valid migration use-case", testConfig{
inputVMI: dummyVMI(nadName),
inputNAD: dummyNAD(nadName),
expectedResponse: reconcile.Result{},
expectedIPAMClaims: []ipamclaimsapi.IPAMClaimSpec{
expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{
{
Network: "goodnet",
ObjectMeta: metav1.ObjectMeta{
Name: "vm1.randomnet",
Namespace: "ns1",
Labels: ownedByVMLabel(vmName),
Finalizers: []string{kubevirtVMFinalizer},
OwnerReferences: []metav1.OwnerReference{{Name: vmName}},
},
Spec: ipamclaimsapi.IPAMClaimSpec{Network: "goodnet"},
},
},
}),
Expand Down Expand Up @@ -297,12 +344,11 @@ func dummyNADWrongFormat(nadName string) *nadv1.NetworkAttachmentDefinition {
}
}

func ipamClaimsSpecExtractor(ipamClaims ...ipamclaimsapi.IPAMClaim) []ipamclaimsapi.IPAMClaimSpec {
ipamClaimsSpec := make([]ipamclaimsapi.IPAMClaimSpec, len(ipamClaims))
func ipamClaimsCleaner(ipamClaims ...ipamclaimsapi.IPAMClaim) []ipamclaimsapi.IPAMClaim {
for i := range ipamClaims {
ipamClaimsSpec[i] = ipamClaims[i].Spec
ipamClaims[i].ObjectMeta.ResourceVersion = ""
}
return ipamClaimsSpec
return ipamClaims
}

func decorateVMWithUID(uid string, vm *virtv1.VirtualMachine) *virtv1.VirtualMachine {
Expand Down

0 comments on commit baecdd1

Please sign in to comment.