refactor operator to match siblings #64
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
size/XXL
openshift-ci-robot
added
the
approved
bparees
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/assign @adambkaplan @adambkaplan i think you have the most experience working in this operator, ptal. Note that there are definitely potential followups around emitting events when observing config changes, and adding objectrefs to aid in mustgather, but this at least gets us looking like the other operators and keeps us from getting further behind if more config observation code gets added. |
| } | ||
|
|
||
| func (l Listers) ResourceSyncer() resourcesynccontroller.ResourceSyncer { | ||
| return nil |
There was a problem hiding this comment.
looks like openshift/cluster-openshift-apiserver-operator has non-nil returns, which drives the cache sync.
See apiserver interfaces.go and observe_config_controller.go
There was a problem hiding this comment.
this isn't about cache sync. resourcesyncer copies resources between namespaces. this operator does not use it anywhere in the code path.
| BuildConfigLister: configInformers.Config().V1().Builds().Lister(), | ||
| BuildConfigSynced: configInformers.Config().V1().Builds().Informer().HasSynced, | ||
| ConfigMapLister: kubeInformersForOperatorNamespace.Core().V1().ConfigMaps().Lister(), | ||
| ConfigMapSynced: kubeInformersForOperatorNamespace.Core().V1().ConfigMaps().Informer().HasSynced, |
There was a problem hiding this comment.
two items:
- Pass in a
ResourceSyncer? - Pre-run cache sync for the operator config object?
There was a problem hiding this comment.
-
we don't have a resourcesyncer to pass because we don't have any resources we are syncing across namepaces.
-
i didn't see an obvious use/value in the pre cache sync given that we need to check it in the observers.
| } | ||
|
|
||
| kubeInformersForOperatorNamespace.Core().V1().ConfigMaps().Informer().AddEventHandler(c.EventHandler()) | ||
| configInformers.Config().V1().Images().Informer().AddEventHandler(c.EventHandler()) |
There was a problem hiding this comment.
Need an event handler for Builds, too.
There was a problem hiding this comment.
well that is interesting, because the existing code didn't have one either, so i'm not sure how this was working.....
There was a problem hiding this comment.
sounds like we need an integration test. Given that we'll need to change cluster config to drive the test, not sure we have a clear path to build these tests at present.
There was a problem hiding this comment.
it can be done in our e2e suite.
There was a problem hiding this comment.
openshift/origin#21864 starts us down that path. Each test adds 1-2 minutes to the total suite.
There was a problem hiding this comment.
I actually meant it can be added in the e2e suite on this repo, that's what i ended up doing. (the test that sets the cluster config and ensures it propagates to the operand config).
There was a problem hiding this comment.
yes, but I think this gets us halfway there. IMO a full e2e should:
- Modify the cluster config
- Verify the operator processed the config change
- Verify the config change rolls out to the operand
- Verifies the expected behavior changed (ex: with
BuildDefaults)
I'd love to test steps 3 and 4 in this repo, but atm origin has better test frameworks on this front.
There was a problem hiding this comment.
yes that would be a full product e2e. what is here is effectively an e2e for the operator behavior.
I don't want to start loading up the controller manager operator e2e suite w/ tests that depend on builds working/build-controller behavior. Origin is the right place for that level of testing.
We should restrict operator e2es to tests that exercise/depend only on the operator behavior. Otherwise we're just setting ourselves up for:
- flakes
- troublesome multistep merges because we have to change behavior somewhere which means changing a test somewhere else, etc.
|
@adambkaplan added precache syncing though i'm not really sure what purpose it serves. also added e2e tests to confirm we are observing build+image cluster config objects properly. still don't think we have any use for resourcesyncer so i have left that out. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, bparees 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Bug 1928141: kube-storage-version-migrator constantly reporting type "Upgradeable" status Unknown
No description provided.