Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug 1853278: observe console-config config map without using a resource sync controller. #298

Merged
merged 1 commit into from Jul 10, 2020
Merged

Bug 1853278: observe console-config config map without using a resource sync controller. #298

merged 1 commit into from Jul 10, 2020

Conversation

vareti
Copy link
Contributor

@vareti vareti commented Jul 7, 2020 •
edited

As v4-0-config-system-console-config config map is not mounted in oauth-server pod, maybe using a resource sync controller to synchronize with console-config config map from openshift-config-managed is not fully beneficial. Instead, the observer could directly watch for any changes in console-config. This would reduce the number of DELETE requests made to kube-apiserver.

@openshift-ci-robot openshift-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Jul 7, 2020
@openshift-ci-robot
Copy link
Contributor

@vareti: This pull request references Bugzilla bug 1853278, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1853278: observe console-config config map without using a resource sync controller.

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 openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jul 7, 2020
@openshift-ci-robot
Copy link
Contributor

@vareti: This pull request references Bugzilla bug 1853278, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1853278: observe console-config config map without using a resource sync controller.

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.

@vareti
Copy link
Contributor Author

vareti commented Jul 7, 2020

looks like flakes

/retest

@vareti
Copy link
Contributor Author

vareti commented Jul 7, 2020

/retest

@mfojtik
Copy link
Member

mfojtik commented Jul 8, 2020

/lgtm
/retest

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2020
@mfojtik
Copy link
Member

mfojtik commented Jul 8, 2020

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2020
mfojtik
mfojtik reviewed Jul 8, 2020
View reviewed changes
pkg/controllers/configobservation/oauth/template_conversions.go Outdated
@@ -82,7 +82,7 @@ func convertTemplatesWithBranding(cmLister corelistersv1.ConfigMapLister, config
}

func getConsoleBranding(cmLister corelistersv1.ConfigMapLister) (string, error) {
cm, err := cmLister.ConfigMaps("openshift-authentication").Get("v4-0-config-system-console-config")
cm, err := cmLister.ConfigMaps("openshift-config-managed").Get("wat")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

giphy

Copy link
Member

@stlaz stlaz Jul 8, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold
this will always result in "Not found", "openshift-config-managed" is not in namespaces that are being synced for the underlying informer

edit: not mentioning the wat

Copy link
Contributor Author

@vareti vareti Jul 8, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edit: not mentioning the wat

haha.. I have no idea how this happened. It was supposed to be console-config.

this will always result in "Not found", "openshift-config-managed" is not in namespaces that are being synced for the underlying informer

Also, I will check on adding openshift-config-managed to list of informer namespaces.

@vareti vareti force-pushed the reduce-delete-calls-by-removing-two-step-sync-of-console-config branch from 072c2b4 to 8446e39 Compare July 8, 2020 12:06
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2020
@vareti
Copy link
Contributor Author

vareti commented Jul 8, 2020

@mfojtik @stlaz addressed your comments. please take a look.

stlaz
stlaz requested changes Jul 8, 2020
View reviewed changes
pkg/operator2/starter.go Outdated
@@ -74,6 +74,7 @@ func RunOperator(ctx context.Context, controllerContext *controllercmd.Controlle
"openshift-authentication",
"openshift-config",
"kube-system",
"openshift-config-managed",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep alphabetical order.

You also need to add it here --v. This will cause the observer to resync on every change of a secret/cm in the openshift-config-managed NS, but I guess it's still better than us polluting the metrics.

cluster-authentication-operator/pkg/controllers/configobservation/configobservercontroller/observe_config_controller.go

Line 28 in 8bb2e80

interestingNamespaces := []string{

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep alphabetical order.

you meant reverse alphabetical order? ;)

Added this namespace to the list you pointed. please take a look

@vareti vareti force-pushed the reduce-delete-calls-by-removing-two-step-sync-of-console-config branch from 8446e39 to ced98a2 Compare July 8, 2020 12:40
@vareti
Copy link
Contributor Author

vareti commented Jul 8, 2020

/retest

@stlaz
Copy link
Member

stlaz commented Jul 9, 2020

/lgtm
/hold
remove hold once you at least manually test it

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 9, 2020
@vareti
Copy link
Contributor Author

vareti commented Jul 10, 2020

PR is verified manually

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2020
@mfojtik
Copy link
Member

mfojtik commented Jul 10, 2020

please /hold for #299 merging

@mfojtik
Copy link
Member

mfojtik commented Jul 10, 2020

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2020
@mfojtik
Copy link
Member

mfojtik commented Jul 10, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2020
@vareti vareti force-pushed the reduce-delete-calls-by-removing-two-step-sync-of-console-config branch from ced98a2 to 62d426c Compare July 10, 2020 12:43
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2020
@vareti
Copy link
Contributor Author

vareti commented Jul 10, 2020

@stlaz @mfojtik I rebased the PR to resolve conflicts. this pr needs LGTM tag again.

@mfojtik
Copy link
Member

mfojtik commented Jul 10, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, stlaz, vareti

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vareti
Copy link
Contributor Author

vareti commented Jul 10, 2020

thanks @mfojtik

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit daa3976 into openshift:master Jul 10, 2020
@openshift-ci-robot
Copy link
Contributor

@vareti: All pull requests linked via external trackers have merged: openshift/cluster-authentication-operator#298. Bugzilla bug 1853278 has been moved to the MODIFIED state.

In response to this:

Bug 1853278: observe console-config config map without using a resource sync controller.

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.

@vrutkovs
Copy link
Member

/cherry-pick release-4.5

@openshift-cherrypick-robot
Copy link

@vrutkovs: #298 failed to apply on top of branch "release-4.5":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
A	pkg/controllers/configobservation/configobservercontroller/observe_config_controller.go
A	pkg/controllers/configobservation/oauth/template_conversions.go
A	pkg/operator/starter.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/operator2/starter.go
CONFLICT (content): Merge conflict in pkg/operator2/starter.go
Auto-merging pkg/operator2/configobservation/configobservercontroller/observe_config_controller.go
CONFLICT (content): Merge conflict in pkg/operator2/configobservation/configobservercontroller/observe_config_controller.go
CONFLICT (modify/delete): pkg/controllers/configobservation/oauth/template_conversions.go deleted in HEAD and modified in remove two step watch for console config. Version remove two step watch for console config of pkg/controllers/configobservation/oauth/template_conversions.go left in tree.
Patch failed at 0001 remove two step watch for console config

In response to this:

/cherry-pick release-4.5

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfojtik mfojtik mfojtik left review comments

@stlaz stlaz stlaz requested changes

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@mfojtik mfojtik

@stlaz stlaz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@vareti @openshift-ci-robot @mfojtik @stlaz @openshift-bot @vrutkovs @openshift-cherrypick-robot @openshift-merge-robot