-
Notifications
You must be signed in to change notification settings - Fork 492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
worker/undertaker: don't ignore all errors #7761
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,8 +11,13 @@ import ( | |
|
||
var ErrModelNotDying = errors.New("model is not dying") | ||
|
||
// ProcessDyingModel checks if there are any machines or services left in | ||
// state. If there are none, the model's life is changed from dying to dead. | ||
// ProcessDyingModel checks if the model is Dying and empty, and if so, | ||
// transitions the model to Dead. | ||
// | ||
// If the model is non-empty because it is the controller model and still | ||
// contains hosted models, an error satisfying IsHasHostedModelsError will | ||
// be returned. If the model is otherwise non-empty, an error satisfying | ||
// IsNonEmptyModelError will be returned. | ||
func (st *State) ProcessDyingModel() (err error) { | ||
buildTxn := func(attempt int) ([]txn.Op, error) { | ||
model, err := st.Model() | ||
|
@@ -25,14 +30,15 @@ func (st *State) ProcessDyingModel() (err error) { | |
} | ||
|
||
if st.IsController() { | ||
// We should not mark the controller model as Dead until | ||
// all hosted models have been removed, otherwise the | ||
// hosted model environs may not have beeen destroyed. | ||
models, err := st.AllModels() | ||
if err != nil { | ||
return nil, errors.Trace(err) | ||
} | ||
for _, model := range models { | ||
if model.UUID() != st.ModelUUID() && model.Life() != Dead { | ||
return nil, errors.Errorf("one or more hosted models are not yet dead") | ||
} | ||
if n := len(models) - 1; n > 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AllModels() call above will return even those models that are already Dead. Do we want them to be part of this count? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, per the comment I added: we shouldn't be able to mark the controller model as Dead until all hosted models are removed |
||
return nil, errors.Trace(hasHostedModelsError(n)) | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice usability consideration :D