OCPBUGS-13895: [WRKLDS-730] route-controller-manager deployment updates#288
Conversation
| requiredConfigMap, _, err := resourcemerge.MergeConfigMap(configMap, "config.yaml", nil, defaultConfig, operatorConfig.Spec.ObservedConfig.Raw, operatorConfig.Spec.UnsupportedConfigOverrides.Raw) | ||
| ocmDefaultConfig := v311_00_assets.MustAsset("v3.11.0/config/defaultconfig.yaml") | ||
| rcmDefaultConfig := v311_00_assets.MustAsset("v3.11.0/config/route-controller-defaultconfig.yaml") | ||
| requiredConfigMap, _, err := resourcemerge.MergeConfigMap(configMap, "config.yaml", nil, ocmDefaultConfig, rcmDefaultConfig, operatorConfig.Spec.ObservedConfig.Raw, operatorConfig.Spec.UnsupportedConfigOverrides.Raw) |
There was a problem hiding this comment.
How is the resulting OpenShiftControllerManagerConfig config gonna look like? If I read the code correctly you combine content of both defaultconfig.yaml and route-controller-defaultconfig.yaml into a single OpenShiftControllerManagerConfig object which is a union of both. Resp. if both have the same keys the latter overrides the former. What's the purpose of the new route-controller-defaultconfig.yaml if both are hardcoded in the assets? You basically want to share the default configuration between both RCM and OCM. Plus, to easily override the default with route-controller-defaultconfig.yaml without changing anything in the code?
There was a problem hiding this comment.
The reason is to have default for RCM that override the behaviour of OCM. But the RCM config should not influence OCM. Practically OCM is empty now, but if somebody introduces some configuration, it might be imporant for RCM as well. This very theoretic though, If you prefer I could split these two and have just dedicated configs for each component.
The main reason is to supply lease name to RCM since we introduced it before and we need to override the default one in library-go.
Other option is to invest in the lease migration, but that would be quite painful.
There was a problem hiding this comment.
Practically OCM is empty now, but if somebody introduces some configuration, it might be imporant for RCM as well. This very theoretic though, If you prefer I could split these two and have just dedicated configs for each component.
There are two sides to this:
- anything introduced to the default config will get propagated to the RCM one. However, in case one intends to introduce a configuration for OCM and forgets about RCM, the RCM might unintentionally start to behave differently.
- separate configs are protected against unintentional RCM configuration. However, if one means to configure both OCM and RCM and forgets about the separation, the RCM will not start to behave differently.
The second option should be easier to detect by QE. E.g. if a new configuration is introduced and RCM does not behave as expected, one might dig deeper and learn there's actually a separate RCM config. So preferably we should separate the configs and put a comment into the default one about the self-standing RCM config.
There was a problem hiding this comment.
I agree, it is better to split it and define new behaviour twice instead of a silent overtaking of values that might be surprising.
I think it is better to express the difference via the filename than a comment so I renamed the defaultconfig to openshift-controller-manager-defaultconfig so both OCM and RCM config are on the same level.
|
@atiratree if my interpretation of the code changes is correct, feel free to unhold /hold |
openshift-ci
bot
added
the
do-not-merge/hold
openshift-ci
bot
added
lgtm
- customize lease name openshift-route-controllers to override the default one supplied by library-go - add RBAC for infrastructures which is used by library-go for configuring leader election
- to better describe the separations of and difference between the OCM and RCM configs
| @@ -0,0 +1,4 @@ | |||
| apiVersion: openshiftcontrolplane.config.openshift.io/v1 | |||
There was a problem hiding this comment.
route-controller-defaultconfig.yaml -> route-controller-manager-defaultconfig.yaml
There was a problem hiding this comment.
I have changed all route-controller-manager files to have a consistent prefix
|
cluster bootstrap is failing |
|
@atiratree: The following test failed, say
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. |
|
/lgtm |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED 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 |
|
/label docs-approved |
openshift-ci
bot
added
docs-approved
atiratree
changed the title
|
/jira refresh |
|
@atiratree: No Jira issue is referenced in the title of this pull request. DetailsIn response to this:
Instructions 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. |
atiratree
changed the title
|
/jira refresh |
|
@atiratree: No Jira issue is referenced in the title of this pull request. DetailsIn response to this:
Instructions 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. |
atiratree
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@atiratree: This pull request references Jira Issue OCPBUGS-13895, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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. |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
/jira refresh |
|
@atiratree: This pull request references Jira Issue OCPBUGS-13895, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions 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. |
|
@atiratree: Jira Issue OCPBUGS-13895: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-13895 has been moved to the MODIFIED state. DetailsIn response to this:
Instructions 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. |
4 participants
The following changes are required for openshift/route-controller-manager#22 refactoring.