fix: Resolve monitor refresh loop

[johnramsden]

Description

Extracts the skip condition in the background monitor refresh loop into shouldSkipMonitorRefresh() and adds a failing test that demonstrates the bug: after the first successful config update, the loop never updates ceph.conf again even when new monitors join the cluster.

Fix involves correcting !first || .. short circuit

After fixing the short circuit a different issue appeared that was failing #702 (comment)

This ended up being more complicated than the original problem to fix.

Fixes #556

Type of change

Delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

How has this been tested?

• Unit test exercises failure and watches for regression
• Integration test verifies full workflow

Contributor checklist

Please check that you have:

• self-reviewed the code in this PR
• added code comments, particularly in less straightforward areas
• checked and added or updated relevant documentation
• checked and added or updated relevant release notes
• added tests to verify effectiveness of this change

[johnramsden]

We have a bit of an interesting situation here.

There was a bug I've already fixed in the commit that has been pushed. Unfortunately it introduced a race condition that was resolved before due to some incorrectness that actually fixed the race condition and stopped backwardCompatPubnet from executing.

Context:

We have backwardCompatPubnet whos responsibility is to make sure public_network is present in the database.

We then have bootstrap that can end up running concurrently with backwardCompatPubnet because backwardCompatPubnet is occurring in a go routine in ceph.Start.

Bootstrap should be the source of truth because it can end up inserting a slightly different value if the user submits --public-network

backwardCompatPubnet will try to set the missing public_network if it is not present and this can be mismatched to what was passed to bootstrap.

An easy solution here would be to insert it with backwardCompatPubnet if none present, but then upsert it in bootstrap since that should be the source of truth (right now it is failing in bootstrap because of the race condition between it and backwardCompatPubnet). If it's already present backwardCompatPubnet will not write it so we don't need to worry about that condition. The concern I have with this is we could end up with an incorrect public network for a very short amount of time and I would be concerned you could end up with services on the wrong network as a result.

I was thinking this could be tackled with a condition variable gated on bootstrap having been executed but I think the issue here is there are scenarios where it will never run such as restarts.

May need to know some sort of other condition that lets us know bootstrapping has finished. Might need to gate on some sort of arbitrary marker that we have such as something being written to the database.

I will continue with this next week

[UtkarshBhatthere]

I thought the original problem only required changing the skip conditions:

// Check if we need to 
if !first || reflect.DeepEqual(oldMonitors, monitors) {
	logger.Debugf("start: monitors unchanged, sleeping: %v", monitors)
	time.Sleep(time.Minute)
	continue
}

It should be if !first && reflect.DeepEqual(oldMon, monitors)
Since we are checking with A OR B if any of them is true, updation will be skipped. !first is true in all subsequent runs.

[johnramsden]

That is correct @utkarsh, there was also a race condition that was caused by this change in the section that was previously dead code. The modification you mentioned has been made and the race condition fix was also made