Implement executionhook controller #4

ashish-amarnath · 2020-01-06T19:55:52Z

Implement executionhook controller

Signed-off-by: Ashish Amarnath ashisham@vmware.com

k8s-ci-robot · 2020-01-06T19:56:01Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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.

ashish-amarnath · 2020-01-06T19:57:02Z

/check cla

k8s-ci-robot · 2020-01-06T19:57:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ashish-amarnath
To complete the pull request process, please assign kow3ns
You can assign the PR to them by writing /assign @kow3ns in a comment when ready.

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

ashish-amarnath · 2020-01-09T21:53:42Z

/check cla

Signed-off-by: Adnan Abdulhussein <prydonius@gmail.com>

* QoL improvements in makefile Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

Signed-off-by: Ashish Amarnath <ashisham@vmware.com> untested change Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

kow3ns

Looks good overall. One comment on the API and the testing is a bit sparse, but its a great start!

kow3ns · 2020-01-16T23:11:44Z

api/v1alpha1/executionhook_types.go

+	// The last error encountered when executing the action. The hook controller might
+	// update this field each time it retries the execution.
+	// +optional
+	Error *HookError `json:"error,omitempty" protobuf:"bytes,5,opt,name=error"`


There can be only one error? That is not an unreasonable assumption. However, this pattern seems to be similar to the Conditions pattern implemented by most built in types. Is there a reason to go with a singular error over Status.Conditions?

The expectation is that the error represents the error condition encountered executing the hook action on a specific container in a pod. So there is a 1:1 mapping between where the hook action was executed (Pod,Container) and an error condition, if any. For that reason, using a singular error was thought of as a better fit. However, using Status.Conditions would be more appropriate if/ when we aggregate the errors into the status of the ExecutionHook object.

I think we were given recommendations not to use Conditions during the API review, although I couldn't remember the reason behind that. @thockin @jingxu97 do you want to comment on this?

kow3ns · 2020-01-16T23:13:41Z

api/v1alpha1/executionhook_types.go

+// More error types could be added, e.g., Forbidden, Unauthorized, AlreadyInProgress, etc.
+const (
+	// The execution hook times out
+	Timeout ErrorType = "Timeout"


Another reason to consider using the Conditions pattern. This proposal might encourage end users to build a state machine off the error conditions (which as you point out in the comment) are likely to be extended and change. Across the rest of the API we generally avoid this as we've learned that its brittle.

Thanks for the suggestion. I will revisit this as I iterate on the implementation. WDYT?

@kow3ns If Conditions pattern is used, end user can still add more condition types?

xing-yang · 2020-01-17T19:09:32Z

Hi @ashish-amarnath, should this PR be dependent on #5?

ashish-amarnath · 2020-01-17T19:11:05Z

@xing-yang PR #4 (this PR) is the super set of changes in PR #5.
PR #5 was created to breakup the API changes from the controller implementation and unit tests, as it was suggested during last weeks meeting.

I would prefer getting PR #4 (this PR) merged and closing PR #5

fejta-bot · 2020-04-16T22:59:22Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

xing-yang · 2020-04-17T01:12:50Z

/lifecycle frozen

k8s-triage-robot · 2021-08-17T21:21:11Z

The lifecycle/frozen label can not be applied to PRs.

This bot removes lifecycle/frozen from PRs because:

Commenting /lifecycle frozen on a PR has not worked since March 2021
PRs that remain open for >150 days are unlikely to be easily rebased

You can:

Rebase this PR and attempt to get it merged
Close this PR with /close

Please send feedback to sig-contributor-experience at kubernetes/community.

/remove-lifecycle frozen

k8s-triage-robot · 2021-11-15T22:19:17Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2021-12-15T23:13:30Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2022-01-14T23:24:00Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2022-01-14T23:24:14Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 6, 2020

k8s-ci-robot requested review from kow3ns and xing-yang January 6, 2020 19:56

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 6, 2020

ashish-amarnath mentioned this pull request Jan 6, 2020

Generate Kubebuilder project for ExecutionHook CRDs and controller #3

Closed

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 6, 2020

ashish-amarnath force-pushed the exec-hook-controller branch from af89594 to 3fc948e Compare January 7, 2020 00:10

ashish-amarnath force-pushed the exec-hook-controller branch 3 times, most recently from 32ec242 to 470307f Compare January 9, 2020 22:07

ashish-amarnath added 6 commits January 9, 2020 14:13

generate kubebuilder project

60c1dd2

Signed-off-by: Adnan Abdulhussein <prydonius@gmail.com>

Fixes to get executionhook API working

0e6ae7b

* QoL improvements in makefile Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

implement podSelection and add unit tests

b72a53a

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

implement hookaction api and update RBAC

307d1f1

Signed-off-by: Ashish Amarnath <ashisham@vmware.com> untested change Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

implement exechookaction runner

20a04ec

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

don't rerun hook-action on succeeded podContainers

f05b3ab

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

ashish-amarnath force-pushed the exec-hook-controller branch from 470307f to f05b3ab Compare January 9, 2020 22:13

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 9, 2020

ashish-amarnath force-pushed the exec-hook-controller branch from 5bae119 to afccff6 Compare January 11, 2020 08:18

add more unit tests

84f28da

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

ashish-amarnath force-pushed the exec-hook-controller branch from afccff6 to 84f28da Compare January 13, 2020 01:34

xing-yang mentioned this pull request Jan 15, 2020

ExecutionHook kubernetes/enhancements#962

Closed

kow3ns approved these changes Jan 16, 2020

View reviewed changes

ashish-amarnath changed the title ~~[WIP] Implement executionhook controller~~ mplement executionhook controller Jan 17, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 17, 2020

ashish-amarnath changed the title ~~mplement executionhook controller~~ Implement executionhook controller Jan 17, 2020

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 16, 2020

k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 17, 2020

k8s-ci-robot removed the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Aug 17, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 15, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 15, 2021

k8s-ci-robot closed this Jan 14, 2022

Implement executionhook controller #4

Implement executionhook controller #4

Uh oh!

Conversation

ashish-amarnath commented Jan 6, 2020

Uh oh!

k8s-ci-robot commented Jan 6, 2020

Uh oh!

ashish-amarnath commented Jan 6, 2020

Uh oh!

k8s-ci-robot commented Jan 6, 2020

Uh oh!

ashish-amarnath commented Jan 9, 2020

Uh oh!

kow3ns left a comment

Choose a reason for hiding this comment

Uh oh!

kow3ns Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

ashish-amarnath Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

xing-yang Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

kow3ns Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

ashish-amarnath Jan 17, 2020 • 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!

xing-yang Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

xing-yang commented Jan 17, 2020

Uh oh!

ashish-amarnath commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fejta-bot commented Apr 16, 2020

Uh oh!

xing-yang commented Apr 17, 2020

Uh oh!

k8s-triage-robot commented Aug 17, 2021

Uh oh!

k8s-triage-robot commented Nov 15, 2021

Uh oh!

k8s-triage-robot commented Dec 15, 2021

Uh oh!

k8s-triage-robot commented Jan 14, 2022

Uh oh!

k8s-ci-robot commented Jan 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ashish-amarnath Jan 17, 2020 •

edited

Loading

ashish-amarnath commented Jan 17, 2020 •

edited

Loading