Merge branch 'main' into oleg/latency-metrics

neonvm/apis/neonvm/v1/virtualmachine_webhook.go

@@ -22,24 +22,19 @@ import (
 	"reflect"
 	"slices"
 
-	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
-func (r *VirtualMachine) SetupWebhookWithManager(mgr ctrl.Manager) error {
-	return ctrl.NewWebhookManagedBy(mgr).
-		For(r).
-		Complete()
-}
-
 //+kubebuilder:webhook:path=/mutate-vm-neon-tech-v1-virtualmachine,mutating=true,failurePolicy=fail,sideEffects=None,groups=vm.neon.tech,resources=virtualmachines,verbs=create;update,versions=v1,name=mvirtualmachine.kb.io,admissionReviewVersions=v1
 
 var _ webhook.Defaulter = &VirtualMachine{}
 
-// Default implements webhook.Defaulter so a webhook will be registered for the type
+// Default implements webhook.Defaulter
+//
+// The controller wraps this logic so it can inject extra control in the webhook.
 func (r *VirtualMachine) Default() {
 	// Nothing to do.
 }
@@ -48,7 +43,9 @@ func (r *VirtualMachine) Default() {
 
 var _ webhook.Validator = &VirtualMachine{}
 
-// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
+// ValidateCreate implements webhook.Validator
+//
+// The controller wraps this logic so it can inject extra control.
 func (r *VirtualMachine) ValidateCreate() (admission.Warnings, error) {
 	// validate .spec.guest.cpus.use and .spec.guest.cpus.max
 	if r.Spec.Guest.CPUs.Use < r.Spec.Guest.CPUs.Min {
@@ -119,7 +116,9 @@ func (r *VirtualMachine) ValidateCreate() (admission.Warnings, error) {
 	return nil, nil
 }
 
-// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
+// ValidateUpdate implements webhook.Validator
+//
+// The controller wraps this logic so it can inject extra control.
 func (r *VirtualMachine) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
 	// process immutable fields
 	before, _ := old.(*VirtualMachine)
@@ -214,7 +213,9 @@ func (r *VirtualMachine) ValidateUpdate(old runtime.Object) (admission.Warnings,
 	return nil, nil
 }
 
-// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
+// ValidateDelete implements webhook.Validator
+//
+// The controller wraps this logic so it can inject extra control in the webhook.
 func (r *VirtualMachine) ValidateDelete() (admission.Warnings, error) {
 	// No deletion validation required currently.
 	return nil, nil

neonvm/apis/neonvm/v1/virtualmachinemigration_webhook.go

@@ -17,24 +17,19 @@ limitations under the License.
 package v1
 
 import (
-	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
-func (r *VirtualMachineMigration) SetupWebhookWithManager(mgr ctrl.Manager) error {
-	return ctrl.NewWebhookManagedBy(mgr).
-		For(r).
-		Complete()
-}
-
 //+kubebuilder:webhook:path=/mutate-vm-neon-tech-v1-virtualmachinemigration,mutating=true,failurePolicy=fail,sideEffects=None,groups=vm.neon.tech,resources=virtualmachinemigrations,verbs=create;update,versions=v1,name=mvirtualmachinemigration.kb.io,admissionReviewVersions=v1
 
 var _ webhook.Defaulter = &VirtualMachineMigration{}
 
-// Default implements webhook.Defaulter so a webhook will be registered for the type
+// Default implements webhook.Defaulter
+//
+// The controller wraps this logic so it can inject extra control in the webhook.
 func (r *VirtualMachineMigration) Default() {
 	// TODO: implement defaults
 }
@@ -43,19 +38,25 @@ func (r *VirtualMachineMigration) Default() {
 
 var _ webhook.Validator = &VirtualMachineMigration{}
 
-// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
+// ValidateCreate implements webhook.Validator
+//
+// The controller wraps this logic so it can inject extra control in the webhook.
 func (r *VirtualMachineMigration) ValidateCreate() (admission.Warnings, error) {
 	// TODO: implement creation validation webhook (?)
 	return nil, nil
 }
 
-// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
+// ValidateUpdate implements webhook.Validator
+//
+// The controller wraps this logic so it can inject extra control in the webhook.
 func (r *VirtualMachineMigration) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
 	// TODO: implement update validation webhook
 	return nil, nil
 }
 
-// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
+// ValidateDelete implements webhook.Validator
+//
+// The controller wraps this logic so it can inject extra control in the webhook.
 func (r *VirtualMachineMigration) ValidateDelete() (admission.Warnings, error) {
 	// TODO: implement deletion validation webhook (?)
 	return nil, nil

neonvm/apis/neonvm/v1/webhook_suite_test.go

@@ -99,7 +99,7 @@ var _ = BeforeSuite(func() {
 	})
 	Expect(err).NotTo(HaveOccurred())
 
-	err = (&VirtualMachine{}).SetupWebhookWithManager(mgr)
+	err = ctrl.NewWebhookManagedBy(mgr).For(&VirtualMachine{}).Complete()
 	Expect(err).NotTo(HaveOccurred())
 
 	//+kubebuilder:scaffold:webhook

neonvm/config/controller/deployment.yaml

@@ -53,6 +53,7 @@ spec:
         - "--metrics-bind-address=:8080"
         - "--leader-elect"
         - "--concurrency-limit=128"
+        - "--skip-update-validation-for="
         - "--enable-container-mgr"
         # See #775 and its links.
         # * cache.writeback=on - don't set O_DSYNC (don't flush every write)

neonvm/controllers/config.go

@@ -3,6 +3,8 @@ package controllers
 import (
 	"time"
 
+	"k8s.io/apimachinery/pkg/types"
+
 	vmv1 "github.com/neondatabase/autoscaling/neonvm/apis/neonvm/v1"
 )
 
@@ -23,6 +25,10 @@ type ReconcilerConfig struct {
 
 	MaxConcurrentReconciles int
 
+	// SkipUpdateValidationFor is the set of object names that we should ignore when doing webhook
+	// update validation.
+	SkipUpdateValidationFor map[types.NamespacedName]struct{}
+
 	// QEMUDiskCacheSettings sets the values of the 'cache.*' settings used for QEMU disks.
 	//
 	// This field is passed to neonvm-runner as the `-qemu-disk-cache-settings` arg, and is directly

neonvm/controllers/functests/vm_controller_test.go

@@ -110,6 +110,7 @@ var _ = Describe("VirtualMachine controller", func() {
 					IsK3s:                   false,
 					UseContainerMgr:         true,
 					MaxConcurrentReconciles: 1,
+					SkipUpdateValidationFor: nil,
 					QEMUDiskCacheSettings:   "cache=none",
 					DefaultMemoryProvider:   vmv1.MemoryProviderDIMMSlots,
 					MemhpAutoMovableRatio:   "301",

neonvm/controllers/metrics.go

@@ -236,7 +236,7 @@ func (d *wrappedReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	} else {
 		d.failing.RecordSuccess(req.NamespacedName)
 		d.conflicting.RecordSuccess(req.NamespacedName)
-		log.Info("Successful reconciliation", "duration", duration.String())
+		log.Info("Successful reconciliation", "duration", duration.String(), "requeueAfter", res.RequeueAfter)
 	}
 	d.Metrics.ObserveReconcileDuration(outcome, duration)
 	d.Metrics.failing.WithLabelValues(d.ControllerName,

neonvm/controllers/vm_controller.go

@@ -183,7 +183,14 @@ func (r *VMReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Re
 		}
 	}
 
-	return ctrl.Result{RequeueAfter: time.Second}, nil
+	// Only quickly requeue if we're scaling or migrating. Otherwise, we aren't expecting any
+	// changes from QEMU, and it's wasteful to repeatedly check.
+	requeueAfter := time.Second
+	if vm.Status.Phase == vmv1.VmPending || vm.Status.Phase == vmv1.VmRunning {
+		requeueAfter = 15 * time.Second
+	}
+
+	return ctrl.Result{RequeueAfter: requeueAfter}, nil
 }
 
 // doFinalizerOperationsForVirtualMachine will perform the required operations before delete the CR.

neonvm/controllers/vm_controller_unit_test.go

@@ -118,6 +118,7 @@ func newTestParams(t *testing.T) *testParams {
 			IsK3s:                   false,
 			UseContainerMgr:         false,
 			MaxConcurrentReconciles: 10,
+			SkipUpdateValidationFor: nil,
 			QEMUDiskCacheSettings:   "",
 			DefaultMemoryProvider:   vmv1.MemoryProviderDIMMSlots,
 			MemhpAutoMovableRatio:   "301",

neonvm/controllers/webhook.go

@@ -0,0 +1,151 @@
+package controllers
+
+// Wrapper around the default VirtualMachine/VirtualMachineMigration webhook interfaces so that the
+// controller has a bit more control over them, without needing to actually implement that control
+// inside of the apis package.
+
+import (
+	"context"
+	"fmt"
+
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/tools/record"
+
+	vmv1 "github.com/neondatabase/autoscaling/neonvm/apis/neonvm/v1"
+	"github.com/neondatabase/autoscaling/pkg/util/stack"
+)
+
+func validateUpdate(
+	ctx context.Context,
+	cfg *ReconcilerConfig,
+	recorder record.EventRecorder,
+	oldObj runtime.Object,
+	newObj interface {
+		webhook.Validator
+		metav1.Object
+	},
+) (admission.Warnings, error) {
+	log := log.FromContext(ctx)
+
+	namespacedName := client.ObjectKeyFromObject(newObj)
+	_, skipValidation := cfg.SkipUpdateValidationFor[namespacedName]
+
+	warnings, err := func() (w admission.Warnings, e error) {
+		// if we plan to skip validation, catch any panics so that they can be ignored.
+		if skipValidation {
+			defer func() {
+				if err := recover(); err != nil {
+					e = fmt.Errorf("validation panicked with: %v", err)
+					st := stack.GetStackTrace(nil, 1).String()
+					log.Error(e, "webhook update validation panicked", "stack", st)
+				}
+			}()
+		}
+
+		return newObj.ValidateUpdate(oldObj)
+	}()
+
+	if err != nil && skipValidation {
+		recorder.Event(
+			newObj,
+			"Warning",
+			"SkippedValidation",
+			"Ignoring failed webhook validation because of controller's '--skip-update-validation-for' flag",
+		)
+		log.Error(err, "Ignoring failed webhook validation")
+		return warnings, nil
+	}
+
+	return warnings, err
+}
+
+type VMWebhook struct {
+	Recorder record.EventRecorder
+	Config   *ReconcilerConfig
+}
+
+func (w *VMWebhook) SetupWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewWebhookManagedBy(mgr).
+		For(&vmv1.VirtualMachine{}).
+		WithDefaulter(w).
+		WithValidator(w).
+		Complete()
+}
+
+var _ webhook.CustomDefaulter = (*VMWebhook)(nil)
+
+// Default implements webhook.CustomDefaulter
+func (w *VMWebhook) Default(ctx context.Context, obj runtime.Object) error {
+	vm := obj.(*vmv1.VirtualMachine)
+	vm.Default()
+	return nil
+}
+
+var _ webhook.CustomValidator = (*VMWebhook)(nil)
+
+// ValidateCreate implements webhook.CustomValidator
+func (w *VMWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	vm := obj.(*vmv1.VirtualMachine)
+	return vm.ValidateCreate()
+}
+
+// ValidateUpdate implements webhook.CustomValidator
+func (w *VMWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
+	newVM := newObj.(*vmv1.VirtualMachine)
+	return validateUpdate(ctx, w.Config, w.Recorder, oldObj, newVM)
+}
+
+// ValidateDelete implements webhook.CustomValidator
+func (w *VMWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	vm := obj.(*vmv1.VirtualMachine)
+	return vm.ValidateDelete()
+}
+
+type VMMigrationWebhook struct {
+	Recorder record.EventRecorder
+	Config   *ReconcilerConfig
+}
+
+func (w *VMMigrationWebhook) SetupWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewWebhookManagedBy(mgr).
+		For(&vmv1.VirtualMachineMigration{}).
+		WithDefaulter(w).
+		WithValidator(w).
+		Complete()
+}
+
+var _ webhook.CustomDefaulter = (*VMWebhook)(nil)
+
+// Default implements webhook.CustomDefaulter
+func (w *VMMigrationWebhook) Default(ctx context.Context, obj runtime.Object) error {
+	vmm := obj.(*vmv1.VirtualMachineMigration)
+	vmm.Default()
+	return nil
+}
+
+var _ webhook.CustomValidator = (*VMWebhook)(nil)
+
+// ValidateCreate implements webhook.CustomValidator
+func (w *VMMigrationWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	vmm := obj.(*vmv1.VirtualMachineMigration)
+	return vmm.ValidateCreate()
+}
+
+// ValidateUpdate implements webhook.CustomValidator
+func (w *VMMigrationWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
+	newVMM := newObj.(*vmv1.VirtualMachineMigration)
+	return validateUpdate(ctx, w.Config, w.Recorder, oldObj, newVMM)
+}
+
+// ValidateDelete implements webhook.CustomValidator
+func (w *VMMigrationWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	vmm := obj.(*vmv1.VirtualMachineMigration)
+	return vmm.ValidateDelete()
+}