Knative support oneoffs for firing events

[liuchangyan]

Fixes #6302 Signed-off-by: Teresaliu [email protected]

Proposed Changes

• Extend the PingSourceSpec with a new variableDate and update corresponding field Date of PingSource API
• Field Date and Schedule are mutually exclusive; once the Date is specified, the event will be fired only once

Pre-review Checklist

• [ ] At least 80% unit test coverage
• [ ] E2E tests for any new behavior
• [ ] Docs PR for any user-facing impact
• [ ] Spec PR for any new API feature
• [ ] Conformance test for any change to the spec

Release Note

Docs

[codecov[bot]]

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (45e7a24) 80.44% compared to head (bcafd25) 80.51%. Report is 453 commits behind head on main.

Files	Patch %	Lines
pkg/adapter/mtping/runner.go	66.66%	7 Missing and 3 partials :warning:
pkg/apis/sources/v1/ping_validation.go	0.00%	5 Missing and 1 partial :warning:
pkg/apis/sources/v1/ping_lifecycle.go	0.00%	4 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6662      +/-   ##
==========================================
+ Coverage   80.44%   80.51%   +0.06%     
==========================================
  Files         236      236              
  Lines       12097    12186      +89     
==========================================
+ Hits         9732     9811      +79     
- Misses       1875     1885      +10     
  Partials      490      490              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[liuchangyan]

/assign @pierDipi @lionelvillard @matzew

[lionelvillard]

@liuchangyan thanks for doing this! Do you think expired one-off PingSource should be marked as completed or expired? Then when the adapter starts, it could filter out expired PingSources. WDYT?

[liuchangyan]

I think expired is better, because completed represents this one-off PingSource has fired succeed. And the function of filter out expired PingSource is not implemented yet in original code? I also need to add some new Status of PingSource, right?

[lionelvillard]

I think expired is better, because completed represents this one-off PingSource has fired succeed.

I agree, expired is better.

And the function of filter out expired PingSource is not implemented yet in original code?

Correct!

I also need to add some new Status of PingSource, right?

I think so.

[knative-prow[bot]]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: liuchangyan Once this PR has been reviewed and has the lgtm label, please ask for approval from lionelvillard by writing /assign @lionelvillard in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[liuchangyan]

@lionelvillard I'm sorry for this PR is pending so long time because of Spring Festival:). The field that one-off PingSource marked I changed to be UnExpired, it represents once this field is true, this source is ready to fire, and it need to be consistent with the other fields. WDYT?

[liuchangyan]

An upgrade is a normal operation, so at least we should be resilient enough to pass upgrades, so I think we should have a way of making the semantic be "at least one as long as the creation time of the pingsource is not earlier than the scheduled time"

@pierDipi You mean before MarkExpired, we need to compare the creation time of the pingsource and the scheduled time instead of time.now and the scheduled time, right? And in pingsource runner, if it is unexpired but the scheduled time has over time.now, we should execute it immediately at least once , have i understood correctly?

[pierDipi]

Yes, to clarify, what I would expect is that even when it is expired, it is always executed at least once and then once marked as completed/done we're sure we have executed it at least once. In addition, by validating datatime (on pingsource create actions only) is expired already or not we are sure that there won't be a pingsource created for a datetime in the past.

[liuchangyan]

In addition, by validating datatime (on pingsource create actions only) is expired already or not we are sure that there won't be a pingsource created for a datetime in the past.

Could we just use completed status, because we execute it anyway. And could we mark Pingsource Status in runner?

[pierDipi]

Sounds good to use completed status and have the runner communicate when it is completed

[knative-prow[bot]]

@liuchangyan: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
upgrade-tests_eventing_main	bcafd25fd3d8745e0c89ba2ae814377e8c3b5f02	link	true	/test upgrade-tests

Your PR dashboard.

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. I understand the commands that are listed here.

[liuchangyan]

Sounds good to use completed status and have the runner communicate when it is completed

@pierDipi I've added it, you could review if anytime you be free

[evankanderson]

Sounds good to use completed status and have the runner communicate when it is completed

I think we should call this Succeeded rather than Completed

https://github.com/knative/specs/blob/main/specs/common/error-signalling.md

1. Each resource MUST have a summary Ready condition (for ongoing systems) or Succeeded condition (for resources that run to completion) with severity="", which MUST use the "True", "False", and "Unknown" status values as follows:

   1. "False" MUST indicate a failure condition.
   2. "Unknown" SHOULD indicate that reconciliation is not yet complete and success or failure is not yet determined.
   3. "True" SHOULD indicate that the resource is fully reconciled and operating correctly.

"Unknown" and "True" are specified as SHOULD rather than MUST requirements because there might be errors which prevent functioning which cannot be determined by the API stack (e.g. DNS record configuration in certain environments). Implementations are expected to treat these as "MUST" for factors within the control of the implementation.

[evankanderson]

(Note that this means that a PingSource with a spec.date which is in the future SHOULD have a Succeeded value of Unknown, as we won't know until the time passes whether or not we were able to deliver the event...)

[liuchangyan]

@pierDipi Thanks for @evankanderson's review, and I think we need to be consistent with knative spec, Succeeded is better, and when we use cron PingSource, the status is set to be Unknown; The status True or Falsewill be used in one-off PingSource, WDYT?

[pierDipi]

Completed sounds good to me

[knative-prow-robot]

PR needs rebase.

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.

[github-actions[bot]]

This Pull Request is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen with /reopen. Mark as fresh by adding the comment /remove-lifecycle stale.

[github-actions[bot]]

This Pull Request is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen with /reopen. Mark as fresh by adding the comment /remove-lifecycle stale.