Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Explicit Workload Deletion #63

Merged
merged 4 commits into from
Jul 27, 2023

Conversation

razo7
Copy link
Member

@razo7 razo7 commented Jul 5, 2023

Explicit/Manuel workload deletion.

Deleting the pods and their volume attachments in order to signal the scheduler that this node's workloads should be rescheduled on a different node.
Includes e2e and unit tests which adds volume attachments and pod to the unhealthy node in order to verify if they have deleted as expected.

ECOPROJECT-1473

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 5, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@razo7
Copy link
Member Author

razo7 commented Jul 5, 2023

/test 4.13-openshift-e2e

Copy link
Contributor

@clobrano clobrano left a comment

Choose a reason for hiding this comment

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

only some typos from my point of view
/lgtm
Giving others a chance to review as well, feel free to unhold
/hold

controllers/fenceagentsremediation_controller.go Outdated Show resolved Hide resolved
pkg/utils/resources.go Outdated Show resolved Hide resolved
controllers/fenceagentsremediation_controller.go Outdated Show resolved Hide resolved
pkg/utils/resources.go Outdated Show resolved Hide resolved
pkg/utils/resources.go Show resolved Hide resolved
pkg/utils/resources.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm label Jul 5, 2023
pkg/utils/resources.go Outdated Show resolved Hide resolved
@razo7 razo7 force-pushed the workload-deletion branch 2 times, most recently from 570e7ba to b1574e8 Compare July 9, 2023 15:12
Copy link
Member

@slintes slintes 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 now, what about tests? :)

@razo7 razo7 changed the title Add explicit workload deletion Add Explicit Workload Deletion Jul 24, 2023
@razo7
Copy link
Member Author

razo7 commented Jul 25, 2023

/test 4.13-openshift-e2e

@razo7 razo7 marked this pull request as ready for review July 26, 2023 14:15
@openshift-ci openshift-ci bot requested review from clobrano and slintes July 26, 2023 14:16
Copy link
Member

@slintes slintes left a comment

Choose a reason for hiding this comment

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

something went wrong while rebasing, at least one commit is missing!

pkg/utils/resources.go Outdated Show resolved Hide resolved
Copy link
Member

@slintes slintes left a comment

Choose a reason for hiding this comment

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

one nit, I leave it up to you if you want to address it

/hold

test/e2e/far_e2e_test.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the lgtm label Jul 27, 2023
controllers/fenceagentsremediation_controller_test.go Outdated Show resolved Hide resolved
controllers/fenceagentsremediation_controller_test.go Outdated Show resolved Hide resolved
controllers/fenceagentsremediation_controller_test.go Outdated Show resolved Hide resolved
controllers/fenceagentsremediation_controller_test.go Outdated Show resolved Hide resolved
test/e2e/far_e2e_test.go Outdated Show resolved Hide resolved
controllers/fenceagentsremediation_controller_test.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm label Jul 27, 2023
Explicit/manual workload deletion. Deleting the pods and their volume attachments in order to signal the scheduler that this node's workloads should be rescheduled on a different node
Update unit tests to create two VA and pod which will be deleted by FAR pod
Update e2e tests to create one VA and one pod which will be deleted by FAR pod
Copy link
Contributor

@clobrano clobrano left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 27, 2023
Function declaration, comments, it's vs its confusion and etc.
@clobrano
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 27, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clobrano, razo7, slintes

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 [clobrano,razo7,slintes]

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

@razo7
Copy link
Member Author

razo7 commented Jul 27, 2023

/unhold

@openshift-merge-robot openshift-merge-robot merged commit 45e1c1f into medik8s:main Jul 27, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants