Merge pull request juju#13545 from ycliuhw/fix/sidecar-units-terminated

jujubot · web-flow · commit 674e9e988112 · 2021-12-07T15:44:03.000+10:00
juju#13545 We have a `foo` sidecar CAAS application containing storage and resources deployed in a model, now we remove this application with `--force` then re-deploy it again using the same application name `foo`. In `2.9`, the new units sometimes will get terminated. The root cause is Juju will schedule (will be processed 1 min later) a `forceDestroyUnit` doc for each unit when we remove the application with `--force` and the application has `storage` and `resources`. If we deploy the application `foo` again immediately after the 1st `foo` app gets removed, those new units will be removed by the previously scheduled `forceDestroyUnit` cleanup steps. So this PR ensures we cancel those scheduled `forceDestroyUnit` cleanup docs in the very last step of the application removal process if the application has already gone or there is no units to remove anymore. ## Checklist - [ ] ~Requires a [pylibjuju](https://github.com/juju/python-libjuju) change~ - [ ] ~Added [integration tests](https://github.com/juju/juju/tree/develop/tests) for the PR~ - [ ] ~Added or updated [doc.go](https://discourse.jujucharms.com/t/readme-in-packages/451) related to packages changed~ - [x] Comments answer the question of why design decisions were made ## QA steps ```console $ juju add-model t1 microk8s $ juju deploy hello-kubecon $ juju remove-application hello-kubecon --force $ juju deploy hello-kubecon # now the application and units should be settled correctly. ``` ## Documentation changes No ## Bug reference https://bugs.launchpad.net/juju/+bug/1946382
diff --git a/state/application.go b/state/application.go
@@ -515,7 +515,7 @@ func (op *DestroyApplicationOperation) destroyOps() ([]txn.Op, error) {
 		if op.app.doc.HasResources && !op.CleanupIgnoringResources {
 			if op.Force {
 				// We need to wait longer than normal for any k8s resources to be fully removed
-				// since it can take a while for the cluster to terminate rnning pods etc.
+				// since it can take a while for the cluster to terminate running pods etc.
 				logger.Debugf("scheduling forced application %q cleanup", op.app.doc.Name)
 				deadline := op.app.st.stateClock.Now().Add(2 * op.MaxWait)
 				cleanupOp := newCleanupAtOp(deadline, cleanupForceApplication, op.app.doc.Name, op.MaxWait)
@@ -714,7 +714,44 @@ func (a *Application) removeOps(asserts bson.D, op *ForcedOperation) ([]txn.Op,
 		removeModelApplicationRefOp(a.st, name),
 		removePodSpecOp(a.ApplicationTag()),
 	)
-	return ops, nil
+
+	cancelCleanupOps, err := a.cancelScheduledCleanupOps()
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	return append(ops, cancelCleanupOps...), nil
+}
+
+func (a *Application) cancelScheduledCleanupOps() ([]txn.Op, error) {
+	appOrUnitPattern := bson.DocElem{
+		Name: "prefix", Value: bson.D{
+			{Name: "$regex", Value: fmt.Sprintf("^%s(/[0-9]+)*$", a.Name())},
+		},
+	}
+	// No unit and app exists now, so cancel the below scheduled cleanup docs to avoid new resources of later deployment
+	// getting removed accidentally because we re-use unit numbers for sidecar applications.
+	cancelCleanupOpsArgs := []cancelCleanupOpsArg{
+		{cleanupForceDestroyedUnit, appOrUnitPattern},
+		{cleanupForceRemoveUnit, appOrUnitPattern},
+		{cleanupForceApplication, appOrUnitPattern},
+	}
+	relations, err := a.Relations()
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	for _, rel := range relations {
+		cancelCleanupOpsArgs = append(cancelCleanupOpsArgs, cancelCleanupOpsArg{
+			cleanupForceDestroyedRelation,
+			bson.DocElem{
+				Name: "prefix", Value: relationKey(rel.Endpoints())},
+		})
+	}
+
+	cancelCleanupOps, err := a.st.cancelCleanupOps(cancelCleanupOpsArgs...)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	return cancelCleanupOps, nil
 }
 
 // IsExposed returns whether this application is exposed. The explicitly open
diff --git a/state/cleanup.go b/state/cleanup.go
@@ -117,6 +117,36 @@ func newCleanupAtOp(when time.Time, kind cleanupKind, prefix string, args ...int
 	}
 }
 
+type cancelCleanupOpsArg struct {
+	kind    cleanupKind
+	pattern bson.DocElem
+}
+
+func (st *State) cancelCleanupOps(args ...cancelCleanupOpsArg) ([]txn.Op, error) {
+	col, closer := st.db().GetCollection(cleanupsC)
+	defer closer()
+	patterns := make([]bson.D, len(args))
+	for i, arg := range args {
+		patterns[i] = bson.D{
+			arg.pattern,
+			{Name: "kind", Value: arg.kind},
+		}
+	}
+	var docs []cleanupDoc
+	if err := col.Find(bson.D{{Name: "$or", Value: patterns}}).All(&docs); err != nil {
+		return nil, errors.Annotate(err, "cannot get cleanups docs")
+	}
+	var ops []txn.Op
+	for _, doc := range docs {
+		ops = append(ops, txn.Op{
+			C:      cleanupsC,
+			Id:     doc.DocID,
+			Remove: true,
+		})
+	}
+	return ops, nil
+}
+
 // NeedsCleanup returns true if documents previously marked for removal exist.
 func (st *State) NeedsCleanup() (bool, error) {
 	cleanups, closer := st.db().GetCollection(cleanupsC)
diff --git a/state/cleanup_test.go b/state/cleanup_test.go
@@ -614,6 +614,46 @@ func (s *CleanupSuite) TestForceDestroyMachineSchedulesRemove(c *gc.C) {
 	c.Assert(err, jc.Satisfies, errors.IsNotFound)
 }
 
+func (s *CleanupSuite) TestRemoveApplicationRemovesAllCleanUps(c *gc.C) {
+	ch := s.AddTestingCharm(c, "dummy")
+	app := s.AddTestingApplication(c, "dummy", ch)
+	unit, err := app.AddUnit(state.AddUnitParams{})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(app.Refresh(), jc.ErrorIsNil)
+	c.Assert(app.UnitCount(), gc.Equals, 1)
+	s.assertDoesNotNeedCleanup(c)
+
+	// app `dummyfoo` and its units should not be impacted after app `dummy` was destroyed.
+	appfoo := s.AddTestingApplication(c, "dummyfoo", ch)
+	unitfoo, err := appfoo.AddUnit(state.AddUnitParams{})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(appfoo.Refresh(), jc.ErrorIsNil)
+	c.Assert(appfoo.UnitCount(), gc.Equals, 1)
+	s.assertDoesNotNeedCleanup(c)
+
+	s.State.ScheduleForceCleanup(state.CleanupForceDestroyedUnit, unit.Name(), 1*time.Minute)
+	s.State.ScheduleForceCleanup(state.CleanupForceRemoveUnit, unit.Name(), 1*time.Minute)
+	s.State.ScheduleForceCleanup(state.CleanupForceApplication, app.Name(), 1*time.Minute)
+	s.assertNeedsCleanup(c)
+
+	op := app.DestroyOperation()
+	op.DestroyStorage = false
+	op.Force = true
+	op.MaxWait = 1 * time.Minute
+	err = s.State.ApplyOperation(op)
+	c.Assert(err, jc.ErrorIsNil)
+
+	s.assertNeedsCleanup(c)
+	s.assertCleanupCount(c, 3)
+
+	c.Assert(unit.Refresh(), jc.Satisfies, errors.IsNotFound)
+	c.Assert(app.Refresh(), jc.Satisfies, errors.IsNotFound)
+
+	c.Assert(unitfoo.Refresh(), jc.ErrorIsNil)
+	c.Assert(appfoo.Refresh(), jc.ErrorIsNil)
+	c.Assert(appfoo.UnitCount(), gc.Equals, 1)
+}
+
 func (s *CleanupSuite) TestForceDestroyMachineRemovesUpgradeSeriesLock(c *gc.C) {
 	machine, err := s.State.AddMachine("quantal", state.JobHostUnits)
 	c.Assert(err, jc.ErrorIsNil)
diff --git a/state/export_test.go b/state/export_test.go
@@ -1101,3 +1101,13 @@ func (st *State) SetClockForTesting(clock clock.Clock) error {
 	}
 	return nil
 }
+
+var (
+	CleanupForceDestroyedUnit = cleanupForceDestroyedUnit
+	CleanupForceRemoveUnit    = cleanupForceRemoveUnit
+	CleanupForceApplication   = cleanupForceApplication
+)
+
+func (st *State) ScheduleForceCleanup(kind cleanupKind, name string, maxWait time.Duration) {
+	st.scheduleForceCleanup(kind, name, maxWait)
+}

Original file line number	Diff line number	Diff line change
`@@ -1101,3 +1101,13 @@ func (st *State) SetClockForTesting(clock clock.Clock) error {`
`1101`	`1101`	`}`
`1102`	`1102`	`return nil`
`1103`	`1103`	`}`
	`1104`	`+`
	`1105`	`+var (`
	`1106`	`+ CleanupForceDestroyedUnit = cleanupForceDestroyedUnit`
	`1107`	`+ CleanupForceRemoveUnit = cleanupForceRemoveUnit`
	`1108`	`+ CleanupForceApplication = cleanupForceApplication`
	`1109`	`+)`
	`1110`	`+`
	`1111`	`+func (st *State) ScheduleForceCleanup(kind cleanupKind, name string, maxWait time.Duration) {`
	`1112`	`+ st.scheduleForceCleanup(kind, name, maxWait)`
	`1113`	`+}`