Kafka source image policy #303

aslom · 2019-03-27T19:33:49Z

Solves #299

aslom · 2019-03-27T19:49:40Z

knative-prow-robot · 2019-03-27T19:49:41Z

@aslom: GitHub didn't allow me to assign the following users: dubee.

Note that only knative members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @dubee

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.

aslom · 2019-03-27T19:49:51Z

/assign @matzew

…_policy # Conflicts: # contrib/kafka/pkg/reconciler/kafkasource.go

bbrowning · 2019-03-27T20:20:44Z

When would we ever want the policy to be Always instead of IfNotPresent?

bbrowning · 2019-03-27T20:22:23Z

Copied from #299 since the comment is really about the implementation here that exposes a config option:

Why make this an option instead of just changing it from Always to IfNotPresent? Won't this reference an image reference be an image digest once we cut a release, and thus IfNotPresent sufficient?

aslom · 2019-03-27T20:25:32Z

@bbrowning you may want different policies depending on your k8s policies, for example how you do upgrades?

bbrowning · 2019-03-27T20:31:10Z

If you look at our generated release.yaml files, you'll see image references like:

gcr.io/knative-releases/github.com/knative/eventing-sources/cmd/github_receive_adapter@sha256:175407a69f62339e56e077ac87f508db4fb873653a68b03eab6b7cb3eea8e507

Those are image digests, and thus unique. IfNotPresent seems like the sensible default and I can't see why you'd ever want to change that to Always?

aslom · 2019-03-27T20:48:24Z

BTW: wit this PR default is for imagePullPolicy is IfNotPresent and you can override not only to Always but also to Never

matzew · 2019-03-27T21:00:18Z

wasnt that changed to IfNotPresent? I think we talked about that. If not, send a PR ;-)

On Wed 27. Mar 2019 at 21:48, Aleksander Slominski ***@***.***> wrote: BTW: wit this PR default is for imagePullPolicy is IfNotPresent and you can override not only to Always but also to Never — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#303 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJnzhdix9QMlu8hy_8lsYsOwsOP5tk8ks5va9kZgaJpZM4cOnYn> .

-- Sent from Gmail Mobile

aslom · 2019-03-28T14:01:09Z

@matzew @dubee I changed default form Always to IfNotPresent both for adapter and for controller. I tested with minikube and standalone k8s and worked in both.

lionelvillard · 2019-03-28T14:18:00Z

the default value for imagePullPolicy is IfNotPresent when the image tag is not latest. Maybe imagePullPolicy shouldn't be specified to allow k8s to do the right thing when the image tag latest (which is Always)?

dubee

@bbrowning, I was using an image pull policy of always for development purposes. After updating the event source locally and publishing it to my container registry, my cluster didn't always seem to pull down the updated image when I deployed a new event source to test my changes.

dubee · 2019-03-28T14:34:07Z

the default value for imagePullPolicy is IfNotPresent when the image tag is not latest. Maybe imagePullPolicy shouldn't be specified to allow k8s to do the right thing when the image tag latest (which is Always)?

Perhaps this solves the problem. 👍

aslom · 2019-03-28T14:38:33Z

contrib/kafka/config/500-controller.yaml

@@ -32,10 +32,12 @@ spec:
      containers:
      - name: manager
        image: github.com/knative/eventing-sources/contrib/kafka/cmd/controller
-        imagePullPolicy: Always
+        imagePullPolicy: IfNotPresent


I could remove it instead of making IfNotPresent as I think k8s default policy is Always - do you want it to be removed? @matzew @dubee @lionelvillard

This will break integration tests

aslom · 2019-03-28T14:40:34Z

contrib/kafka/config/500-controller.yaml

        env:
        - name: KAFKA_RA_IMAGE
          value: github.com/knative/eventing-sources/contrib/kafka/cmd/receive_adapter
+        - name: KAFKA_RA_IMAGE_PULL_POLLICY


If that KAFKA_RA_IMAGE_PULL_POLLICY env variable is not present then IfNotPresent is used - I think ability to configure image pull policy for receive adapter is good? @dubee @matzew

aslom · 2019-03-28T14:41:49Z

@dubee @matzew @lionelvillard @bbrowning I have added comments to lines of code above showing changes about image policy - please comment what you think is missing/need changing.

aslom · 2019-03-28T15:01:56Z

Here is what I found in k8s docs - the default pull policy is IfNotPresent for non latest tagged images:
https://kubernetes.io/docs/concepts/containers/images/

Considering how complex it is making receiver adapter image pull policy configurable is good? 5 separate cases:
https://kubernetes.io/docs/concepts/configuration/overview/#container-images

bbrowning · 2019-03-28T19:42:38Z

@bbrowning, I was using an image pull policy of always for development purposes. After updating the event source locally and publishing it to my container registry, my cluster didn't always seem to pull down the updated image when I deployed a new event source to test my changes.

Perhaps you were using tags and not digests to reference your image? Knative releases specifically use digests for all image references to avoid this issue.

aslom · 2019-04-03T17:20:58Z

@bbrowning @dubee I made the changes - image pull policy setting is now removed form kafka source

dubee

/lgtm

knative-prow-robot · 2019-04-04T16:54:14Z

@dubee: changing LGTM is restricted to assignees, and only knative/eventing-sources repo collaborators may be assigned issues.

In response to this:

/lgtm

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-04-09T14:18:35Z

@aslom what about: https://github.com/knative/eventing-sources/blob/8ff3a26a58debffb0225bb7afeb932d12f27115a/contrib/kafka/pkg/reconciler/resources/receive_adapter_test.go#L97

@lberk ^

PASS ok github.com/knative/eventing-sources/contrib/kafka/pkg/reconciler 0.049s

…_policy

aslom · 2019-04-09T17:27:30Z

@matzew @lberk fixed missing unit test

PASS
ok  	github.com/knative/eventing-sources/contrib/kafka/pkg/reconciler	0.049s

matzew · 2019-04-09T17:37:36Z

/lgtm

matzew · 2019-04-09T17:38:23Z

/ok-to-test

matzew · 2019-04-09T17:41:02Z

/test pull-knative-eventing-sources-integration-tests

aslom · 2019-04-09T17:46:52Z

/test pull-knative-eventing-sources-integration-tests

aslom · 2019-04-09T20:21:36Z

/test pull-knative-eventing-sources-integration-tests

aslom · 2019-04-09T20:22:06Z

/ok-to-test

matzew · 2019-04-10T05:22:31Z

/approve

matzew · 2019-04-10T05:22:51Z

@n3wscott mind doing an approve ?

n3wscott · 2019-04-10T18:30:03Z

/approve

knative-prow-robot · 2019-04-10T18:30:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aslom, dubee, matzew, n3wscott

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

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

aslom and others added 4 commits March 15, 2019 10:21

Merge remote-tracking branch 'knative/master'

731a6e7

Merge remote-tracking branch 'knative/master'

3a1d847

Merge remote-tracking branch 'knative/master'

c4a7041

make image pull policy configurable

e42e43f

Solves knative#299

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

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

knative-prow-robot requested review from evankanderson and vaikas March 27, 2019 19:34

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 Mar 27, 2019

knative-prow-robot assigned matzew Mar 27, 2019

Merge remote-tracking branch 'knative/master' into kafka_source_image…

9931314

…_policy # Conflicts: # contrib/kafka/pkg/reconciler/kafkasource.go

forcing merge commit

d5c5d65

dubee reviewed Mar 28, 2019

View reviewed changes

aslom commented Mar 28, 2019

View reviewed changes

Remove functionality to set image pull policy for receive adapter

2ed4931

knative-prow-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 3, 2019

aslom added 2 commits April 3, 2019 13:11

remove imagePullPolicy also from controller

df78731

reverse formatting changes

c424594

aslom mentioned this pull request Apr 3, 2019

Allow to set imagePullPolicy for knative sources #339

Closed

dubee approved these changes Apr 4, 2019

View reviewed changes

aslom added 2 commits April 9, 2019 13:23

fix failing unit test

2d7cdee

PASS ok github.com/knative/eventing-sources/contrib/kafka/pkg/reconciler 0.049s

Merge remote-tracking branch 'knative/master' into kafka_source_image…

671b5c9

…_policy

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 9, 2019

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

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

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

knative-prow-robot merged commit 7396a3e into knative:master Apr 10, 2019

aslom mentioned this pull request Apr 17, 2019

Apache Kafka event source does not work with minikube local docker #299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kafka source image policy #303

Kafka source image policy #303

aslom commented Mar 27, 2019

aslom commented Mar 27, 2019

knative-prow-robot commented Mar 27, 2019

aslom commented Mar 27, 2019

bbrowning commented Mar 27, 2019 •

edited

Loading

bbrowning commented Mar 27, 2019

aslom commented Mar 27, 2019

bbrowning commented Mar 27, 2019

aslom commented Mar 27, 2019

matzew commented Mar 27, 2019 via email

aslom commented Mar 28, 2019

lionelvillard commented Mar 28, 2019

dubee left a comment •

edited

Loading

dubee commented Mar 28, 2019

aslom Mar 28, 2019 •

edited

Loading

n3wscott Apr 10, 2019

aslom Mar 28, 2019

aslom commented Mar 28, 2019

aslom commented Mar 28, 2019

bbrowning commented Mar 28, 2019

aslom commented Apr 3, 2019

dubee left a comment

knative-prow-robot commented Apr 4, 2019

matzew commented Apr 9, 2019

aslom commented Apr 9, 2019

matzew commented Apr 9, 2019

matzew commented Apr 9, 2019

matzew commented Apr 9, 2019

aslom commented Apr 9, 2019

aslom commented Apr 9, 2019

aslom commented Apr 9, 2019

matzew commented Apr 10, 2019

matzew commented Apr 10, 2019

n3wscott commented Apr 10, 2019

knative-prow-robot commented Apr 10, 2019

Kafka source image policy #303

Kafka source image policy #303

Conversation

aslom commented Mar 27, 2019

aslom commented Mar 27, 2019

knative-prow-robot commented Mar 27, 2019

aslom commented Mar 27, 2019

bbrowning commented Mar 27, 2019 • edited Loading

bbrowning commented Mar 27, 2019

aslom commented Mar 27, 2019

bbrowning commented Mar 27, 2019

aslom commented Mar 27, 2019

matzew commented Mar 27, 2019 via email

aslom commented Mar 28, 2019

lionelvillard commented Mar 28, 2019

dubee left a comment • edited Loading

Choose a reason for hiding this comment

dubee commented Mar 28, 2019

aslom Mar 28, 2019 • edited Loading

Choose a reason for hiding this comment

n3wscott Apr 10, 2019

Choose a reason for hiding this comment

aslom Mar 28, 2019

Choose a reason for hiding this comment

aslom commented Mar 28, 2019

aslom commented Mar 28, 2019

bbrowning commented Mar 28, 2019

aslom commented Apr 3, 2019

dubee left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Apr 4, 2019

matzew commented Apr 9, 2019

aslom commented Apr 9, 2019

matzew commented Apr 9, 2019

matzew commented Apr 9, 2019

matzew commented Apr 9, 2019

aslom commented Apr 9, 2019

aslom commented Apr 9, 2019

aslom commented Apr 9, 2019

matzew commented Apr 10, 2019

matzew commented Apr 10, 2019

n3wscott commented Apr 10, 2019

knative-prow-robot commented Apr 10, 2019

bbrowning commented Mar 27, 2019 •

edited

Loading

dubee left a comment •

edited

Loading

aslom Mar 28, 2019 •

edited

Loading