Skip to content
This repository has been archived by the owner on Jul 10, 2020. It is now read-only.

Add sink resources #2

Merged
merged 3 commits into from
Dec 10, 2018
Merged

Add sink resources #2

merged 3 commits into from
Dec 10, 2018

Conversation

wfernandes
Copy link
Contributor

@wfernandes wfernandes commented Nov 14, 2018

This PR is intended to initiate conversation around adding sink resources to observability. We have attempted to follow knative conventions/guidelines as much as possible.

We are looking for feedback.

Summary of the changes:

  • Add sink resources config
  • Add sink and event controllers
  • Add tests for sink-resources CRDs
  • Add test infra
  • Add e2e tests
  • Add CONTRIBUTING guidelines for development
  • Move role bindings to separate files and make file names consistent
  • Fix fluent bit labels
  • Pin fluent-bit-out-syslog version to v0.9
  • Update imagePullPolicy to IfNotPresent for fluent-bit-out-syslog
  • Refactor signal handling to use knative/pkg/signals
  • Modify controllers to implement cache.ResourceEventHandler interface
    https://godoc.org/k8s.io/client-go/tools/cache#ResourceEventHandler
  • Pin version of oratos/crosstalk-receiver in e2e tests to v0.3
  • Pin version of ubuntu to Xenial in e2e tests. Xenial is currently OSL approved.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@knative-prow-robot knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 14, 2018
@jasonkeene
Copy link
Contributor

As far as the google bot is concerned. I am the author of the commits and I confirm that I am ok with these commits being contributed to the project. I can not set the cla label to yes so I leave that up to the powers that be.

This is a large change. It is most of the work that we have put into the sink resources. This is just a first stab at getting this functionality into knative. There has been suggestions that we consider using the knative/pkg/controller for our controllers, however that isn't within the scope of this initial PR.

We'd appreciate any feedback on this change.

@dprotaso
Copy link
Member

/assign @dprotaso

@yanweiguo
Copy link

cc @mdemirhan

This is to make the PR easier to comprehend
@googlebot
Copy link

CLAs look good, thanks!

- Add sink resources config
- Add sink and event controllers
- Add tests for sink-resources CRDs
- Add test infra
- Add CONTRIBUTING guidelines for development
- Add e2e tests

Signed-off-by: Jason Keene <jkeene@pivotal.io>
@mdemirhan
Copy link

This one is too big to to a proper review. Should we check this in and provide feedback in an async manner?

@jasonkeene
Copy link
Contributor

@mdemirhan We'd be fine with that. Additionally, @dprotaso asked to separate out the vendor/ dir into its own commit to make it easier to review the actual content of the change. You can view the actual changes here:

e28485b

@jasonkeene
Copy link
Contributor

jasonkeene commented Nov 16, 2018

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2018
@jasonkeene
Copy link
Contributor

/approve

@jasonkeene
Copy link
Contributor

Something to be aware of is that the CRDs we originally created were targeted towards using syslog only. As a result we were able to do some custom validation on fields such as host/port:

https://github.com/knative/observability/pull/2/files#diff-135361e4eaf0dcd19d26d6aecc6209b1

This might have to go away when we end up supporting other output types. I imagine elasticsearch and stackdriver are two types we'd want to support shortly. Other types might have different configuration than host, port, enable_tls and insecure_skip_verify. I've put some thought into different structures for the LogSink CRDs that we might want to use but I really like the simplicity of what we currently have.

Any thoughts/feedback on the CRDs alone?

cc/ @dprotaso @mdemirhan

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2018
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@jasonkeene
Copy link
Contributor

jasonkeene commented Nov 19, 2018

As far as the google bot is concerned. I am the author of the new commit and I confirm that I am ok with this commit being contributed to the project.

Not sure how to prevent cla/google from complaining when we push changes in the future.

Copy link
Member

@dprotaso dprotaso left a comment

Choose a reason for hiding this comment

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

Take a look at my comments and let me know if you have any questions!

.gitmodules Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
config/200-roles-sink-controller.yaml Outdated Show resolved Hide resolved
config/500-fluent-bit-daemon.yaml Outdated Show resolved Hide resolved
config/500-fluent-bit-daemon.yaml Outdated Show resolved Hide resolved
config/500-fluent-bit-daemon.yaml Outdated Show resolved Hide resolved
test/e2e/clusterlogsink_test.go Outdated Show resolved Hide resolved
test/e2e/clusterlogsink_test.go Outdated Show resolved Hide resolved
test/e2e/clusterlogsink_test.go Outdated Show resolved Hide resolved
test/e2e/logsink_test.go Outdated Show resolved Hide resolved
@wfernandes
Copy link
Contributor Author

@dprotaso I removed the priorityClassName from the fluent-bit daemonset manifest. We had that in there because we copied it from another fluent-bit k8s manifest. I believe this is the same action taken by knative/serving.

@Benjamintf1
Copy link
Contributor

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2018
@Benjamintf1
Copy link
Contributor

Does it think jason or I don't approve of our commits being in this project? Can anyone fix this check?

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2018
- Improve e2e tests
- Move role bindings to separate files and make file names consistent
- Fix fluent bit labels
- Pin fluent-bit-out-syslog version to v0.9
- Update imagePullPolicy to IfNotPresent for fluent-bit-out-syslog
- Refactor signal handling to use knative/pkg/signals
- Modify controllers to implement cache.ResourceEventHandler interface
  https://godoc.org/k8s.io/client-go/tools/cache#ResourceEventHandler
- Pin version of oratos/crosstalk-receiver in e2e tests to v0.3
- Pin version of ubuntu to Xenial in e2e tests. Xenial is currently OSL approved.
- Remove priorityClassName property for fluent-bit daemonset
@googlebot
Copy link

CLAs look good, thanks!

@Benjamintf1
Copy link
Contributor

Benjamintf1 commented Dec 7, 2018

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2018
@Benjamintf1
Copy link
Contributor

/approve

@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Benjamintf1, jasonkeene, wfernandes

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 [Benjamintf1,jasonkeene,wfernandes]

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

@knative-prow-robot knative-prow-robot merged commit ea3e17e into knative:master Dec 10, 2018
@wfernandes wfernandes deleted the sink-resources branch December 10, 2018 20:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants