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.

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

Send WaitForDatastore if there's a watch error #1063

Merged
merged 1 commit into from Mar 28, 2019

Conversation

Projects
None yet
3 participants
@lmm
Copy link
Member

commented Mar 20, 2019

Description

This updates the syncer to be able to report WaitForDatastore if any of its watcherCaches are not in-sync. Previously, the watcherSyncer state only went from WaitForDatastore -> ResyncInProgress -> InSync. This PR adds another state change: InSync -> WaitForDatastore.

Fixes #478

This depends on a change to confd to handle multiple InSync statuses: projectcalico/confd#230

Todos

  • Tests
  • Documentation
  • Release note

Release Note

Improve `calico/node` readiness probe reporting when watches on the datastore error out.

@lmm lmm force-pushed the lmm:syncer-readiness branch 2 times, most recently from 466011e to cf38938 Mar 20, 2019

@@ -131,21 +131,30 @@ var _ = Describe("Test the backend datastore multi-watch syncer", func() {
By("Driving a bunch of List complete, Watch fail events for 2/3 resource types")
expectedDuration := watchersyncer.WatchPollInterval * 2
minDuration := 70 * expectedDuration / 100
maxDuration := 130 * expectedDuration / 100
maxDuration := 140 * expectedDuration / 100

This comment has been minimized.

Copy link
@lmm

lmm Mar 22, 2019

Author Member

This section is a bit slower now with all of the ExpectStatusUpdate calls

@@ -131,21 +131,30 @@ var _ = Describe("Test the backend datastore multi-watch syncer", func() {
By("Driving a bunch of List complete, Watch fail events for 2/3 resource types")
expectedDuration := watchersyncer.WatchPollInterval * 2
minDuration := 70 * expectedDuration / 100
maxDuration := 130 * expectedDuration / 100
maxDuration := 140 * expectedDuration / 100
before := time.Now()

This comment has been minimized.

Copy link
@lmm

lmm Mar 22, 2019

Author Member

Move this up since at the original location of the time.Now() call, all the events will have been handled while expecting the status updates.

@lmm lmm force-pushed the lmm:syncer-readiness branch from cf38938 to 4dedb22 Mar 22, 2019

rs.clientListResponse(r1, emptyList)
rs.ExpectStatusUpdate(api.ResyncInProgress)
rs.clientWatchResponse(r1, genError)
rs.ExpectStatusUnchanged()

This comment has been minimized.

Copy link
@caseydavenport

caseydavenport Mar 22, 2019

Member

So I understand, were these test changes required? Or were they added to test the new functionality?

It seems like this might be covered in the new tests you added above, so wondering what this change is for.

This comment has been minimized.

Copy link
@lmm

lmm Mar 22, 2019

Author Member

The ExpectStatusUpdate ones are required for any status change but you're right the ExpectStatusUnchanged calls aren't required and aren't needed here as that's tested above.

@caseydavenport

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

@lmm could you squash your commits? Then I'll merge. Thanks!

@lmm lmm force-pushed the lmm:syncer-readiness branch from 5c6b77f to 032bc00 Mar 28, 2019

@caseydavenport caseydavenport merged commit d0e0716 into projectcalico:master Mar 28, 2019

2 checks passed

license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details

@lmm lmm deleted the lmm:syncer-readiness branch Apr 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.