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.

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

feat(cmd): apply MultiNamespacedCacheBuilder, bump-up controller-runtime to 0.8.3 #176

Merged

Conversation

boranx
Copy link
Member

@boranx boranx commented May 12, 2021

Fixes: https://issues.redhat.com/browse/OSD-7118

unblocked
Currently, this PR is blocked as it would require go 1.15, bp supports 1.13.8 as of now.

a problem:
kubernetes-sigs/controller-runtime#934
With MultiNamespacedCacheBuilder, using the client, we're making a call to be able to get cluster platform(https://github.com/openshift/cloud-ingress-operator/blob/master/pkg/utils/infrastructure.go#L18-L22), which is a global resource, then we're seeing errors in the reconcile loop of the controllers because of that issue above:

"logger":"controller_sshd","msg":"Failed to get cluster's platform","error":"unable to get: /cluster because of unknown namespace for the cache"

the issue above is Solved:
Using the same pattern used on unit tests of the package(dynamic client + unstructured.Unstructured):
https://github.com/kubernetes-sigs/controller-runtime/pull/1326/files#diff-3e99be25ef623d0bc22234a878bef63a1fef2029a18d5b35f9079537e9f59ebeR432-R441

Requirement:

Progress to the desired version
#177

@openshift-ci openshift-ci bot requested review from lisa and mwoodson May 12, 2021 16:36
@boranx
Copy link
Member Author

boranx commented May 12, 2021

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 12, 2021
@codecov
Copy link

codecov bot commented May 12, 2021

Codecov Report

Merging #176 (ce32228) into master (3ef81f3) will increase coverage by 0.23%.
The diff coverage is 72.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   26.96%   27.19%   +0.23%     
==========================================
  Files          33       33              
  Lines        2440     2445       +5     
==========================================
+ Hits          658      665       +7     
+ Misses       1723     1722       -1     
+ Partials       59       58       -1     
Impacted Files Coverage Δ
pkg/cloudclient/gcp/private.go 7.39% <0.00%> (ø)
pkg/controller/apischeme/apischeme_controller.go 0.00% <ø> (ø)
...ublishingstrategy/publishingstrategy_controller.go 27.67% <ø> (ø)
...troller/routerservice/router_service_controller.go 34.28% <0.00%> (ø)
pkg/utils/infrastructure.go 83.87% <81.81%> (-2.50%) ⬇️
pkg/cloudclient/aws/private.go 21.89% <100.00%> (ø)
pkg/controller/sshd/sshd_controller.go 60.99% <100.00%> (+0.70%) ⬆️
pkg/testutils/testutils.go 84.75% <100.00%> (ø)

@boranx boranx added do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. and removed do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. labels May 12, 2021
@boranx boranx changed the title feat(cmd): add MultiNamespacedCacheBuilder to manager feat(cmd): add MultiNamespacedCacheBuilder to manager(BLOCKED) May 12, 2021
@boranx boranx changed the title feat(cmd): add MultiNamespacedCacheBuilder to manager(BLOCKED) feat(cmd): add MultiNamespacedCacheBuilder to manager (blocked) May 12, 2021
@boranx boranx force-pushed the MultiNamespacedCacheBuilder branch from 9b12087 to 567b7a4 Compare May 12, 2021 19:11
@@ -36,7 +36,7 @@ spec:
env:
# "" so that the cache can read objects outside its namespace
- name: WATCH_NAMESPACE
value: ""
value: "openshift-sre-sshd,openshift-cloud-ingress-operator,openshift-ingress,openshift-ingress-operator,openshift-kube-apiserver"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work?? That's awesome, and way better.

Copy link
Member Author

Choose a reason for hiding this comment

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

@boranx boranx changed the title feat(cmd): add MultiNamespacedCacheBuilder to manager (blocked) feat(cmd): add MultiNamespacedCacheBuilder to manager May 13, 2021
@boranx boranx changed the title feat(cmd): add MultiNamespacedCacheBuilder to manager [WIP]feat(cmd): add MultiNamespacedCacheBuilder to manager May 17, 2021
@boranx boranx changed the title [WIP]feat(cmd): add MultiNamespacedCacheBuilder to manager [WIP] feat(cmd): add MultiNamespacedCacheBuilder to manager May 17, 2021
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2021
@boranx boranx changed the title [WIP] feat(cmd): add MultiNamespacedCacheBuilder to manager feat(cmd): add MultiNamespacedCacheBuilder to manager May 17, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2021
@boranx boranx changed the title feat(cmd): add MultiNamespacedCacheBuilder to manager feat(cmd): apply MultiNamespacedCacheBuilder, bump-up container-runtime to 0.8.3 May 17, 2021
@boranx boranx changed the title feat(cmd): apply MultiNamespacedCacheBuilder, bump-up container-runtime to 0.8.3 feat(cmd): apply MultiNamespacedCacheBuilder, bump-up controller-runtime to 0.8.3 May 17, 2021
@boranx boranx force-pushed the MultiNamespacedCacheBuilder branch 3 times, most recently from 9f0579c to 96404a0 Compare May 18, 2021 15:24
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2021
@boranx boranx force-pushed the MultiNamespacedCacheBuilder branch from 96404a0 to e816cfe Compare May 24, 2021 09:29
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2021
@boranx boranx force-pushed the MultiNamespacedCacheBuilder branch 3 times, most recently from 45abeb2 to 93cf3f2 Compare May 24, 2021 12:45
Copy link
Member

@2uasimojo 2uasimojo left a comment

Choose a reason for hiding this comment

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

This looks good from what I can tell. A couple of nonblocking questions/observations inline. I'll leave it to CIO maintainers to stamp.

pkg/cloudclient/aws/private.go Show resolved Hide resolved
pkg/controller/routerservice/router_service_controller.go Outdated Show resolved Hide resolved
pkg/controller/routerservice/router_service_controller.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fahlmant fahlmant left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

@openshift-ci openshift-ci bot 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. and removed lgtm Indicates that a PR is ready to be merged. labels May 24, 2021
@boranx boranx force-pushed the MultiNamespacedCacheBuilder branch 3 times, most recently from 12e1cfe to 7d662bc Compare May 25, 2021 08:34
@boranx boranx force-pushed the MultiNamespacedCacheBuilder branch from 7d662bc to ce32228 Compare May 25, 2021 08:37
Copy link
Contributor

@fahlmant fahlmant left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 25, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 25, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: boranx, fahlmant

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

@boranx
Copy link
Member Author

boranx commented May 25, 2021

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2021
@openshift-merge-robot openshift-merge-robot merged commit 6e0ff56 into openshift:master May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants