[WRKLDS-730] refactor route controller manager to use library-go server and ControlerCommand for generic setup #22
Conversation
|
The CI passes even without operator PR being merged. In RCM logs we can that the leader election obtains an incorrect default lock - this could cause a race if an old instance of RCM would use the original |
…llerCommand for generic setup - simplify ControllerContext
|
/test all |
|
@atiratree: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
correct lease is being picked up in the recent run |
|
|
||
| type ControllerContext struct { | ||
| // TODO: Make this minimal config instead of passing entire controller manager config | ||
| type EnhancedControllerContext struct { |
There was a problem hiding this comment.
Why the rename from ControllerContext to EnhancedControllerContext? Is ControllerContext no longer a valid keyword?
There was a problem hiding this comment.
to distinguish it from the embedded controllercmd.ControllerContext - we will need the embedded one later refactoring when accessing for example the EventRecorder
| ) | ||
|
|
||
| // TODO make this an actual API server built on the genericapiserver | ||
| func RunControllerServer(servingInfo configv1.HTTPServingInfo, kubeExternal clientgoclientset.Interface) error { |
There was a problem hiding this comment.
RunControllerServer is no longer needed? Has it been ever used before?
There was a problem hiding this comment.
it was run before, now it is responsibility of the library-go
| } | ||
|
|
||
| // only serve if we have serving information. | ||
| if config.ServingInfo != nil { |
There was a problem hiding this comment.
Has ServingInfo ever been set?
There was a problem hiding this comment.
probably not from the outside, but we are calling SetRecommendedHTTPServingInfoDefaults which initializes it
|
|
||
| config := obj.(*openshiftcontrolplanev1.OpenShiftControllerManagerConfig) | ||
| /// this isn't allowed to be nil when by itself. | ||
| // TODO remove this when the old path is gone. |
There was a problem hiding this comment.
Removing ServingInfo is probably related to this action?
There was a problem hiding this comment.
I think this is a legacy TODO, when the code was still in origin
ingvagabund
added
the
approved
|
/lgtm |
ingvagabund
added
the
jira/valid-bug
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: atiratree, ingvagabund The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
and simplify ControllerContext.
CI is expected to fail until we get the operator PR in (#22 (comment))
TODO: