*: simplify Watch API by using dynamic resource client by hasbro17 · Pull Request #34 · operator-framework/operator-sdk

hasbro17 · 2018-02-21T21:32:36Z

This PR simplifies the Watch() API by removing the rest client argument. Instead the user now passes in the apiVersion and Kind which can be used to create a dynamic resource client which is used to setup the informer for that resource.

hasbro17 · 2018-02-21T22:15:35Z

Testing out the Informer+dynamic-client Watch implementation separately. Will ping here once it works.

hongchaodeng · 2018-02-21T22:07:42Z

+	"k8s.io/client-go/rest"
+)
+
+// MustNewKubeClientAndConfig returns the in-cluster config and kubernetes client


Better put these func into k8sutil/client.go

hongchaodeng · 2018-02-21T22:10:05Z

+)
+
+// init initializes the restMapper and clientPool needed to create a resource client dynamically
+func init() {


We should not put any side effect into util/ package.
Probably have another package called pkg/k8sclient/

hongchaodeng · 2018-02-21T22:15:25Z

 // obj is an instance of the resource type, e.g. &Pod{}.
-// resourcePluralName is the plural name of the resource, e.g. “pods”.
-// resourceClient is the rest client for the resource, e.g. `kubeclient.CoreV1().RESTClient()`.
+// apiVersion is the APIVersion of the resource, e.g "v1" for pods, or "cache.example.com/v1alpha1" for memcached custom resource


e.g "v1" for pods, or "cache.example.com/v1alpha1" for memcached custom resource

This is understatement of the specification of kube API.
I would suggest to mention at least 3 things:

reference upstream kube API doc: https://kubernetes.io/docs/reference/

describe that apiVersion would be of format "groupName/version" except "core" group. List some examples.

underscore that "core" group would be only "v1". List some examples.

hongchaodeng · 2018-02-21T22:16:15Z

-// resourcePluralName is the plural name of the resource, e.g. “pods”.
-// resourceClient is the rest client for the resource, e.g. `kubeclient.CoreV1().RESTClient()`.
+// apiVersion is the APIVersion of the resource, e.g "v1" for pods, or "cache.example.com/v1alpha1" for memcached custom resource
+// kind is the Kind of the resource, e.g "Pod" for pods, or "Memcached" for memcached custom resource


Give a reference where user can find the kind.
This would be a good reference: https://kubernetes.io/docs/reference/

hongchaodeng · 2018-02-21T22:19:32Z

This closes #16

hasbro17 · 2018-02-22T01:35:47Z

Fixed the informer's Run() to be blocking and not shutdown the queue.
Moved all resource client utils to pkg/k8sclient/client.go
Removed objType from the Watch API since with using the resource client the type will always be *unstructured.Unstructured.

I've tested this implementation of informer+dynamic-client separately and verified that it works in watching Pod, EtcdCluster, VaultService etc as long as the Group,Version and Kind is correct.

@hongchaodeng PTAL.

hongchaodeng · 2018-02-22T03:18:15Z

-// obj is an instance of the resource type, e.g. &Pod{}.
-// resourcePluralName is the plural name of the resource, e.g. “pods”.
-// resourceClient is the rest client for the resource, e.g. `kubeclient.CoreV1().RESTClient()`.
+// apiVersion is the APIVersion of the resource, e.g "v1" for pods, or "cache.example.com/v1alpha1" for memcached custom resource


This previous comment wasn't addressed: #34 (comment)

Paste again:

This is understatement of the specification of kube API.
I would suggest to mention at least 3 things:

reference upstream kube API doc: https://kubernetes.io/docs/reference/

describe that apiVersion would be of format "groupName/version" except "core" group. List some examples.

underscore that "core" group would be only "v1". List some examples.

hongchaodeng · 2018-02-22T03:18:27Z

-// resourcePluralName is the plural name of the resource, e.g. “pods”.
-// resourceClient is the rest client for the resource, e.g. `kubeclient.CoreV1().RESTClient()`.
+// apiVersion is the APIVersion of the resource, e.g "v1" for pods, or "cache.example.com/v1alpha1" for memcached custom resource
+// kind is the Kind of the resource, e.g "Pod" for pods, or "Memcached" for memcached custom resource


Previous comment wasn't addressed: #34 (comment)

Paste again:

Give a reference where user can find the kind.
This would be a good reference: https://kubernetes.io/docs/reference/

hongchaodeng · 2018-02-22T03:19:00Z

+	return resource, nil
+}
+
+// MustNewKubeClientAndConfig returns the in-cluster config and kubernetes client


MustNewKubeClientAndConfig -> mustNewKubeClientAndConfig

hongchaodeng · 2018-02-22T03:19:25Z

+	return kubernetes.NewForConfigOrDie(cfg), cfg
+}
+
+// inClusterConfig returns the in-cluster config accessible to a pod


to a pod -> inside a pod

hongchaodeng · 2018-02-22T03:20:15Z


 	if !cache.WaitForCacheSync(ctx.Done(), i.sharedIndexInformer.HasSynced) {
-		return errors.New("Timed out waiting for caches to sync")
+		logrus.Error("Timed out waiting for caches to sync")


This would slip the error down.
If we can't handle the error here, just panic initially.

fanminshi · 2018-02-22T19:13:45Z

+	}
+	resource := &metav1.APIResource{
+		Name:       mapping.Resource,
+		Namespaced: mapping.Scope == meta.RESTScopeNamespace,


I just curious when will an APIResource not be in a Namespace?

So it's not common but if you watch cluster-wide resources like StorageClass or ClusterRole then those are non-namespaced.

I see. Thanks for the clarification!

hasbro17 · 2018-02-22T19:32:16Z

Fixed. PTAL and double check the Watch() API comment.

fanminshi · 2018-02-22T20:04:25Z

-// resourceClient is the rest client for the resource, e.g. `kubeclient.CoreV1().RESTClient()`.
+// apiVersion for a resource is of the format "Group/Version" except for the "Core" group whose APIVersion is just "v1". For e.g:
+//   - Deployments have Group "apps" and Version "v1beta2" giving the APIVersion "apps/v1beta2"
+//   - Pods have Group "Core" and Version "v1" giving the APIVersion "v1"


Maybe add an example on what's the APIVersion for Custom Resource? I am guessing that is what most user will be using SDK.Watch for.

fanminshi · 2018-02-22T20:04:59Z

+// kind is the Kind of the resource, e.g "Pod" for pods
+// Consult the API reference for the Group, Version and Kind of a resource: https://kubernetes.io/docs/reference/
+// namespace is the Namespace to watch for the resource
 // opts provide more options for doing the watch.


I don't think we have opts yet right? maybe remove it and add later?

fanminshi · 2018-02-22T20:06:08Z

some suggestions around sdk.Watch()'s comment. Then sdk.Watch() interface lgtm.

hasbro17 · 2018-02-22T20:26:42Z

Added the APIVersion example comment for Memcached CR.

hongchaodeng · 2018-02-22T20:44:53Z

LGTM

fanminshi · 2018-02-22T20:46:15Z

lgtm

Currently, scorecard's XUnit output does not conform to any XUnit schema. This makes the scorecard XUnit output conform with the XUnit schema defined by the Jenkins XUnit plugin. This is sample output from running scorecard against the sample Go v3 Memcached operator: ```xml <testsuites name="scorecard"> <testsuite name="olm-bundle-validation-test" tests="1" skipped="0" failures="0" errors="0"> <properties> <property name="spec.image" value="quay.io/operator-framework/scorecard-test:v1.19.0"></property> <property name="spec.entrypoint" value="scorecard-test olm-bundle-validation"></property> <property name="labels.test" value="olm-bundle-validation-test"></property> </properties> <testcase name="olm-bundle-validation" time="0001-01-01T00:00:00Z"> <system-out>time=&operator-framework#34;2022-04-12T19:21:52Z&operator-framework#34; level=debug msg=&operator-framework#34;Found manifests directory&operator-framework#34; name=bundle-test
time=&operator-framework#34;2022-04-12T19:21:52Z&operator-framework#34; level=debug msg=&operator-framework#34;Found metadata directory&operator-framework#34; name=bundle-test
time=&operator-framework#34;2022-04-12T19:21:52Z&operator-framework#34; level=debug msg=&operator-framework#34;Getting mediaType info from manifests directory&operator-framework#34; name=bundle-test
time=&operator-framework#34;2022-04-12T19:21:52Z&operator-framework#34; level=debug msg=&operator-framework#34;Found annotations file&operator-framework#34; name=bundle-test
time=&operator-framework#34;2022-04-12T19:21:52Z&operator-framework#34; level=debug msg=&operator-framework#34;Could not find optional dependencies file&operator-framework#34; name=bundle-test
</system-out> </testcase> </testsuite>  </testsuites> ``` The full output can be recreated by installing the `operator-sdk` tool built from this commit and running scorecard against the sample Go v3 memcached operator: ``` cd /path/to/operator-sdk make install operator-sdk scorecard testdata/go/v3/memcached-operator ``` The XUnit XSD schema can be found here: https://github.com/jenkinsci/xunit-plugin/blob/master/src/main/resources/org/jenkinsci/plugins/xunit/types/model/xsd/junit-10.xsd The tool used to validate the output against the schema can be found here: https://www.freeformatter.com/xml-validator-xsd.html Signed-off-by: Ryan King <ryking@redhat.com>

Currently, scorecard's XUnit output does not conform to any XUnit schema. This makes the scorecard XUnit output conform with the XUnit schema defined by the Jenkins XUnit plugin. This is sample output from running scorecard against the sample Go v3 Memcached operator: ```xml <testsuites name="scorecard"> <testsuite name="olm-bundle-validation-test" tests="1" skipped="0" failures="0" errors="0"> <properties> <property name="spec.image" value="quay.io/operator-framework/scorecard-test:v1.19.0"></property> <property name="spec.entrypoint" value="scorecard-test olm-bundle-validation"></property> <property name="labels.test" value="olm-bundle-validation-test"></property> </properties> <testcase name="olm-bundle-validation" time="0001-01-01T00:00:00Z"> <system-out>time="2022-04-12T19:21:52Z" level=debug msg="Found manifests directory" name=bundle-test
time="2022-04-12T19:21:52Z" level=debug msg="Found metadata directory" name=bundle-test
time="2022-04-12T19:21:52Z" level=debug msg="Getting mediaType info from manifests directory" name=bundle-test
time="2022-04-12T19:21:52Z" level=debug msg="Found annotations file" name=bundle-test
time="2022-04-12T19:21:52Z" level=debug msg="Could not find optional dependencies file" name=bundle-test
</system-out> </testcase> </testsuite>  </testsuites> ``` The full output can be recreated by installing the `operator-sdk` tool built from this commit and running scorecard against the sample Go v3 memcached operator: ``` cd /path/to/operator-sdk make install operator-sdk scorecard testdata/go/v3/memcached-operator ``` The XUnit XSD schema can be found here: https://github.com/jenkinsci/xunit-plugin/blob/master/src/main/resources/org/jenkinsci/plugins/xunit/types/model/xsd/junit-10.xsd The tool used to validate the output against the schema can be found here: https://www.freeformatter.com/xml-validator-xsd.html Signed-off-by: Ryan King <ryking@redhat.com>

V0.15.1 merge

…perator-framework#34

dep: add constraints

b47f869

hongchaodeng reviewed Feb 21, 2018

View reviewed changes

hongchaodeng mentioned this pull request Feb 21, 2018

pkg/generator: add auto-gen for apis/../register.go #36

Merged

hasbro17 changed the title *: simplify Watch API by using dynamic resource client [WIP]*: simplify Watch API by using dynamic resource client Feb 22, 2018

hasbro17 changed the title ~~[WIP]*: simplify Watch API by using dynamic resource client~~ *: simplify Watch API by using dynamic resource client Feb 22, 2018

hongchaodeng reviewed Feb 22, 2018

View reviewed changes

fanminshi reviewed Feb 22, 2018

View reviewed changes

*: create dynamic client for watching a resource

326984e

hongchaodeng merged commit 1663ab7 into operator-framework:master Feb 22, 2018

hongchaodeng deleted the haseeb/use-dynamic-client-for-watch branch February 22, 2018 20:50

fanminshi mentioned this pull request Feb 22, 2018

Action Items #10

Closed

21 tasks

m1kola pushed a commit to m1kola/operator-sdk that referenced this pull request Jun 7, 2024

Merge pull request operator-framework#34 from fabianvf/v0.15.0-merge

a497e0f

V0.15.1 merge

m1kola pushed a commit to m1kola/operator-sdk that referenced this pull request Jun 7, 2024

internal/scaffold/helm/entrypoint.go: resolve merge of this file from o…

e58323f

…perator-framework#34

Conversation

hasbro17 commented Feb 21, 2018

Uh oh!

hasbro17 commented Feb 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongchaodeng Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongchaodeng commented Feb 21, 2018

Uh oh!

hasbro17 commented Feb 22, 2018

Uh oh!

hongchaodeng Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongchaodeng Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongchaodeng Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasbro17 commented Feb 22, 2018

Uh oh!

fanminshi Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanminshi commented Feb 22, 2018

Uh oh!

hasbro17 commented Feb 22, 2018

Uh oh!

hongchaodeng commented Feb 22, 2018

Uh oh!

fanminshi commented Feb 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hongchaodeng Feb 21, 2018 •

edited

Loading

hongchaodeng Feb 22, 2018 •

edited

Loading

hongchaodeng Feb 22, 2018 •

edited

Loading

hongchaodeng Feb 22, 2018 •

edited

Loading

fanminshi Feb 22, 2018 •

edited

Loading