requeue after the last node has its node-state label set to Started during cluster creation #77
Conversation
This commit fixes an issue in which we basically see out of order status updates. Specifically, the .Status.SuperUserUpserted property is getting set before the last node is added to .Status.NodeStatuses. The actual order of operations is correct though. The super user is not created until all C* nodes are started. It is just that the status updates basically occur out of order.
jsanda
requested review from
bradfordcp,
jimdickinson,
johntrimble,
respringer and
sandoichi
as code owners
tests/scale_up_status_updates/scale_up_status_updates_suite_test.go
Outdated
Show resolved
Hide resolved
|
|
||
| var _ = Describe(testName, func() { | ||
| Context("when in a new cluster", func() { | ||
| Specify("the operator can scale up a datacenter", func() { |
There was a problem hiding this comment.
Might want to make this a bit more descriptive for what you are actually testing. We have a couple of other scale up tests that do nothing but increase the node count, and we probably want to distinguish the messages in this one.
There was a problem hiding this comment.
yeah, I just copied/pasted from the scale up test. I will update this.
There was a problem hiding this comment.
@sandoichi wdyt about the updated message?
| step = "checking that all nodes have been started" | ||
| nodeStatusesHostIds := ns.GetNodeStatusesHostIds(dcName) | ||
| Expect(len(nodeStatusesHostIds), 6) | ||
| }) |
There was a problem hiding this comment.
Most of our tests will typically test that the dc can be successfully deleted at the end of the test scenario. It probably isn't super important to have on every single one (especially since you really aren't doing any crazy patching or unusual things) so I'll let you decide if you want to add the logic to this test or not. See our scale up test for reference if you choose to (again, purely optional from my perspective).
There was a problem hiding this comment.
I will add the deletion check(s). I would rather be consistent with other tests.
| @@ -476,6 +476,9 @@ func (rc *ReconciliationContext) CheckPodsReady(endpointData httphelper.CassMeta | |||
| desiredSize := int(rc.Datacenter.Spec.Size) | |||
|
|
|||
| if desiredSize == readyPodCount && desiredSize == startedLabelCount { | |||
| if nodeStarted { | |||
There was a problem hiding this comment.
I think you should add a one line comment here
also why not move this directly under
nodeIsStarting, nodeStarted, err := rc.findStartingNodes()
?
There was a problem hiding this comment.
comment added.
I hadn't really considered moving the if block. That will require some further discussion to ensure I don't wind up introducing some regressions.
There was a problem hiding this comment.
yeah let's not introduce instability, I can tackle this when refactoring some stuff in an upcoming PR
|
I ran all integration tests locally. I get one failure: I also ran the tests in master and got the same failure. |
|
We'll see if thats a GKE vs KIND issue... |
This PR is for #72. It fixes an issue in which we basically see out of order status updates. Specifically, the
.Status.SuperUserUpsertedproperty is getting set before the last node is added to.Status.NodeStatuses. The actual order of operations is correct though. The super user is not created until all C* nodes are started. It is just that the status updates basically occur out of order.