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 1680504. Create collector clusterrole to watch pods/ns #106

Merged
merged 1 commit into from Mar 1, 2019
Merged

bug 1680504. Create collector clusterrole to watch pods/ns #106

merged 1 commit into from Mar 1, 2019

Conversation

jcantrill
Copy link
Contributor

This PR fixes https://bugzilla.redhat.com/show_bug.cgi?id=1680504 by:

  • creates a clusterrole for the logcollector to be able to watch all pods and namespaces

@jcantrill
Copy link
Contributor Author

/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 Feb 28, 2019
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 28, 2019
@richm
Copy link
Contributor

richm commented Feb 28, 2019

I suggest setting fluentd with loglevel trace or debug to see the metadata cache stats to see if it is reporting any problems, and the cache stats appear to be changing correctly

@richm richm mentioned this pull request Feb 28, 2019
Closed
@jcantrill jcantrill force-pushed the 1680504_fluent_cluster_role_binding branch from e78b7ca to 3088eff Compare February 28, 2019 16:17
@jcantrill
Copy link
Contributor Author

/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 Feb 28, 2019
richm
richm approved these changes Feb 28, 2019
View reviewed changes
Copy link
Contributor

@richm richm left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2019
@openshift-bot
Copy link
Contributor

/retest

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

@jcantrill
Copy link
Contributor Author

/test e2e-operator

@openshift-bot
Copy link
Contributor

/retest

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

@jcantrill jcantrill force-pushed the 1680504_fluent_cluster_role_binding branch from 3088eff to e3be0ee Compare February 28, 2019 19:55
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 28, 2019
@jcantrill
Copy link
Contributor Author

@ewolinetz @richm Note rsyslog e2e test failed. Temporarily disabled for 4.0 since its no longer techpreview. We'll have to re-enable for 4.x

@richm
Copy link
Contributor

richm commented Feb 28, 2019

looking at the full test output - https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-logging-operator/106/pull-ci-openshift-cluster-logging-operator-master-e2e-operator/155/build-log.txt - fluentd failed too - so it might not be a problem with rsyslog

ewolinetz
ewolinetz suggested changes Feb 28, 2019
View reviewed changes
Copy link
Contributor

@ewolinetz ewolinetz left a comment

Choose a reason for hiding this comment

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

uncomment the 'removeFluentd' line, it is required

ewolinetz
ewolinetz reviewed Feb 28, 2019
View reviewed changes
pkg/k8shandler/collection.go
[]string{"get", "list", "watch"},
),
)
clusterRole, err := utils.CreateClusterRole("metadata-reader", clusterrules, cluster)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update this to follow the other role creation patterns for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you are asking here. I added code to a reusable method that does the same thing as if it was inlined.

@ewolinetz
Copy link
Contributor

@jcantrill you need to add the permission to * clusterroles for the clo SA in manifests/03-role.yaml

@jcantrill jcantrill force-pushed the 1680504_fluent_cluster_role_binding branch from e3be0ee to ede2092 Compare March 1, 2019 14:42
@jcantrill jcantrill force-pushed the 1680504_fluent_cluster_role_binding branch 2 times, most recently from 9d2e8c3 to d98bb92 Compare March 1, 2019 16:23
ewolinetz
ewolinetz reviewed Mar 1, 2019
View reviewed changes
manifests/03-role.yaml Outdated Show resolved Hide resolved
@jcantrill jcantrill force-pushed the 1680504_fluent_cluster_role_binding branch from d98bb92 to aa7fe82 Compare March 1, 2019 18:07
@ewolinetz
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 1, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ewolinetz, jcantrill, richm

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:
  • OWNERS [ewolinetz,jcantrill,richm]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit aff8977 into openshift:master Mar 1, 2019
@jcantrill jcantrill deleted the 1680504_fluent_cluster_role_binding branch March 1, 2019 20:55
pmoogi-redhat pushed a commit to pmoogi-redhat/cluster-logging-operator that referenced this pull request Apr 26, 2022
@openshift-merge-robot
Merge pull request openshift#106 from jcantrill/1680504_fluent_cluste…
c5f569d 
…r_role_binding

bug 1680504. Create collector clusterrole to watch pods/ns
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richm richm richm approved these changes

@ewolinetz ewolinetz ewolinetz approved these changes

Assignees

@richm richm

@ewolinetz ewolinetz

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jcantrill @richm @openshift-bot @ewolinetz @openshift-ci-robot @openshift-merge-robot