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.

Sign up for GitHub

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

Jump to bottom

Check pod status before hook #5211

Merged
merged 1 commit into from
Sep 13, 2023
Merged

Check pod status before hook #5211

merged 1 commit into from
Sep 13, 2023

Conversation

cleverhu
Copy link
Contributor

@cleverhu cleverhu commented Aug 15, 2022 •
edited
Loading

Thank you for contributing to Velero!

Please add a summary of your change

Skip exec pod command when pod's phase is succeeded or failed.

Does your change fix a particular issue?

Fixes #5137

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@cleverhu cleverhu force-pushed the check-pod-status-before-hook branch from 1c07235 to b9c9f8d Compare August 15, 2022 19:51
@cleverhu
Copy link
Contributor Author

cleverhu commented Aug 15, 2022 •
edited
Loading

This is my first pr and I am following internal/hook/wait_exec_hook_handler.go to handle the pod phase. I don't know whether it's proper and whether it needs to consider the special situation of containers.

@reasonerjt reasonerjt requested a review from blackpiglet August 22, 2022 05:38
@blackpiglet
Copy link
Contributor

Please add a changlog according to this document: https://velero.io/docs/v1.5/code-standards/#adding-a-changelog

@cleverhu cleverhu changed the title Check pod status before hook [WIP]Check pod status before hook Aug 26, 2022
@cleverhu cleverhu force-pushed the check-pod-status-before-hook branch from b9c9f8d to b1b5a54 Compare August 26, 2022 05:38
@cleverhu cleverhu force-pushed the check-pod-status-before-hook branch 2 times, most recently from 7a0055e to c122415 Compare August 26, 2022 07:52
@cleverhu cleverhu changed the title [WIP]Check pod status before hook Check pod status before hook Aug 26, 2022
@cleverhu
Copy link
Contributor Author

@blackpiglet i have add the unit test for the case, please take a look.

@cleverhu cleverhu changed the title Check pod status before hook [WIP]Check pod status before hook Aug 26, 2022
@cleverhu cleverhu force-pushed the check-pod-status-before-hook branch from c122415 to 1dc0387 Compare August 26, 2022 08:32
@cleverhu cleverhu force-pushed the check-pod-status-before-hook branch from 1dc0387 to 3a43a20 Compare August 26, 2022 09:07
@cleverhu cleverhu changed the title [WIP]Check pod status before hook Check pod status before hook Aug 26, 2022
@cleverhu
Copy link
Contributor Author

@blackpiglet i have add the changlog and the unit test, do you have any spare time to review? Thank you!

@cleverhu cleverhu requested review from blackpiglet and removed request for qiuming-best and shubham-pampattiwar August 28, 2022 05:26
@blackpiglet
Copy link
Contributor

@cleverhu
It's not easy to do a UT for this case.
There are two ways that I can both accept:

  • Revert to inital state of this PR. No update of the UT cases.
  • Put the new logic into a function, and do UT like this case TestEnsureContainerExists.

@cleverhu cleverhu force-pushed the check-pod-status-before-hook branch from 3a43a20 to 041f61d Compare August 29, 2022 16:38
@cleverhu cleverhu force-pushed the check-pod-status-before-hook branch from 041f61d to ae3785c Compare August 29, 2022 16:39
@cleverhu
Copy link
Contributor Author

@reasonerjt PTAL

@stale
Copy link

stale bot commented Nov 1, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the staled label Nov 1, 2022
@haslersn haslersn mentioned this pull request Nov 7, 2022
Closed
@haslersn
Copy link

haslersn commented Nov 7, 2022

What's the status?

@stale stale bot removed the staled label Nov 7, 2022
@stale
Copy link

stale bot commented Jan 7, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the staled label Jan 7, 2023
@haslersn
Copy link

haslersn commented Jan 7, 2023

Not stale, because the issue still needs to be fixed.

@stale stale bot removed the staled label Jan 7, 2023
blackpiglet
blackpiglet reviewed Jan 8, 2023
View reviewed changes
pkg/podexec/pod_command_executor.go
@@ -123,6 +123,12 @@ func (e *defaultPodCommandExecutor) ExecutePodCommand(log logrus.FieldLogger, it
"hookTimeout": localHook.Timeout,
},
)

if pod.Status.Phase == corev1api.PodSucceeded || pod.Status.Phase == corev1api.PodFailed {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late response, but after going through the code again, I think we may need to change the condition check from current status into something like this:

if pod.Status.Phase !=  corev1api.PodRunning {
    ...
}

@blackpiglet
Copy link
Contributor

Furthermore, after this PR is put on hold for a while, this cannot be merged directly.
Could you help to merge main branch to align with the latest change?

@stale
Copy link

stale bot commented Mar 12, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the staled label Mar 12, 2023
@haslersn
Copy link

Not stale, because the issue still needs to be fixed.

@stale stale bot removed the staled label Mar 13, 2023
@stale
Copy link

stale bot commented May 18, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the staled label May 18, 2023
@haslersn
Copy link

haslersn commented Jun 6, 2023

Still not stale, because the issue still needs to be fixed.

@stale stale bot removed the staled label Jun 6, 2023
@haslersn
Copy link

@ywk253100 I think your review is missing here

@haslersn haslersn mentioned this pull request Sep 12, 2023
Open
3 tasks
@ywk253100 ywk253100 merged commit 9b1cffc into vmware-tanzu:main Sep 13, 2023
@haslersn
Copy link

@ywk253100 Did you take a look at #6811 which implements this slightly differently? In particular, it logs an error when trying to run a hook on a Failed Pod, so this doesn't go unnoticed. I think having a Failed Pod is typically something which the user didn't intend, so the fact that we cannot run the hook there is also something which the user didn't intend and should be treated as an error in the backup process.

haslersn added a commit to haslersn/velero that referenced this pull request Sep 13, 2023
@haslersn
Revert "check pod status before hook (vmware-tanzu#5211)"
1728828 
This reverts commit 9b1cffc.
haslersn
haslersn reviewed Sep 13, 2023
View reviewed changes
pkg/podexec/pod_command_executor.go
@@ -123,6 +123,12 @@ func (e *defaultPodCommandExecutor) ExecutePodCommand(log logrus.FieldLogger, it
"hookTimeout": localHook.Timeout,
},
)

if pod.Status.Phase == corev1api.PodSucceeded || pod.Status.Phase == corev1api.PodFailed {
hookLog.Infof("Pod entered phase %s before some post-backup exec hooks ran", pod.Status.Phase)
Copy link

Choose a reason for hiding this comment

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

This code path also applies to pre-hooks as far as I can tell.

pkg/podexec/pod_command_executor_test.go
@@ -262,6 +262,37 @@ func TestEnsureContainerExists(t *testing.T) {
assert.NoError(t, err)
}

func TestPodCompeted(t *testing.T) {
Copy link

Choose a reason for hiding this comment

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

Typo

kaovilai pushed a commit to kaovilai/velero that referenced this pull request Oct 2, 2023
@cleverhu @kaovilai
check pod status before hook (vmware-tanzu#5211)
58c1e96 
Signed-off-by: cleverhu <[email protected]>
Co-authored-by: cleverhu <[email protected]>
kaovilai pushed a commit to kaovilai/velero that referenced this pull request Oct 2, 2023
@cleverhu @kaovilai
check pod status before hook (vmware-tanzu#5211)
e9cc56a 
Signed-off-by: cleverhu <[email protected]>
Co-authored-by: cleverhu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blackpiglet blackpiglet blackpiglet approved these changes

@haslersn haslersn haslersn left review comments

@ywk253100 ywk253100 ywk253100 approved these changes

Assignees

@cleverhu cleverhu

Labels
has-changelog has-unit-tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Velero tries to run preHook and postHook on Completed Pods
4 participants
@cleverhu @blackpiglet @haslersn @ywk253100