From 2837b29535bb85fc07b04f98a59c112b93d99fad Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Fri, 12 Apr 2024 15:13:39 +0100 Subject: [PATCH 1/2] Add a No Bastion e2e test --- Makefile | 3 +- .../kustomize/no-bastion/kustomization.yaml | 11 +++ .../no-bastion/patch-no-bastion.yaml | 3 + test/e2e/shared/common.go | 92 ++++++++++--------- test/e2e/shared/defaults.go | 1 + test/e2e/suites/e2e/e2e_test.go | 36 ++++++++ 6 files changed, 101 insertions(+), 45 deletions(-) create mode 100644 test/e2e/data/kustomize/no-bastion/kustomization.yaml create mode 100644 test/e2e/data/kustomize/no-bastion/patch-no-bastion.yaml diff --git a/Makefile b/Makefile index 1e9b096730..a0991f0ed4 100644 --- a/Makefile +++ b/Makefile @@ -161,7 +161,8 @@ e2e-templates: $(addprefix $(E2E_NO_ARTIFACT_TEMPLATES_DIR)/, \ cluster-template.yaml \ cluster-template-flatcar.yaml \ cluster-template-k8s-upgrade.yaml \ - cluster-template-flatcar-sysext.yaml) + cluster-template-flatcar-sysext.yaml \ + cluster-template-no-bastion.yaml) # Currently no templates that require CI artifacts # $(addprefix $(E2E_TEMPLATES_DIR)/, add-templates-here.yaml) \ diff --git a/test/e2e/data/kustomize/no-bastion/kustomization.yaml b/test/e2e/data/kustomize/no-bastion/kustomization.yaml new file mode 100644 index 0000000000..0c05231807 --- /dev/null +++ b/test/e2e/data/kustomize/no-bastion/kustomization.yaml @@ -0,0 +1,11 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization + +resources: +- ../default + +patches: +- path: patch-no-bastion.yaml + target: + kind: OpenStackCluster + name: \${CLUSTER_NAME} diff --git a/test/e2e/data/kustomize/no-bastion/patch-no-bastion.yaml b/test/e2e/data/kustomize/no-bastion/patch-no-bastion.yaml new file mode 100644 index 0000000000..15704bc138 --- /dev/null +++ b/test/e2e/data/kustomize/no-bastion/patch-no-bastion.yaml @@ -0,0 +1,3 @@ +--- +- op: remove + path: /spec/bastion diff --git a/test/e2e/shared/common.go b/test/e2e/shared/common.go index b8ad587b90..8bef9e9964 100644 --- a/test/e2e/shared/common.go +++ b/test/e2e/shared/common.go @@ -214,51 +214,55 @@ func (o OpenStackLogCollector) CollectMachineLog(ctx context.Context, management return fmt.Errorf("error writing server JSON %s: %s", serverJSON, err) } - srvUser := o.E2EContext.E2EConfig.GetVariable(SSHUserMachine) - executeCommands( - ctx, - o.E2EContext.Settings.ArtifactFolder, - o.E2EContext.Settings.Debug, - outputPath, - ip, - openStackCluster.Status.Bastion.FloatingIP, - srvUser, - []command{ - // don't do this for now, it just takes to long - // { - // title: "systemd", - // cmd: "journalctl --no-pager --output=short-precise | grep -v 'audit:\\|audit\\['", - // }, - { - title: "kern", - cmd: "journalctl --no-pager --output=short-precise -k", + if openStackCluster.Status.Bastion == nil { + Logf("Skipping log collection for machine %q since no bastion is available", m.Name) + } else { + srvUser := o.E2EContext.E2EConfig.GetVariable(SSHUserMachine) + executeCommands( + ctx, + o.E2EContext.Settings.ArtifactFolder, + o.E2EContext.Settings.Debug, + outputPath, + ip, + openStackCluster.Status.Bastion.FloatingIP, + srvUser, + []command{ + // don't do this for now, it just takes to long + // { + // title: "systemd", + // cmd: "journalctl --no-pager --output=short-precise | grep -v 'audit:\\|audit\\['", + // }, + { + title: "kern", + cmd: "journalctl --no-pager --output=short-precise -k", + }, + { + title: "containerd-info", + cmd: "crictl --runtime-endpoint unix:///run/containerd/containerd.sock info", + }, + { + title: "containerd-containers", + cmd: "crictl --runtime-endpoint unix:///run/containerd/containerd.sock ps", + }, + { + title: "containerd-pods", + cmd: "crictl --runtime-endpoint unix:///run/containerd/containerd.sock pods", + }, + { + title: "cloud-final", + cmd: "journalctl --no-pager -u cloud-final", + }, + { + title: "kubelet", + cmd: "journalctl --no-pager -u kubelet.service", + }, + { + title: "containerd", + cmd: "journalctl --no-pager -u containerd.service", + }, }, - { - title: "containerd-info", - cmd: "crictl --runtime-endpoint unix:///run/containerd/containerd.sock info", - }, - { - title: "containerd-containers", - cmd: "crictl --runtime-endpoint unix:///run/containerd/containerd.sock ps", - }, - { - title: "containerd-pods", - cmd: "crictl --runtime-endpoint unix:///run/containerd/containerd.sock pods", - }, - { - title: "cloud-final", - cmd: "journalctl --no-pager -u cloud-final", - }, - { - title: "kubelet", - cmd: "journalctl --no-pager -u kubelet.service", - }, - { - title: "containerd", - cmd: "journalctl --no-pager -u containerd.service", - }, - }, - ) + ) + } return nil } diff --git a/test/e2e/shared/defaults.go b/test/e2e/shared/defaults.go index 3c0f32c56e..5caa6eb8e1 100644 --- a/test/e2e/shared/defaults.go +++ b/test/e2e/shared/defaults.go @@ -46,6 +46,7 @@ const ( OpenStackNodeMachineFlavor = "OPENSTACK_NODE_MACHINE_FLAVOR" SSHUserMachine = "SSH_USER_MACHINE" FlavorDefault = "" + FlavorNoBastion = "no-bastion" FlavorWithoutLB = "without-lb" FlavorMultiNetwork = "multi-network" FlavorMultiAZ = "multi-az" diff --git a/test/e2e/suites/e2e/e2e_test.go b/test/e2e/suites/e2e/e2e_test.go index 09ee609346..45423af394 100644 --- a/test/e2e/suites/e2e/e2e_test.go +++ b/test/e2e/suites/e2e/e2e_test.go @@ -245,6 +245,42 @@ var _ = Describe("e2e tests [PR-Blocking]", func() { }) }) + Describe("Workload cluster (no bastion)", func() { + It("should be creatable and deletable", func() { + shared.Logf("Creating a cluster") + clusterName := fmt.Sprintf("cluster-%s", namespace.Name) + configCluster := defaultConfigCluster(clusterName, namespace.Name) + configCluster.ControlPlaneMachineCount = ptr.To(int64(1)) + configCluster.WorkerMachineCount = ptr.To(int64(1)) + configCluster.Flavor = shared.FlavorNoBastion + createCluster(ctx, configCluster, clusterResources) + md := clusterResources.MachineDeployments + + workerMachines := framework.GetMachinesByMachineDeployments(ctx, framework.GetMachinesByMachineDeploymentsInput{ + Lister: e2eCtx.Environment.BootstrapClusterProxy.GetClient(), + ClusterName: clusterName, + Namespace: namespace.Name, + MachineDeployment: *md[0], + }) + controlPlaneMachines := framework.GetControlPlaneMachinesByCluster(ctx, framework.GetControlPlaneMachinesByClusterInput{ + Lister: e2eCtx.Environment.BootstrapClusterProxy.GetClient(), + ClusterName: clusterName, + Namespace: namespace.Name, + }) + Expect(workerMachines).To(HaveLen(1)) + Expect(controlPlaneMachines).To(HaveLen(1)) + + shared.Logf("Waiting for worker nodes to be in Running phase") + statusChecks := []framework.MachineStatusCheck{framework.MachinePhaseCheck(string(clusterv1.MachinePhaseRunning))} + machineStatusInput := framework.WaitForMachineStatusCheckInput{ + Getter: e2eCtx.Environment.BootstrapClusterProxy.GetClient(), + Machine: &workerMachines[0], + StatusChecks: statusChecks, + } + framework.WaitForMachineStatusCheck(ctx, machineStatusInput, e2eCtx.E2EConfig.GetIntervals(specName, "wait-machine-status")...) + }) + }) + Describe("Workload cluster (flatcar)", func() { It("should be creatable and deletable", func() { // Flatcar default user is "core" From 118f715ca3544f20798d5e68c29c539751e85fa6 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Fri, 12 Apr 2024 15:14:07 +0100 Subject: [PATCH 2/2] Fix crash on delete with no bastion --- controllers/openstackcluster_controller.go | 14 ++++++-------- controllers/openstackmachine_controller.go | 15 ++++----------- pkg/cloud/services/compute/instance.go | 15 +++++++++------ 3 files changed, 19 insertions(+), 25 deletions(-) diff --git a/controllers/openstackcluster_controller.go b/controllers/openstackcluster_controller.go index 7ba565cb90..d310aced6a 100644 --- a/controllers/openstackcluster_controller.go +++ b/controllers/openstackcluster_controller.go @@ -291,15 +291,13 @@ func deleteBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStac } // If no instance was created we currently need to check for orphaned - // volumes. This requires resolving the instance spec. - // TODO: write volumes to status resources on creation so this is no longer required. + // volumes. if instanceStatus == nil { - instanceSpec, err := bastionToInstanceSpec(openStackCluster, cluster) - if err != nil { - return err - } - if err := computeService.DeleteVolumes(instanceSpec); err != nil { - return fmt.Errorf("delete volumes: %w", err) + bastion := openStackCluster.Spec.Bastion + if bastion != nil && bastion.Spec != nil { + if err := computeService.DeleteVolumes(bastionName(cluster.Name), bastion.Spec.RootVolume, bastion.Spec.AdditionalBlockDevices); err != nil { + return fmt.Errorf("delete volumes: %w", err) + } } } else { instanceNS, err := instanceStatus.NetworkStatus() diff --git a/controllers/openstackmachine_controller.go b/controllers/openstackmachine_controller.go index a1869e3b51..ee923ec812 100644 --- a/controllers/openstackmachine_controller.go +++ b/controllers/openstackmachine_controller.go @@ -300,19 +300,12 @@ func (r *OpenStackMachineReconciler) reconcileDelete(scope *scope.WithLogger, cl } // If no instance was created we currently need to check for orphaned - // volumes. This requires resolving the instance spec. - // TODO: write volumes to status resources on creation so this is no longer required. - if instanceStatus == nil && openStackMachine.Status.Resolved != nil { - instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, "") - if err != nil { - return ctrl.Result{}, err - } - if err := computeService.DeleteVolumes(instanceSpec); err != nil { + // volumes. + if instanceStatus == nil { + if err := computeService.DeleteVolumes(openStackMachine.Name, openStackMachine.Spec.RootVolume, openStackMachine.Spec.AdditionalBlockDevices); err != nil { return ctrl.Result{}, fmt.Errorf("delete volumes: %w", err) } - } - - if instanceStatus != nil { + } else { if err := computeService.DeleteInstance(openStackMachine, instanceStatus); err != nil { conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeleteFailedReason, clusterv1.ConditionSeverityError, "Deleting instance failed: %v", err) return ctrl.Result{}, fmt.Errorf("delete instance: %w", err) diff --git a/pkg/cloud/services/compute/instance.go b/pkg/cloud/services/compute/instance.go index cb7146ca4d..aa8d6e5ca2 100644 --- a/pkg/cloud/services/compute/instance.go +++ b/pkg/cloud/services/compute/instance.go @@ -431,11 +431,14 @@ func (s *Service) DeleteInstance(eventObject runtime.Object, instanceStatus *Ins } // DeleteVolumes deletes any cinder volumes which were created for the instance. -// Note that this must only be called when the server was not successfully +// Note that this need only be called when the server was not successfully // created. If the server was created the volume will have been added with // DeleteOnTermination=true, and will be automatically cleaned up with the // server. -func (s *Service) DeleteVolumes(instanceSpec *InstanceSpec) error { +// We don't pass InstanceSpec here because we only require instance name, +// rootVolume, and additionalBlockDevices, and resolving the whole InstanceSpec +// introduces unnecessary failure modes. +func (s *Service) DeleteVolumes(instanceName string, rootVolume *infrav1.RootVolume, additionalBlockDevices []infrav1.AdditionalBlockDevice) error { /* Attaching volumes to an instance is a two-step process: @@ -455,13 +458,13 @@ func (s *Service) DeleteVolumes(instanceSpec *InstanceSpec) error { DeleteOnTermination will ensure it is deleted in that case. */ - if hasRootVolume(instanceSpec) { - if err := s.deleteVolume(instanceSpec.Name, "root"); err != nil { + if rootVolume != nil && rootVolume.SizeGiB > 0 { + if err := s.deleteVolume(instanceName, "root"); err != nil { return err } } - for _, volumeSpec := range instanceSpec.AdditionalBlockDevices { - if err := s.deleteVolume(instanceSpec.Name, volumeSpec.Name); err != nil { + for _, volumeSpec := range additionalBlockDevices { + if err := s.deleteVolume(instanceName, volumeSpec.Name); err != nil { return err } }