Add Syncer Skeleton #509
Add Syncer Skeleton #509
Conversation
k8s-ci-robot
added
cncf-cla: yes
freehan
force-pushed
the
noop
branch
2 times, most recently
from
f66130e to
68e38a0
Compare
pkg/neg/syncers/noop.go
Outdated
|
|
||
| // noopNegSyncer is a NEG syncer template. | ||
| // It handles state transitions and backoff retry operations. | ||
| type noopNegSyncer struct { |
There was a problem hiding this comment.
Not the biggest fan of calling this noopNegSyncer.
I would just call this Syncer.
pkg/neg/syncers/noop.go
Outdated
| backoff backoffHandler | ||
| } | ||
|
|
||
| func newNoopSyncer(negSyncerKey NegSyncerKey, networkEndpointGroupName string, serviceLister cache.Indexer, recorder record.EventRecorder) *noopNegSyncer { |
There was a problem hiding this comment.
Would it make more sense to pass in a RecordProducer? (see pkg/events)
Right now the ControllerContext implements that interface so in whatever code calls newNoopSyncer, you can just pass a context.
There was a problem hiding this comment.
Yes. That sounds good.
But not sure why there is recorder per namespace
pkg/neg/syncers/noop.go
Outdated
| type noopNegSyncer struct { | ||
| // metadata | ||
| NegSyncerKey | ||
| negName string |
There was a problem hiding this comment.
Looks like NegSyncerKey contains the name so how come you need negName?
There was a problem hiding this comment.
NegSyncerKey contains the name, namespace, port of the service
pkg/neg/syncers/noop.go
Outdated
|
|
||
| // syncer states | ||
| stateLock sync.Mutex | ||
| stopped bool |
There was a problem hiding this comment.
I would suggest abstracting these states into a type like:
type SyncState int
const (
StateStopped SyncState = iota
StateShuttingDown SyncState
...
)
There was a problem hiding this comment.
But then I only have one variable to store state, right?
when state == ShuttingDown. Should IsStopped return true?
Otherwise, it would be the same as having 2 booleans.
pkg/neg/syncers/noop.go
Outdated
| } | ||
| } | ||
|
|
||
| func (s *noopNegSyncer) Sync() bool { |
pkg/neg/syncers/noop.go
Outdated
| } | ||
| } | ||
|
|
||
| func (s *noopNegSyncer) IsStopped() bool { |
There was a problem hiding this comment.
Do these need to be exposed?
There was a problem hiding this comment.
Yes. Because the SyncerManager calls it.
pkg/neg/syncers/noop_test.go
Outdated
| t.Fatalf("Failed to start syncer: %v", err) | ||
| } | ||
|
|
||
| if err := wait.PollImmediate(time.Second, 30*time.Second, func() (bool, error) { |
There was a problem hiding this comment.
My suggestion would be to make the retry handler an interface and then implement a fake which retries at a much faster pace. That way, unit test completion won't be blocked on this.
There was a problem hiding this comment.
Add a block/unblock mechanism for the tester.
But it still needs to poll for a few seconds just to make sure the other go routine can execute completely. This is to make sure the test does not flak.
freehan
force-pushed
the
noop
branch
4 times, most recently
from
4e1b54a to
f949f9c
Compare
|
Ready for another round. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: freehan, rramkumar1 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The syncer skeleton contains all the mechanism that handles syncer life cycle, including
cc @agau4779