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 null pointer check for vs #5439

Closed
wants to merge 1 commit into from

Conversation

cleverhu
Copy link
Contributor

@cleverhu cleverhu commented Oct 12, 2022

Thank you for contributing to Velero!

Please add a summary of your change

Add null pointer check for vs.

Does your change fix a particular issue?

Fixes #5438

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.

sseago
sseago previously approved these changes Oct 12, 2022
@draghuram
Copy link
Contributor

Change looks good but while you are modifying the function, the log line at https://github.com/vmware-tanzu/velero/blob/main/pkg/controller/backup_controller.go#L997 seems incorrect.

logger.Debugf("Deleting VolumeSnapshotContent %s", vsc.Name)

The following line actually deletes the volume snapshot, not volume snapshot content. Perhaps the message can be fixed in this PR?

@cleverhu
Copy link
Contributor Author

Change looks good but while you are modifying the function, the log line at https://github.com/vmware-tanzu/velero/blob/main/pkg/controller/backup_controller.go#L997 seems incorrect.

logger.Debugf("Deleting VolumeSnapshotContent %s", vsc.Name)

The following line actually deletes the volume snapshot, not volume snapshot content. Perhaps the message can be fixed in this PR?

Ok,I will update the code.

draghuram
draghuram previously approved these changes Oct 13, 2022
@cleverhu cleverhu requested review from sseago and removed request for ywk253100 October 14, 2022 09:44
sseago
sseago previously approved these changes Oct 14, 2022
@draghuram
Copy link
Contributor

@cleverhu Can you please resolve the conflicts?

Can the PR be merged once conflicts are resolved? We are seeing the same crash reported from our users as well (cloudcasa.io).

@cleverhu
Copy link
Contributor Author

@cleverhu Can you please resolve the conflicts?

Can the PR be merged once conflicts are resolved? We are seeing the same crash reported from our users as well (cloudcasa.io).

Ok,I will do it.Thanks

@blackpiglet
Copy link
Contributor

blackpiglet commented Oct 20, 2022

#5388
This PR already contains the fix.

@cleverhu
Copy link
Contributor Author

#5388 This PR already contains the fix.

It seems better, the pr fix a comment for it.

Signed-off-by: cleverhu <shouping.hu@daocloud.io>
sshende-catalogicsoftware added a commit to catalogicsoftware/velero that referenced this pull request Oct 20, 2022
@blackpiglet
Copy link
Contributor

Since #5388 is merged, close this one for now. Feel free to reopen to address things not covered.

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.

error Timed out awaiting reconciliation of volumesnapshot
4 participants