Issue #364 Add Support to Kafka Source for Resource spec #399

lberk · 2019-05-07T00:29:10Z

As mentioned in Issue #364 , this is experimenting adding resource spec for

resources.limits.cpu
resources.limits.memory
resources.requests.cpu
resources.requests.memory

As part of this change, we've moved the cmp test to kmp (was hitting errors similar to those seen in Serving 2622), Matching the approach in the serving repo.

knative-prow-robot · 2019-05-07T00:29:24Z

Hi @lberk. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

matzew · 2019-05-07T05:21:31Z

/ok-to-test

matzew · 2019-05-07T05:31:45Z

In the config/300-kafkasource.yaml, let's add this:

            resources:
              properties:
                limits:
                  properties:
                    cpu:
                      type: string
                    memory:
                      type: string
                  type: object
                requests:
                  properties:
                    cpu:
                      type: string
                    memory:
                      type: string
                  type: object
              type: object

you can generate that file (but CAUTION) with:

cd contrib/kafka
go run ../../vendor/sigs.k8s.io/controller-tools/cmd/controller-gen/main.go all 
mv config/crds/sources_v1alpha1_kafkasource.yaml config/300-kafkasource.yaml

but only ... pick the relevant diff ...

I'd also suggest adding this new feature to the samples, to make it a little bit more visible

matzew · 2019-05-07T06:59:47Z

Regarding the coverage - let's add a test that does NOT specify the Resource Limits, so that we can test if the actual defaults are applied:

							Resources: corev1.ResourceRequirements{
								Limits: corev1.ResourceList{
									corev1.ResourceCPU:    resource.MustParse("100m"),
									corev1.ResourceMemory: resource.MustParse("500Mi"),
								},
								Requests: corev1.ResourceList{
									corev1.ResourceCPU:    resource.MustParse("10m"),
									corev1.ResourceMemory: resource.MustParse("10Mi"),
								},
							},

than we get back to 100% :)

matzew · 2019-05-07T09:14:39Z

I deployed a source, no specific setting (assuming defaults), doing k get kafkasources kafka-source -oyaml gives me:

apiVersion: sources.eventing.knative.dev/v1alpha1
kind: KafkaSource
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"sources.eventing.knative.dev/v1alpha1","kind":"KafkaSource","metadata":{"annotations":{},"name":"kafka-source","namespace":"default"},"spec":{"bootstrapServers":"my-cluster-kafka-bootstrap.kafka:9092","consumerGroup":"my-group","sink":{"apiVersion":"serving.knative.dev/v1alpha1","kind":"Service","name":"event-display"},"topics":"my-topic"}}
  creationTimestamp: "2019-05-07T09:11:12Z"
  generation: 2
  name: kafka-source
  namespace: default
  resourceVersion: "21585"
  selfLink: /apis/sources.eventing.knative.dev/v1alpha1/namespaces/default/kafkasources/kafka-source
  uid: 0f203a34-70a8-11e9-9cdb-e452b3720906
spec:
  bootstrapServers: my-cluster-kafka-bootstrap.kafka:9092
  consumerGroup: my-group
  net:
    sasl:
      password: {}
      user: {}
    tls: {}
  resources:
    limits: {}
    requests: {}
  sink:
    apiVersion: serving.knative.dev/v1alpha1
    kind: Service
    name: event-display
  topics: my-topic

Expected ?

matzew · 2019-05-07T09:15:20Z

looking in the underlying deployment, I do indeed see the defaults:

...
        resources:
          limits:
            cpu: 100m
            memory: 100M
          requests:
            cpu: 10m
            memory: 10M
...

matzew · 2019-05-07T09:19:02Z

Now, updating/changing a running source (by explicitly adding NON-DEFAULT settings) and runningk get kafkasources kafka-source -oyaml

gives me:

apiVersion: sources.eventing.knative.dev/v1alpha1
kind: KafkaSource
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"sources.eventing.knative.dev/v1alpha1","kind":"KafkaSource","metadata":{"annotations":{},"name":"kafka-source","namespace":"default"},"spec":{"bootstrapServers":"my-cluster-kafka-bootstrap.kafka:9092","consumerGroup":"my-group","resources":{"limits":{"cpu":"200m","memory":"200M"},"requests":{"cpu":"20m","memory":"20M"}},"sink":{"apiVersion":"serving.knative.dev/v1alpha1","kind":"Service","name":"event-display"},"topics":"my-topic"}}
  creationTimestamp: "2019-05-07T09:11:12Z"
  generation: 3
  name: kafka-source
  namespace: default
  resourceVersion: "22294"
  selfLink: /apis/sources.eventing.knative.dev/v1alpha1/namespaces/default/kafkasources/kafka-source
  uid: 0f203a34-70a8-11e9-9cdb-e452b3720906
spec:
  bootstrapServers: my-cluster-kafka-bootstrap.kafka:9092
  consumerGroup: my-group
  net:
    sasl:
      password: {}
      user: {}
    tls: {}
  resources:
    limits:
      cpu: 200m
      memory: 200M
    requests:
      cpu: 20m
      memory: 20M
  sink:

But looking at the underlying deployment I still see:

        resources:
          limits:
            cpu: 100m
            memory: 100M
          requests:
            cpu: 10m
            memory: 10M

related: #400

matzew · 2019-05-07T09:41:09Z

Now, creating a new source, where I set values, like:

  resources:
    limits:
      cpu: 200m
      memory: 200Mi
    requests:
      cpu: 200m
      memory: 200Mi

It's all correct in the kafkasource AND the underlying deployment

contrib/kafka/pkg/reconciler/resources/receive_adapter.go

matzew · 2019-05-07T10:10:32Z

For the defaults, lets use: requests": {"memory": "512Mi", "cpu": "250m"}, "limits": {"memory": "512Mi", "cpu": "250m"}} we use this as our setting currently

On Tue 7. May 2019 at 07:23, covbot ***@***.***> wrote: The following is the coverage report on pkg/. Say /test pull-knative-eventing-sources-go-coverage to re-run this coverage report File Old Coverage New Coverage Delta contrib/kafka/pkg/reconciler/resources/receive_adapter.go <https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_eventing-sources/399/pull-knative-eventing-sources-go-coverage/1125631815193726979/artifacts/line-cov.html#file1> 100.0% 80.0% -20.0 — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#399 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABGPTSJQRDZELJPIHQ2T6TPUEG6XANCNFSM4HLEMG7Q> .

-- Sent from Gmail Mobile

lberk · 2019-05-07T20:39:23Z

Updated as requested, and rebased on master

config/300-kafkasource.yaml - add in resource properties
pkg/reconcilier/resources/receive_adapter.go - increase the defaults to proper values
pkg/reconcilier/resources/receive_adapter_test.go - add second testcase (we should probably create a proper test struct in the future)
samples/event-sources.yaml - add in an example resource specification

matzew · 2019-05-08T08:06:12Z

deploying a source, without the resource config, I still see it empty, when running k get kafkasources kafka-source -oyaml I am getting:

...
spec:
  bootstrapServers: my-cluster-kafka-bootstrap.kafka:9092
  consumerGroup: new
  net:
    sasl:
      password: {}
      user: {}
    tls: {}
  resources:
    limits: {}
    requests: {}
  sink:
    apiVersion: serving.knative.dev/v1alpha1
    kind: Service
    name: event-display
  topics: my-topic

I'd expect the sepc.resources would tell the actual defaults

matzew · 2019-05-22T16:02:42Z

/lgtm
/approve

Harwayne

deploying a source, without the resource config, I still see it empty, ... I'd expect the sepc.resources would tell the actual defaults

I think we normally achieve that through Webhook defaulting. I don't think I've seen a reconciler manipulate the spec of the resource it is reconciling (although I'm sure someone somewhere does it).

contrib/kafka/pkg/reconciler/resources/receive_adapter.go

contrib/kafka/pkg/apis/sources/v1alpha1/kafka_types.go

knative-metrics-robot · 2019-05-24T14:43:48Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-sources-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
contrib/kafka/pkg/reconciler/kafkasource.go	89.1%	87.3%	-1.7

Add the kafka_types for resources requests and limits (cpu, memory) which will translate to the deployment template as resources.limits.cpu resources.limits.memory resources.requests.cpu resources.requests.memory Use kmp for testing intead of cmp. As seen in serving issue #2622, we should use knative/pkg/kmp (otherwise we get unit test errors) Update the Gopkg.lock file accordingly

config/300-kafkasource.yaml - add in resource properties pkg/reconcilier/resources/receive_adapter.go - increase the defaults to proper values pkg/reconcilier/resources/receive_adapter_test.go - add second testcase (we should probably create a proper test struct in the future) samples/event-sources.yaml - add in an example resource specification pkg/apis/sources/v1alpha1/kafka_types - Fix spelling mistake caught by sockpuppet Readd cmp for testing/building

…rements" This reverts commit 76ffab5.

As per feedback, `kubectl get kafkasource -oyaml` should show some status regarding the correctness of the resources supplied, and if they were correctly parsed. Add this, as well as a description/error message for which resource failed (and why). Update kafkasource_test accordingly

knative-metrics-robot · 2019-05-29T23:52:02Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-sources-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
contrib/kafka/pkg/apis/sources/v1alpha1/kafka_types.go	93.3%	82.4%	-11.0
contrib/kafka/pkg/reconciler/kafkasource.go	89.1%	83.8%	-5.3

knative-metrics-robot · 2019-05-29T23:53:07Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-sources-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
contrib/kafka/pkg/apis/sources/v1alpha1/kafka_types.go	93.3%	82.4%	-11.0
contrib/kafka/pkg/reconciler/kafkasource.go	89.1%	83.8%	-5.3

Harwayne

/lgtm

contrib/kafka/pkg/reconciler/resources/receive_adapter.go

matzew · 2019-05-31T07:36:40Z

/approve

matzew · 2019-05-31T07:38:41Z

@n3wscott or @grantr This PR was /lgtm'd by @Harwayne, and I could, in theory, approve it.

However, since it also updates non Kafka file (Gopkg.lock), my /approve is not good enough :-(

evankanderson

/approve

Thanks to @Harwayne and @danp for asking the question I was going to ask.

knative-prow-robot · 2019-05-31T18:17:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, lberk, matzew

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~~ [evankanderson]

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

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 7, 2019

knative-prow-robot requested review from grantr and matzew May 7, 2019 00:29

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 7, 2019

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 7, 2019

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 7, 2019

matzew reviewed May 7, 2019

View reviewed changes

contrib/kafka/pkg/reconciler/resources/receive_adapter.go Outdated Show resolved Hide resolved

matzew reviewed May 7, 2019

View reviewed changes

contrib/kafka/pkg/reconciler/resources/receive_adapter.go Outdated Show resolved Hide resolved

matzew reviewed May 7, 2019

View reviewed changes

contrib/kafka/pkg/reconciler/resources/receive_adapter.go Outdated Show resolved Hide resolved

lberk force-pushed the gh364 branch from f7cc3e9 to a483bc5 Compare May 7, 2019 20:37

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 7, 2019

lberk changed the title ~~Issue #365 Add Support to Kafka Source for Resource spec~~ Issue #364 Add Support to Kafka Source for Resource spec May 7, 2019

lberk force-pushed the gh364 branch from a483bc5 to 04e8abd Compare May 21, 2019 19:26

mattmoor-sockpuppet reviewed May 21, 2019

View reviewed changes

lberk force-pushed the gh364 branch from 04e8abd to 0a25edb Compare May 21, 2019 19:29

mattmoor-sockpuppet reviewed May 21, 2019

View reviewed changes

lberk force-pushed the gh364 branch from 0a25edb to 19a37b5 Compare May 21, 2019 19:30

lberk force-pushed the gh364 branch from 751ceb5 to f315193 Compare May 21, 2019 20:17

knative-prow-robot assigned matzew May 22, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2019

Harwayne reviewed May 22, 2019

View reviewed changes

contrib/kafka/pkg/reconciler/resources/receive_adapter.go Show resolved Hide resolved

contrib/kafka/pkg/apis/sources/v1alpha1/kafka_types.go Show resolved Hide resolved

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 24, 2019

lberk added 7 commits May 29, 2019 19:46

Fix rebase/merge removing Kafka SASL cert/key/cacert bits

f1fe341

Update underlying deployment by reconciling expected changes

b587770

Per feedback change KafkaResourceSpec to corev1.ResourceRequirements

caf2c12

Revert "Per feedback change KafkaResourceSpec to corev1.ResourceRequi…

4d1b988

…rements" This reverts commit 76ffab5.

lberk force-pushed the gh364 branch from 37df1a3 to 181df29 Compare May 29, 2019 23:48

mattmoor-sockpuppet reviewed May 29, 2019

View reviewed changes

Spelling Mistake Fix

c0d5253

knative-prow-robot assigned Harwayne May 30, 2019

Harwayne reviewed May 30, 2019

View reviewed changes

contrib/kafka/pkg/reconciler/resources/receive_adapter.go Show resolved Hide resolved

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 30, 2019

evankanderson reviewed May 31, 2019

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2019

knative-prow-robot merged commit 0743eb4 into knative:master May 31, 2019

matzew mentioned this pull request Sep 10, 2019

Adding matzew to OWNERS #582

Merged

lberk mentioned this pull request May 21, 2020

Add lberk to approvers knative/eventing#3165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #364 Add Support to Kafka Source for Resource spec #399

Issue #364 Add Support to Kafka Source for Resource spec #399

lberk commented May 7, 2019 •

edited

Loading

knative-prow-robot commented May 7, 2019

matzew commented May 7, 2019

matzew commented May 7, 2019

matzew commented May 7, 2019

matzew commented May 7, 2019

matzew commented May 7, 2019

matzew commented May 7, 2019 •

edited

Loading

matzew commented May 7, 2019

matzew commented May 7, 2019 via email

lberk commented May 7, 2019

matzew commented May 8, 2019

matzew commented May 22, 2019

Harwayne left a comment

knative-metrics-robot commented May 24, 2019

knative-metrics-robot commented May 29, 2019

knative-metrics-robot commented May 29, 2019

Harwayne left a comment

matzew commented May 31, 2019

matzew commented May 31, 2019

evankanderson left a comment

knative-prow-robot commented May 31, 2019

Issue #364 Add Support to Kafka Source for Resource spec #399

Issue #364 Add Support to Kafka Source for Resource spec #399

Conversation

lberk commented May 7, 2019 • edited Loading

knative-prow-robot commented May 7, 2019

matzew commented May 7, 2019

matzew commented May 7, 2019

matzew commented May 7, 2019

matzew commented May 7, 2019

matzew commented May 7, 2019

matzew commented May 7, 2019 • edited Loading

matzew commented May 7, 2019

matzew commented May 7, 2019 via email

lberk commented May 7, 2019

matzew commented May 8, 2019

matzew commented May 22, 2019

Harwayne left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented May 24, 2019

knative-metrics-robot commented May 29, 2019

knative-metrics-robot commented May 29, 2019

Harwayne left a comment

Choose a reason for hiding this comment

matzew commented May 31, 2019

matzew commented May 31, 2019

evankanderson left a comment

Choose a reason for hiding this comment

knative-prow-robot commented May 31, 2019

lberk commented May 7, 2019 •

edited

Loading

matzew commented May 7, 2019 •

edited

Loading