Skip to content
This repository has been archived by the owner on Jun 4, 2021. It is now read-only.

Implemented CronJob source #135

Merged
merged 6 commits into from
Dec 5, 2018
Merged

Implemented CronJob source #135

merged 6 commits into from
Dec 5, 2018

Conversation

whynowy
Copy link
Contributor

@whynowy whynowy commented Dec 1, 2018

Used Deployment as receive adapter instead of CronJob because of
the batched job side car issue.

kubernetes/kubernetes#25908

If this issue is resolved in the future, we can switch to use CronJob.

Proposed Changes

  • Provide Cron based event source support without external dependencies.

@knative-prow-robot knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 1, 2018
@whynowy whynowy changed the title Implemented CronJob resource Implemented CronJob source Dec 3, 2018
Harwayne
Harwayne reviewed Dec 3, 2018
View reviewed changes
pkg/adapter/cronjobevents/adapter.go Outdated Show resolved Hide resolved
pkg/adapter/cronjobevents/adapter.go Show resolved Hide resolved
pkg/adapter/cronjobevents/adapter.go Outdated Show resolved Hide resolved
pkg/controller/cronjobsource/reconcile.go Outdated Show resolved Hide resolved
pkg/controller/cronjobsource/reconcile.go Outdated Show resolved Hide resolved
pkg/controller/cronjobsource/reconcile.go Outdated Show resolved Hide resolved
pkg/controller/cronjobsource/reconcile.go Show resolved Hide resolved
pkg/controller/cronjobsource/reconcile.go Outdated Show resolved Hide resolved
pkg/controller/cronjobsource/reconcile.go Show resolved Hide resolved
pkg/controller/cronjobsource/reconcile.go Show resolved Hide resolved
@Harwayne
Copy link
Contributor

Harwayne commented Dec 3, 2018

/ok-to-test

@knative-prow-robot 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 Dec 3, 2018
Harwayne
Harwayne reviewed Dec 4, 2018
View reviewed changes
Copy link
Contributor

@Harwayne Harwayne left a comment

Choose a reason for hiding this comment

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

Looks good, just one last sticking point 😛

pkg/controller/cronjobsource/reconcile.go Show resolved Hide resolved
vaikas
vaikas reviewed Dec 4, 2018
View reviewed changes
Copy link
Contributor

@vaikas vaikas left a comment

Choose a reason for hiding this comment

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

Just curious why not utilize this so we don't need to have something running continuously.
https://kubernetes.io/docs/tasks/job/automated-tasks-with-cron-jobs/

cmd/cronjob_receive_adapter/main.go Outdated Show resolved Hide resolved
cmd/cronjob_receive_adapter/main.go Outdated Show resolved Hide resolved
pkg/controller/cronjobsource/provider.go Outdated Show resolved Hide resolved
pkg/adapter/cronjobevents/adapter.go Show resolved Hide resolved
config/default.yaml Show resolved Hide resolved
pkg/controller/cronjobsource/reconcile.go Show resolved Hide resolved
@whynowy
Copy link
Contributor Author

whynowy commented Dec 5, 2018 •
edited
Loading

@vaikas-google, The reason I didn't use k8s CronJob was put in the commit message, there's an issue with batched job if it has a sidecar, it can not run to completed status, see the issue below. And we can not disable istio-proxy sidecar otherwise it can not talk to the sink.

kubernetes/kubernetes#25908

Harwayne
Harwayne reviewed Dec 5, 2018
View reviewed changes
pkg/adapter/cronjobevents/adapter.go Outdated Show resolved Hide resolved
pkg/adapter/cronjobevents/adapter.go Outdated Show resolved Hide resolved
@Harwayne
Copy link
Contributor

Harwayne commented Dec 5, 2018

/lgtm

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

vaikas commented Dec 5, 2018

@whynowy
Oops, sorry I missed that completely, my bad :(

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2018
@whynowy
Copy link
Contributor Author

whynowy commented Dec 5, 2018

@whynowy
Oops, sorry I missed that completely, my bad :(

/lgtm
/approve

:) It looks like I need to rebase... doing it.

@whynowy
Implemented CronJob source
873417a 
Used Deployment as receive adapter instead of CronJob because of
the batched job side car issue.

kubernetes/kubernetes#25908

If this issue is resolved in the future, we can switch to use CronJob.
@whynowy
Fixup!
c3e000e
@whynowy
Use DeepDerivative to compare spec changes and validate schedule string
a7dc04a
@whynowy
No need to run goroutine for cron
b466eb6
@whynowy
Use CR unstructured client for cronjob source reconcile
4eed8cd
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2018
@whynowy
Copy link
Contributor Author

whynowy commented Dec 5, 2018 •
edited
Loading

@Harwayne @vaikas-google - rebased and updated the code to use unstructured client for dynamic objects

@knative-metrics-robot
Copy link

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
cmd/cronjob_receive_adapter/main.go Do not exist 0.0%
pkg/adapter/cronjobevents/adapter.go Do not exist 37.8%
pkg/apis/sources/v1alpha1/cron_job_types.go Do not exist 100.0%
pkg/controller/add_cronjobsources.go Do not exist 100.0%
pkg/controller/cronjobsource/provider.go Do not exist 0.0%
pkg/controller/cronjobsource/reconcile.go Do not exist 87.3%
pkg/controller/cronjobsource/resources/receive_adapter.go Do not exist 100.0%

@vaikas
Copy link
Contributor

vaikas commented Dec 5, 2018

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vaikas-google, whynowy

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:

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 6d895a6 into knative:master Dec 5, 2018
@whynowy whynowy deleted the cronjob-source1 branch December 5, 2018 22:40
@Harwayne Harwayne mentioned this pull request Feb 7, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Harwayne Harwayne Harwayne left review comments

@vaikas vaikas vaikas left review comments

@evankanderson evankanderson Awaiting requested review from evankanderson

@grantr grantr Awaiting requested review from grantr

Assignees

@Harwayne Harwayne

@vaikas vaikas

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@whynowy @Harwayne @vaikas @knative-metrics-robot @knative-prow-robot