Skip to content

Commit f4e5bdf

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #2017 from shiftstack/issue2016
🐛 Fix crash on delete with no bastion
2 parents 4cf3c1b + 118f715 commit f4e5bdf

File tree

9 files changed

+120
-70
lines changed

9 files changed

+120
-70
lines changed

‎Makefile‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,8 @@ e2e-templates: $(addprefix $(E2E_NO_ARTIFACT_TEMPLATES_DIR)/, \
161161
cluster-template.yaml \
162162
cluster-template-flatcar.yaml \
163163
cluster-template-k8s-upgrade.yaml \
164-
cluster-template-flatcar-sysext.yaml)
164+
cluster-template-flatcar-sysext.yaml \
165+
cluster-template-no-bastion.yaml)
165166
# Currently no templates that require CI artifacts
166167
# $(addprefix $(E2E_TEMPLATES_DIR)/, add-templates-here.yaml) \
167168

‎controllers/openstackcluster_controller.go‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -291,15 +291,13 @@ func deleteBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStac
291291
}
292292

293293
// If no instance was created we currently need to check for orphaned
294-
// volumes. This requires resolving the instance spec.
295-
// TODO: write volumes to status resources on creation so this is no longer required.
294+
// volumes.
296295
if instanceStatus == nil {
297-
instanceSpec, err := bastionToInstanceSpec(openStackCluster, cluster)
298-
if err != nil {
299-
return err
300-
}
301-
if err := computeService.DeleteVolumes(instanceSpec); err != nil {
302-
return fmt.Errorf("delete volumes: %w", err)
296+
bastion := openStackCluster.Spec.Bastion
297+
if bastion != nil && bastion.Spec != nil {
298+
if err := computeService.DeleteVolumes(bastionName(cluster.Name), bastion.Spec.RootVolume, bastion.Spec.AdditionalBlockDevices); err != nil {
299+
return fmt.Errorf("delete volumes: %w", err)
300+
}
303301
}
304302
} else {
305303
instanceNS, err := instanceStatus.NetworkStatus()

‎controllers/openstackmachine_controller.go‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -300,19 +300,12 @@ func (r *OpenStackMachineReconciler) reconcileDelete(scope *scope.WithLogger, cl
300300
}
301301

302302
// If no instance was created we currently need to check for orphaned
303-
// volumes. This requires resolving the instance spec.
304-
// TODO: write volumes to status resources on creation so this is no longer required.
305-
if instanceStatus == nil && openStackMachine.Status.Resolved != nil {
306-
instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, "")
307-
if err != nil {
308-
return ctrl.Result{}, err
309-
}
310-
if err := computeService.DeleteVolumes(instanceSpec); err != nil {
303+
// volumes.
304+
if instanceStatus == nil {
305+
if err := computeService.DeleteVolumes(openStackMachine.Name, openStackMachine.Spec.RootVolume, openStackMachine.Spec.AdditionalBlockDevices); err != nil {
311306
return ctrl.Result{}, fmt.Errorf("delete volumes: %w", err)
312307
}
313-
}
314-
315-
if instanceStatus != nil {
308+
} else {
316309
if err := computeService.DeleteInstance(openStackMachine, instanceStatus); err != nil {
317310
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeleteFailedReason, clusterv1.ConditionSeverityError, "Deleting instance failed: %v", err)
318311
return ctrl.Result{}, fmt.Errorf("delete instance: %w", err)

‎pkg/cloud/services/compute/instance.go‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -431,11 +431,14 @@ func (s *Service) DeleteInstance(eventObject runtime.Object, instanceStatus *Ins
431431
}
432432

433433
// DeleteVolumes deletes any cinder volumes which were created for the instance.
434-
// Note that this must only be called when the server was not successfully
434+
// Note that this need only be called when the server was not successfully
435435
// created. If the server was created the volume will have been added with
436436
// DeleteOnTermination=true, and will be automatically cleaned up with the
437437
// server.
438-
func (s *Service) DeleteVolumes(instanceSpec *InstanceSpec) error {
438+
// We don't pass InstanceSpec here because we only require instance name,
439+
// rootVolume, and additionalBlockDevices, and resolving the whole InstanceSpec
440+
// introduces unnecessary failure modes.
441+
func (s *Service) DeleteVolumes(instanceName string, rootVolume *infrav1.RootVolume, additionalBlockDevices []infrav1.AdditionalBlockDevice) error {
439442
/*
440443
Attaching volumes to an instance is a two-step process:
441444
@@ -455,13 +458,13 @@ func (s *Service) DeleteVolumes(instanceSpec *InstanceSpec) error {
455458
DeleteOnTermination will ensure it is deleted in that case.
456459
*/
457460

458-
if hasRootVolume(instanceSpec) {
459-
if err := s.deleteVolume(instanceSpec.Name, "root"); err != nil {
461+
if rootVolume != nil && rootVolume.SizeGiB > 0 {
462+
if err := s.deleteVolume(instanceName, "root"); err != nil {
460463
return err
461464
}
462465
}
463-
for _, volumeSpec := range instanceSpec.AdditionalBlockDevices {
464-
if err := s.deleteVolume(instanceSpec.Name, volumeSpec.Name); err != nil {
466+
for _, volumeSpec := range additionalBlockDevices {
467+
if err := s.deleteVolume(instanceName, volumeSpec.Name); err != nil {
465468
return err
466469
}
467470
}

‎test/e2e/data/kustomize/no-bastion/kustomization.yaml‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
apiVersion: kustomize.config.k8s.io/v1beta1
2+
kind: Kustomization
3+
4+
resources:
5+
- ../default
6+
7+
patches:
8+
- path: patch-no-bastion.yaml
9+
target:
10+
kind: OpenStackCluster
11+
name: \${CLUSTER_NAME}

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
---
2+
- op: remove
3+
path: /spec/bastion

‎test/e2e/shared/common.go‎

Lines changed: 48 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -214,51 +214,55 @@ func (o OpenStackLogCollector) CollectMachineLog(ctx context.Context, management
214214
return fmt.Errorf("error writing server JSON %s: %s", serverJSON, err)
215215
}
216216

217-
srvUser := o.E2EContext.E2EConfig.GetVariable(SSHUserMachine)
218-
executeCommands(
219-
ctx,
220-
o.E2EContext.Settings.ArtifactFolder,
221-
o.E2EContext.Settings.Debug,
222-
outputPath,
223-
ip,
224-
openStackCluster.Status.Bastion.FloatingIP,
225-
srvUser,
226-
[]command{
227-
// don't do this for now, it just takes to long
228-
// {
229-
// title: "systemd",
230-
// cmd: "journalctl --no-pager --output=short-precise | grep -v 'audit:\\|audit\\['",
231-
// },
232-
{
233-
title: "kern",
234-
cmd: "journalctl --no-pager --output=short-precise -k",
217+
if openStackCluster.Status.Bastion == nil {
218+
Logf("Skipping log collection for machine %q since no bastion is available", m.Name)
219+
} else {
220+
srvUser := o.E2EContext.E2EConfig.GetVariable(SSHUserMachine)
221+
executeCommands(
222+
ctx,
223+
o.E2EContext.Settings.ArtifactFolder,
224+
o.E2EContext.Settings.Debug,
225+
outputPath,
226+
ip,
227+
openStackCluster.Status.Bastion.FloatingIP,
228+
srvUser,
229+
[]command{
230+
// don't do this for now, it just takes to long
231+
// {
232+
// title: "systemd",
233+
// cmd: "journalctl --no-pager --output=short-precise | grep -v 'audit:\\|audit\\['",
234+
// },
235+
{
236+
title: "kern",
237+
cmd: "journalctl --no-pager --output=short-precise -k",
238+
},
239+
{
240+
title: "containerd-info",
241+
cmd: "crictl --runtime-endpoint unix:///run/containerd/containerd.sock info",
242+
},
243+
{
244+
title: "containerd-containers",
245+
cmd: "crictl --runtime-endpoint unix:///run/containerd/containerd.sock ps",
246+
},
247+
{
248+
title: "containerd-pods",
249+
cmd: "crictl --runtime-endpoint unix:///run/containerd/containerd.sock pods",
250+
},
251+
{
252+
title: "cloud-final",
253+
cmd: "journalctl --no-pager -u cloud-final",
254+
},
255+
{
256+
title: "kubelet",
257+
cmd: "journalctl --no-pager -u kubelet.service",
258+
},
259+
{
260+
title: "containerd",
261+
cmd: "journalctl --no-pager -u containerd.service",
262+
},
235263
},
236-
{
237-
title: "containerd-info",
238-
cmd: "crictl --runtime-endpoint unix:///run/containerd/containerd.sock info",
239-
},
240-
{
241-
title: "containerd-containers",
242-
cmd: "crictl --runtime-endpoint unix:///run/containerd/containerd.sock ps",
243-
},
244-
{
245-
title: "containerd-pods",
246-
cmd: "crictl --runtime-endpoint unix:///run/containerd/containerd.sock pods",
247-
},
248-
{
249-
title: "cloud-final",
250-
cmd: "journalctl --no-pager -u cloud-final",
251-
},
252-
{
253-
title: "kubelet",
254-
cmd: "journalctl --no-pager -u kubelet.service",
255-
},
256-
{
257-
title: "containerd",
258-
cmd: "journalctl --no-pager -u containerd.service",
259-
},
260-
},
261-
)
264+
)
265+
}
262266
return nil
263267
}
264268

‎test/e2e/shared/defaults.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ const (
4646
OpenStackNodeMachineFlavor = "OPENSTACK_NODE_MACHINE_FLAVOR"
4747
SSHUserMachine = "SSH_USER_MACHINE"
4848
FlavorDefault = ""
49+
FlavorNoBastion = "no-bastion"
4950
FlavorWithoutLB = "without-lb"
5051
FlavorMultiNetwork = "multi-network"
5152
FlavorMultiAZ = "multi-az"

‎test/e2e/suites/e2e/e2e_test.go‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,42 @@ var _ = Describe("e2e tests [PR-Blocking]", func() {
245245
})
246246
})
247247

248+
Describe("Workload cluster (no bastion)", func() {
249+
It("should be creatable and deletable", func() {
250+
shared.Logf("Creating a cluster")
251+
clusterName := fmt.Sprintf("cluster-%s", namespace.Name)
252+
configCluster := defaultConfigCluster(clusterName, namespace.Name)
253+
configCluster.ControlPlaneMachineCount = ptr.To(int64(1))
254+
configCluster.WorkerMachineCount = ptr.To(int64(1))
255+
configCluster.Flavor = shared.FlavorNoBastion
256+
createCluster(ctx, configCluster, clusterResources)
257+
md := clusterResources.MachineDeployments
258+
259+
workerMachines := framework.GetMachinesByMachineDeployments(ctx, framework.GetMachinesByMachineDeploymentsInput{
260+
Lister: e2eCtx.Environment.BootstrapClusterProxy.GetClient(),
261+
ClusterName: clusterName,
262+
Namespace: namespace.Name,
263+
MachineDeployment: *md[0],
264+
})
265+
controlPlaneMachines := framework.GetControlPlaneMachinesByCluster(ctx, framework.GetControlPlaneMachinesByClusterInput{
266+
Lister: e2eCtx.Environment.BootstrapClusterProxy.GetClient(),
267+
ClusterName: clusterName,
268+
Namespace: namespace.Name,
269+
})
270+
Expect(workerMachines).To(HaveLen(1))
271+
Expect(controlPlaneMachines).To(HaveLen(1))
272+
273+
shared.Logf("Waiting for worker nodes to be in Running phase")
274+
statusChecks := []framework.MachineStatusCheck{framework.MachinePhaseCheck(string(clusterv1.MachinePhaseRunning))}
275+
machineStatusInput := framework.WaitForMachineStatusCheckInput{
276+
Getter: e2eCtx.Environment.BootstrapClusterProxy.GetClient(),
277+
Machine: &workerMachines[0],
278+
StatusChecks: statusChecks,
279+
}
280+
framework.WaitForMachineStatusCheck(ctx, machineStatusInput, e2eCtx.E2EConfig.GetIntervals(specName, "wait-machine-status")...)
281+
})
282+
})
283+
248284
Describe("Workload cluster (flatcar)", func() {
249285
It("should be creatable and deletable", func() {
250286
// Flatcar default user is "core"

0 commit comments

Comments
 (0)