Merge pull request #2017 from shiftstack/issue2016

🐛 Fix crash on delete with no bastion

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) \
 

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()

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)

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
 		}
 	}

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}

test/e2e/data/kustomize/no-bastion/patch-no-bastion.yaml

@@ -0,0 +1,3 @@
+---
+- op: remove
+  path: /spec/bastion

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
 }
 

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"

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"