diff --git a/internal/controller/onboarding_controller.go b/internal/controller/onboarding_controller.go index f94a738f..aa923e90 100644 --- a/internal/controller/onboarding_controller.go +++ b/internal/controller/onboarding_controller.go @@ -24,6 +24,7 @@ import ( "net/http" "strings" "time" + "uuid" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" @@ -37,6 +38,7 @@ import ( logger "sigs.k8s.io/controller-runtime/pkg/log" "github.com/gophercloud/gophercloud/v2" + "github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes" "github.com/gophercloud/gophercloud/v2/openstack/compute/v2/hypervisors" "github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers" "github.com/gophercloud/gophercloud/v2/openstack/compute/v2/services" @@ -72,6 +74,7 @@ type OnboardingController struct { testComputeClient *gophercloud.ServiceClient testImageClient *gophercloud.ServiceClient testNetworkClient *gophercloud.ServiceClient + testVolumeClient *gophercloud.ServiceClient requeueInterval time.Duration } @@ -284,6 +287,12 @@ func (r *OnboardingController) smokeTest(ctx context.Context, hv *kvmv1.Hypervis log.Error(err, "failed to delete test instance", "id", id) } + // Aborted builds can leak the boot volume; delete it so the test + // project does not exhaust its volume quota. + if err := r.deleteOrphanedTestVolumes(ctx); err != nil { + log.Error(err, "failed to delete orphaned test volumes") + } + return ctrl.Result{RequeueAfter: defaultWaitTime}, nil case "ACTIVE": @@ -401,6 +410,57 @@ func (r *OnboardingController) deleteTestServers(ctx context.Context, host strin return errors.Join(errs...) } +// deleteOrphanedTestVolumes deletes unattached boot volumes left behind by +// aborted test server builds. Nova names boot volumes after the instance +// UUID, so a volume is only deleted when it is unattached and no instance +// with that ID exists in the project anymore. +func (r *OnboardingController) deleteOrphanedTestVolumes(ctx context.Context) error { + log := logger.FromContext(ctx) + + serverPages, err := servers.ListSimple(r.testComputeClient, servers.ListOpts{}).AllPages(ctx) + if err != nil { + return err + } + serverList, err := servers.ExtractServers(serverPages) + if err != nil { + return err + } + existingInstances := make(map[string]struct{}, len(serverList)) + for _, server := range serverList { + existingInstances[server.ID] = struct{}{} + } + + volumePages, err := volumes.List(r.testVolumeClient, volumes.ListOpts{}).AllPages(ctx) + if err != nil { + return err + } + volumeList, err := volumes.ExtractVolumes(volumePages) + if err != nil { + return err + } + + var errs []error + for _, volume := range volumeList { + if volume.Status != "available" { + continue + } + if _, err := uuid.Parse(volume.Name); err != nil { + continue + } + if _, exists := existingInstances[volume.Name]; exists { + continue + } + log.Info("deleting orphaned test volume", "id", volume.ID, "name", volume.Name) + if err := volumes.Delete(ctx, r.testVolumeClient, volume.ID, nil).ExtractErr(); err != nil { + if !gophercloud.ResponseCodeIs(err, http.StatusNotFound) { + errs = append(errs, err) + } + } + } + + return errors.Join(errs...) +} + func (r *OnboardingController) lookupNovaProperties(ctx context.Context, hv *kvmv1.Hypervisor) (hypervisorID, serviceID string, err error) { hypervisorAddress := hv.Labels[corev1.LabelHostname] if hypervisorAddress == "" { @@ -478,6 +538,12 @@ func (r *OnboardingController) createOrGetTestServer(ctx context.Context, zone, return foundServer, nil } + // Aborted builds can leak boot volumes, which eventually exhausts the + // test project's volume quota. + if err := r.deleteOrphanedTestVolumes(ctx); err != nil { + log.Error(err, "failed to delete orphaned test volumes") + } + flavorRef := r.TestFlavorID imageRef, err := r.findTestImage(ctx) @@ -605,6 +671,10 @@ func (r *OnboardingController) SetupWithManager(mgr ctrl.Manager) error { } r.testNetworkClient.ResourceBase = fmt.Sprintf("%vv2.0/", r.testNetworkClient.Endpoint) + if r.testVolumeClient, err = openstack.GetServiceClient(ctx, "volumev3", testAuth); err != nil { + return err + } + if r.Clock == nil { r.Clock = clock.RealClock{} } diff --git a/internal/controller/onboarding_controller_test.go b/internal/controller/onboarding_controller_test.go index b1d8b347..13349dae 100644 --- a/internal/controller/onboarding_controller_test.go +++ b/internal/controller/onboarding_controller_test.go @@ -227,6 +227,7 @@ var _ = Describe("Onboarding Controller", func() { testComputeClient: client.ServiceClient(fakeServer), testImageClient: client.ServiceClient(fakeServer), testNetworkClient: client.ServiceClient(fakeServer), + testVolumeClient: client.ServiceClient(fakeServer), } DeferCleanup(func() { @@ -412,6 +413,8 @@ var _ = Describe("Onboarding Controller", func() { serverActionHandler func(http.ResponseWriter, *http.Request) serverDeleteHandler func(http.ResponseWriter, *http.Request) serverDetailHandler func(http.ResponseWriter, *http.Request) + serverListHandler func(http.ResponseWriter, *http.Request) + volumeListHandler func(http.ResponseWriter, *http.Request) ) BeforeEach(func(ctx SpecContext) { @@ -433,11 +436,14 @@ var _ = Describe("Onboarding Controller", func() { Expect(err).NotTo(HaveOccurred()) }) - fakeServer.Mux.HandleFunc("GET /servers", func(w http.ResponseWriter, r *http.Request) { + serverListHandler = func(w http.ResponseWriter, _ *http.Request) { w.Header().Add("Content-Type", "application/json") w.WriteHeader(http.StatusOK) _, err := fmt.Fprint(w, emptyServersBody) Expect(err).NotTo(HaveOccurred()) + } + fakeServer.Mux.HandleFunc("GET /servers", func(w http.ResponseWriter, r *http.Request) { + serverListHandler(w, r) }) serverDetailHandler = func(w http.ResponseWriter, _ *http.Request) { @@ -471,6 +477,16 @@ var _ = Describe("Onboarding Controller", func() { Expect(err).NotTo(HaveOccurred()) }) + volumeListHandler = func(w http.ResponseWriter, _ *http.Request) { + w.Header().Add("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, err := fmt.Fprint(w, `{"volumes": []}`) + Expect(err).NotTo(HaveOccurred()) + } + fakeServer.Mux.HandleFunc("GET /volumes/detail", func(w http.ResponseWriter, r *http.Request) { + volumeListHandler(w, r) + }) + fakeServer.Mux.HandleFunc("POST /servers", func(w http.ResponseWriter, r *http.Request) { w.Header().Add("Content-Type", "application/json") w.WriteHeader(http.StatusOK) @@ -716,5 +732,105 @@ var _ = Describe("Onboarding Controller", func() { }) }) + When("the test server ended up in an error state", func() { + var ( + serverDeleteCalls int + orphanedVolumeDeleteCalls int + liveVolumeDeleteCalls int + inUseVolumeDeleteCalls int + ) + + BeforeEach(func(ctx SpecContext) { + By("Overriding HV status to Testing state") + hv := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed()) + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeOnboarding, + Status: metav1.ConditionTrue, + Reason: kvmv1.ConditionReasonTesting, + }) + Expect(k8sClient.Status().Update(ctx, hv)).To(Succeed()) + + // Construct the server name the controller will look for. + serverName := fmt.Sprintf("%s-%s-%s", testPrefixName, hypervisorName, string(hv.UID)) + + // GET /servers/detail returns the test server in ERROR state. + serverDetailHandler = func(w http.ResponseWriter, _ *http.Request) { + _, err := fmt.Fprintf(w, + `{"servers": [{"id": "server-id", "name": %q, "status": "ERROR"}], "servers_links": []}`, + serverName) + Expect(err).NotTo(HaveOccurred()) + } + + // The unfiltered server list holds an unrelated live instance + // whose boot volume must not be deleted. + serverListHandler = func(w http.ResponseWriter, _ *http.Request) { + w.Header().Add("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, err := fmt.Fprint(w, `{"servers": [{"id": "c48f6247-abe4-4a24-824e-e820f-afd1c2bc7521", "name": "live-instance", "status": "ACTIVE"}]}`) + Expect(err).NotTo(HaveOccurred()) + } + + fakeServer.Mux.HandleFunc("GET /servers/server-id", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Add("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, err := fmt.Fprint(w, `{"server": {"id": "server-id", "name": "server-id", "status": "ERROR", "fault": {"message": "Build of instance 77b7129b-aecb-4d6e-820f-afd1c2bc7521 aborted: VolumeLimitExceeded: Maximum number of volumes allowed (100) exceeded for quota 'volumes_premium'."}}}`) + Expect(err).NotTo(HaveOccurred()) + }) + + serverDeleteHandler = func(w http.ResponseWriter, _ *http.Request) { + serverDeleteCalls++ + w.WriteHeader(http.StatusAccepted) + } + + // Three volumes: an orphaned boot volume (instance gone), the + // boot volume of the live instance (instance exists), and an + // in-use volume (attached). + volumeListHandler = func(w http.ResponseWriter, _ *http.Request) { + w.Header().Add("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, err := fmt.Fprint(w, `{"volumes": [ + {"id": "orphaned-volume-id", "name": "77b7129b-aecb-4d6e-820f-afd1c2bc7521", "status": "available"}, + {"id": "live-instance-volume-id", "name": "c48f6247-abe4-4a24-824e-e820f-afd1c2bc7521", "status": "available"}, + {"id": "in-use-volume-id", "name": "in-use-volume-id", "status": "in-use"} + ]}`) + Expect(err).NotTo(HaveOccurred()) + } + fakeServer.Mux.HandleFunc("DELETE /volumes/orphaned-volume-id", func(w http.ResponseWriter, _ *http.Request) { + orphanedVolumeDeleteCalls++ + w.WriteHeader(http.StatusAccepted) + }) + fakeServer.Mux.HandleFunc("DELETE /volumes/live-instance-volume-id", func(w http.ResponseWriter, _ *http.Request) { + liveVolumeDeleteCalls++ + w.WriteHeader(http.StatusAccepted) + }) + fakeServer.Mux.HandleFunc("DELETE /volumes/in-use-volume-id", func(w http.ResponseWriter, _ *http.Request) { + inUseVolumeDeleteCalls++ + w.WriteHeader(http.StatusAccepted) + }) + }) + + It("should delete the server and the orphaned volume and record the fault", func(ctx SpecContext) { + _, err := onboardingReconciler.Reconcile(ctx, reconcileReq) + Expect(err).NotTo(HaveOccurred()) + + By("Verifying the failed server was deleted") + Expect(serverDeleteCalls).To(Equal(1)) + + By("Verifying only the orphaned volume was deleted") + Expect(orphanedVolumeDeleteCalls).To(Equal(1)) + Expect(liveVolumeDeleteCalls).To(BeZero()) + Expect(inUseVolumeDeleteCalls).To(BeZero()) + + By("Verifying the onboarding condition records the fault") + hv := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed()) + onboardingCond := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeOnboarding) + Expect(onboardingCond).NotTo(BeNil()) + Expect(onboardingCond.Reason).To(Equal(kvmv1.ConditionReasonTesting)) + Expect(onboardingCond.Message).To(ContainSubstring("VolumeLimitExceeded")) + }) + }) + }) })