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

Add support for cluster-scoped CRDs for kube-state-metrics #2809

Merged
merged 10 commits into from
Apr 10, 2020

Conversation

raffaelespazzoli
Copy link
Contributor

Signed-off-by: Raffaele Spazzoli [email protected]

this should fix #2707
it introspects the passed gvks to see if they are namespaced. Logic remains the same for namespaced ones, for cluster scoped CRDs, a different watch is built to list/watch at the cluster level.

@camilamacedo86

Description of the change:

it introspects the passed gvks to see if they are namespaced. Logic remains the same for namespaced ones, for cluster scoped CRDs, a different watch is built to list/watch at the cluster level.

Motivation for the change:

enable to produce metrics for cluster scoped CRDs.

@camilamacedo86 camilamacedo86 requested review from joelanford and removed request for shawn-hurley April 9, 2020 14:17
@joelanford joelanford changed the title should fix #2707 Add support for cluster-scoped CRDs for kube-state-metrics Apr 9, 2020
@raffaelespazzoli
Copy link
Contributor Author

@joelanford I don't see the checks making progress, could it be that your renaming the issue has broken the integration with travis-ci?

@raffaelespazzoli
Copy link
Contributor Author

raffaelespazzoli commented Apr 9, 2020

@joelanford it has happened again: the task: continuous-integration/travis-ci seems to be hung and I can't follow the link to it to check what is going on.
is there a way to relaunch all the checks?

@camilamacedo86
Copy link
Contributor

camilamacedo86 commented Apr 9, 2020

try to close and re-open the pr @raffaelespazzoli
usually it works.

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Just a few more suggestions.

Can you also add a few lines to the CHANGELOG:

  • Under "Deprecated":

    Deprecated `pkg/kubemetrics.NewMetricsStores`. Use `pkg/kubemetrics.NewNamespacedMetricsStores` instead. ([#2809](https://github.com/operator-framework/operator-sdk/pull/2809))

  • Under "Added":

    Added support for generating kube-state-metrics metrics for cluster-scoped resources. Also added `pkg/kubemetrics.NewNamespacedMetricsStores` and `pkg/kubemetrics.NewClusterScopedMetricsStores` to support this new feature. ([#2809](https://github.com/operator-framework/operator-sdk/pull/2809))

Looks good to me after addressing these suggestions. Thanks so much for contributing this fix!

pkg/kube-metrics/store.go Show resolved Hide resolved
@raffaelespazzoli
Copy link
Contributor Author

@joelanford
what's the policy on error and log messages. It keep failing on those things. and I can't find any mention on this in the style guide.

Checking format of error and log messages...
Error messages do not begin with lower case:
/home/travis/gopath/src/github.com/operator-framework/operator-sdk/pkg/kube-metrics/metrics.go:142:	return false, errors.New("Unable to find type: " + gvk.String() + " in server")
Makefile:209: recipe for target 'test-sanity' failed
make: *** [test-sanity] Error 255

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 9, 2020
CHANGELOG.md Outdated
@@ -2,8 +2,8 @@

### Added

- Added the [`generate csv --deploy-dir --apis-dir --crd-dir`](website/content/en/docs/cli/operator-sdk_generate_csv.md#options) flags to allow configuring input locations for operator manifests and API types directories to the CSV generator in lieu of a config. See the CLI reference doc or `generate csv -h` help text for more details. ([#2511](https://github.com/operator-framework/operator-sdk/pull/2511))
- Added the [`generate csv --output-dir`](website/content/en/docs/cli/operator-sdk_generate_csv.md#options) flag to allow configuring the output location for the catalog directory. ([#2511](https://github.com/operator-framework/operator-sdk/pull/2511))
- Added the [`generate csv --deploy-dir --apis-dir --crd-dir`](doc/cli/operator-sdk_generate_csv.md#options) flags to allow configuring input locations for operator manifests and API types directories to the CSV generator in lieu of a config. See the CLI reference doc or `generate csv -h` help text for more details. ([#2511](https://github.com/operator-framework/operator-sdk/pull/2511))
Copy link
Contributor

@camilamacedo86 camilamacedo86 Apr 9, 2020

Choose a reason for hiding this comment

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

We just need to fix the changelog.
The links changed and then it will not pass in the CI because they are broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Added the [`generate csv --deploy-dir --apis-dir --crd-dir`](doc/cli/operator-sdk_generate_csv.md#options) flags to allow configuring input locations for operator manifests and API types directories to the CSV generator in lieu of a config. See the CLI reference doc or `generate csv -h` help text for more details. ([#2511](https://github.com/operator-framework/operator-sdk/pull/2511))
- Added the [`generate csv --deploy-dir --apis-dir --crd-dir`](website/content/en/docs/cli/operator-sdk_generate_csv.md#options) flags to allow configuring input locations for operator manifests and API types directories to the CSV generator in lieu of a config. See the CLI reference doc or `generate csv -h` help text for more details. ([#2511](https://github.com/operator-framework/operator-sdk/pull/2511))

Signed-off-by: Raffaele Spazzoli <[email protected]>
Signed-off-by: Raffaele Spazzoli <[email protected]>
Signed-off-by: Raffaele Spazzoli <[email protected]>
Signed-off-by: Raffaele Spazzoli <[email protected]>
fixed log messages per @camilamacedo86

Signed-off-by: Raffaele Spazzoli <[email protected]>
Signed-off-by: Raffaele Spazzoli <[email protected]>
@raffaelespazzoli
Copy link
Contributor Author

I give up, now it's asking to have error message with lower case:

Checking format of error and log messages...
Error messages do not begin with lower case:
/home/travis/gopath/src/github.com/operator-framework/operator-sdk/pkg/kube-metrics/metrics.go:142:	return false, errors.New("Unable to find type: " + gvk.String() + " in server")
Makefile:209: recipe for target 'test-sanity' failed
make: *** [test-sanity] Error 255
The command "make test-sanity" exited with 2.

you have to fix your sanity check because it's driving me insane :)

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Tested Locally with CRD and Cluster-Scope:

$ kubectl logs deployment.apps/memcached-operator -n memcached
{"level":"info","ts":1586476933.6907554,"logger":"cmd","msg":"Operator Version: 0.0.1"}
{"level":"info","ts":1586476933.6909409,"logger":"cmd","msg":"Go Version: go1.13.3"}
{"level":"info","ts":1586476933.6910954,"logger":"cmd","msg":"Go OS/Arch: linux/amd64"}
{"level":"info","ts":1586476933.691209,"logger":"cmd","msg":"Version of operator-sdk: v0.16.0+git"}
{"level":"info","ts":1586476933.691481,"logger":"leader","msg":"Trying to become the leader."}
{"level":"info","ts":1586476934.0657084,"logger":"leader","msg":"No pre-existing lock was found."}
{"level":"info","ts":1586476934.0715778,"logger":"leader","msg":"Became the leader."}
{"level":"info","ts":1586476934.430387,"logger":"controller-runtime.metrics","msg":"metrics server is starting to listen","addr":"0.0.0.0:8383"}
{"level":"info","ts":1586476934.4311326,"logger":"cmd","msg":"Registering Components."}
{"level":"info","ts":1586476935.1817207,"logger":"metrics","msg":"Metrics Service object created","Service.Name":"memcached-operator-metrics","Service.Namespace":"memcached"}
{"level":"info","ts":1586476935.5474622,"logger":"cmd","msg":"Starting the Cmd."}
{"level":"info","ts":1586476935.548912,"logger":"controller-runtime.manager","msg":"starting metrics server","path":"/metrics"}
{"level":"info","ts":1586476935.548941,"logger":"controller-runtime.controller","msg":"Starting EventSource","controller":"memcached-controller","source":"kind source: /, Kind="}
{"level":"info","ts":1586476935.6514418,"logger":"controller-runtime.controller","msg":"Starting EventSource","controller":"memcached-controller","source":"kind source: /, Kind="}
{"level":"info","ts":1586476935.7533202,"logger":"controller-runtime.controller","msg":"Starting EventSource","controller":"memcached-controller","source":"kind source: /, Kind="}
{"level":"info","ts":1586476935.8547926,"logger":"controller-runtime.controller","msg":"Starting Controller","controller":"memcached-controller"}
{"level":"info","ts":1586476935.8550828,"logger":"controller-runtime.controller","msg":"Starting workers","controller":"memcached-controller","worker count":1}
{"level":"info","ts":1586476935.8556492,"logger":"controller_memcached","msg":"Reconciling Memcached.","Request.Namespace":"","Request.Name":"example-memcached"}

We just need to fix the rebase/conflict in the changelog to pass in the CI.
Great work 👍

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2020
CHANGELOG.md Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

CHANGELOG.md Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 merged commit ccdf920 into operator-framework:master Apr 10, 2020
@raffaelespazzoli raffaelespazzoli deleted the fix#2707 branch April 10, 2020 13:37
estroz pushed a commit to estroz/operator-sdk that referenced this pull request Apr 15, 2020
…framework#2809)

Cluster-scoped CRD’s were not supported in the `pkg/kube-metrics` implementation since it was always looking for the GKV's in the List of namespaces informed which cause the issue "resource not found" since the CRD is not in any specific namespace. 

With the changes made here `pkg/kube-metrics` is able to List and Watch cluster-scoped CRDs.  Closes: operator-framework#2707
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues faced when the operator and CRD are cluster-scoped in the metrics
4 participants