modified resource sync to use an informer #137
Conversation
rg2011
force-pushed
the
fix/issue-134-bis
branch
2 times, most recently
from
6a905ca to
6d67b02
Compare
|
I have updated the PR to filter out updates in the I'm removing |
rg2011
changed the title
| @@ -474,9 +473,9 @@ func TestGenericSync(t *testing.T) { | |||
|
|
|||
There was a problem hiding this comment.
❓ How do you judge our test coverage here, does it inspire confidence? (I really don't know, it's a genuine question since you're working on this.)
There was a problem hiding this comment.
After #135 we have the initial load covered, and I'm confident on that part. But I have failed to add tests for addition / update / removal of resources after the initial load, and that's a problem.
I have tried both FakeDynamicClient.Tracker and FakeDynamicClient.Resource(){.Create, .Update, .Delete}, but I couldn't make them work. The tests didn't work even with the previous version.
So I'm currently testing this "on the field", in my company's dev environment, before we deploy the service to our production. In parallel, I'm checking out the FakeDynamicClient code to try and find out why the tests don't work.
There was a problem hiding this comment.
I could make the tests work in 602be1f and now the coverage is around 78%. I think it's safe to remove the draft label now.
rg2011
force-pushed
the
fix/issue-134-bis
branch
from
6d67b02 to
c421f3b
Compare
|
Hey @rg2011 , are you on the OPA Slack by any chance? |
rg2011
force-pushed
the
fix/issue-134-bis
branch
3 times, most recently
from
a773b8f to
911a83a
Compare
Signed-off-by: rg2011 <[email protected]>
rg2011
force-pushed
the
fix/issue-134-bis
branch
2 times, most recently
from
8f5864c to
b7adb52
Compare
Signed-off-by: rg2011 <[email protected]>
rg2011
force-pushed
the
fix/issue-134-bis
branch
from
b7adb52 to
602be1f
Compare
Signed-off-by: rg2011 <[email protected]>
rg2011
force-pushed
the
fix/issue-134-bis
branch
from
7e180bb to
10f90c4
Compare
|
@rg2011 didn't get to this today. I'll re-review later this week 🤞 Thanks for pushing this forward. |
Signed-off-by: rg2011 <[email protected]>
rg2011
force-pushed
the
fix/issue-134-bis
branch
from
d49ee72 to
9a98fac
Compare
Signed-off-by: rg2011 <[email protected]>
| return f.actor(req, value) | ||
| } | ||
|
|
||
| var errNotSupported = errors.New("PostData not supported") |
There was a problem hiding this comment.
[nit We could inline this, it's only used once (unless I'm missing something).
There was a problem hiding this comment.
No real reason to create a variable, other than I'm used to it because it makes it easier to check this kind of errors with errors.Is later. But this particular error will never be checked, so we could remove it.
| return fmt.Sprintf("{req: %q, path: %q, op: %q}", r.req, r.path, r.op) | ||
| } | ||
|
|
||
| func optional(expected ...[]byte) []byte { |
|
@rg2011 @anderseknert @srenatus should I merge this ? |
Rewrite of data.GenericSync that uses an informer to manage the connection to the Kubernetes API. Fixes issue #134
The informer uses a
workqueueto schedule updates to OPA. To avoid an initial flood of individual resource updates, the controller keeps dropping items from the queue until it has completed the first bulk update to OPA.A major drawback of this PR is that the flood might occur later on, if the informer restarts the watch and flushes all resources to the queue again. The controller will then update OPA item by item.I'm publishing this PR as a draft, in case someone is interested and wants to give it a thought. Any suggestion on how to avoid an update flood when the informer reconnects would be appreciated.