Skip to content
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.

Sign up for GitHub

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

Fix cell iteration order #202

Merged
merged 1 commit into from
Dec 16, 2022
Merged

Fix cell iteration order #202

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions controllers/nova_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,14 +188,25 @@ func (r *NovaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (resul
instance.Status.Conditions.MarkTrue(novav1.NovaAPIDBReadyCondition, condition.DBReadyMessage)
}

// We need to create a list of cellNames to iterate on and as the map
// iteration order is undefined we need to make sure that cell0 is the
// first to allow dependency handling during ensureCell calls.
orderedCellNames := []string{Cell0Name}
for cellName := range instance.Spec.CellTemplates {
if cellName != Cell0Name {
orderedCellNames = append(orderedCellNames, cellName)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this works but we could also use https://pkg.go.dev/github.com/wk8/go-ordered-map
nice find either way.
ill admit i had not checked if the hash order was consistent in golang and given the test run with a random seed which would also presumably impact the random number generator used for the hashing it makes sesce that this would be non deterministic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you noted below simply having an insertion order preserving map is not enough as we need to process cell0 before all the other cells depending on cell0 (those where hasAPIAccess = True).


// Create the Cell DBs. Note that we are not returning on error or if the
// DB creation is still in progress. We move forward with whathever we can
// and relay on the watch to get reconciled if some of the resources change
// status
cellDBs := map[string]*nova.Database{}
var failedDBs []string
var creatingDBs []string
for cellName, cellTemplate := range instance.Spec.CellTemplates {
for _, cellName := range orderedCellNames {
cellTemplate := instance.Spec.CellTemplates[cellName]
cellDB, status, err := r.ensureCellDB(ctx, h, instance, cellName, cellTemplate)
if err != nil {
failedDBs = append(failedDBs, fmt.Sprintf("%s(%v)", cellName, err.Error()))
Expand Down Expand Up @@ -255,10 +266,11 @@ func (r *NovaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (resul
cellMQs := map[string]*nova.MessageBus{}
var failedMQs []string
var creatingMQs []string
for cellName, cellTemplate := range instance.Spec.CellTemplates {
for _, cellName := range orderedCellNames {
var cellMQ string
var status nova.MessageBusStatus
var err error
cellTemplate := instance.Spec.CellTemplates[cellName]
// cell0 does not need its own cell message bus it uses the
// API message bus instead
if cellName == Cell0Name {
Expand Down Expand Up @@ -306,7 +318,8 @@ func (r *NovaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (resul
readyCells := []string{}
cells := map[string]*novav1.NovaCell{}
allCellsReady := true
for cellName, cellTemplate := range instance.Spec.CellTemplates {
for _, cellName := range orderedCellNames {
cellTemplate := instance.Spec.CellTemplates[cellName]
cellDB := cellDBs[cellName]
cellMQ := cellMQs[cellName]
if cellDB.Status != nova.DBCompleted {
Expand All @@ -326,8 +339,10 @@ func (r *NovaReconciler) Reconcile(ctx context.Context, req ctrl.Request) (resul
continue
}

cell0, ok := cells[Cell0Name]
if cellName != Cell0Name && cellTemplate.HasAPIAccess && (!ok || !cell0.IsReady()) {
// The cell0 is always handled first in the loop as we iterate on
// orderdCellNames. So for any other cells we can get cell0 from cells
// as it is already there.
if cellName != Cell0Name && cellTemplate.HasAPIAccess && !cells[Cell0Name].IsReady() {
allCellsReady = false
skippedCells = append(skippedCells, cellName)
util.LogForObject(
Expand Down