-
Notifications
You must be signed in to change notification settings - Fork 18
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
Draft fix for the OADP 4289 - to be tested before proposed upstream #331
base: oadp-1.4
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
@sseago @shubham-pampattiwar @weshayutin if you could comment if it's proper fix. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mpryc The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@sseago I've revisited that block of code and reworked it to be much simpler without so many if-else statements. Also checking if the error is type of conflict is unnecessary as the function If you could comment on current implementation. |
3234e26
to
25a069f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated workflow is a lot clearer -- only one patch attempt each time the func is called. The only problems I see now are 1) handling of the obj we're adding managed fields for prior to generating patch, and 2) we shouldn't error our for NotFound errors, to replicate current behavior (where the NotFound logic was added a few releases ago to handle an edge case that a user ran into). Comments inline.
pkg/restore/restore.go
Outdated
} else { | ||
ctx.log.Infof("the managed fields for %s is patched", kube.NamespaceAndName(obj)) | ||
withoutManagedFields = createdObj.DeepCopy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
withManagedFields = createdObj
(I don't think we need deepcopy here, since this is the last use of createdObj)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The createdObj is used later, so I wonder if we need to do some assignment so createdObj becomes updated with managed fields in a situation we get one fresh from cluster ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. Actually, in that case, a simpler change. Ignore the suggestion to create withManagedFields
and instead, above we can set createdObj
to the result of resourceClient.Get
and omit the else clause completely. We'll do the deepCopy outside of the if block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed.
|
||
if err != nil { | ||
ctx.log.Errorf("error patching managed fields %s: %v", kube.NamespaceAndName(obj), err) | ||
errs.Add(namespace, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the log.Errorf
here, since we already logged the error inside the retry func. Also, we need to add the if block around errs.Add
and return
here, since we don't error out if it's a NotFound error:
if !apierrors.IsNotFound(err) {
errs.Add(namespace, err)
return warnings, errs, itemExists
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For that I think we need to also include:
if err != nil && !apierrors.IsNotFound(err) {
errs.Add(namespace, err)
return warnings, errs, itemExists
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now.
Signed-off-by: allenxu404 <[email protected]>
Signed-off-by: lou <[email protected]> Co-authored-by: lou <[email protected]>
Signed-off-by: Lyndon-Li <[email protected]>
1. Skip deleting the restore files from storage if the backup/BSL is not found 2. Allow deleting the restore files from storage even though the BSL is readonly Signed-off-by: Wenkai Yin(尹文开) <[email protected]>
Add PSA audit and warn labels. Signed-off-by: Xun Jiang <[email protected]>
Signed-off-by: Lyndon-Li <[email protected]>
Signed-off-by: allenxu404 <[email protected]>
Signed-off-by: Lyndon-Li <[email protected]>
Signed-off-by: Lyndon-Li <[email protected]>
Signed-off-by: Lyndon-Li <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
(cherry picked from commit 8e01d1b) Signed-off-by: David Zaninovic <[email protected]>
Replace the base image with paketobuildpacks image Fixes vmware-tanzu#6851 Signed-off-by: Wenkai Yin(尹文开) <[email protected]>
Signed-off-by: Lyndon-Li <[email protected]>
Signed-off-by: allenxu404 <[email protected]>
Signed-off-by: Lyndon-Li <[email protected]>
Use informer cache with dynamic client for Get calls on restore When enabled, also make the Get call before create. Add server and install parameter to allow disabling this feature, but enable by default Signed-off-by: Scott Seago <[email protected]>
When creating resources with generateName, apimachinery does not guarantee uniqueness when it appends the random suffix to the generateName stub, so if it fails with already exists error, we need to retry. Signed-off-by: Scott Seago <[email protected]>
Signed-off-by: Sebastian Glab <[email protected]>
Signed-off-by: allenxu404 <[email protected]>
(cherry picked from commit ccb545f) Update PR-BZ automation mapping (openshift#84) (cherry picked from commit aa2b019) Update PR-BZ automation (openshift#92) Co-authored-by: Rayford Johnson <[email protected]> (cherry picked from commit ecc563f) Add publish workflow (openshift#108) (cherry picked from commit f87b779)
Code-gen no longer required on verify due to vmware-tanzu#6039 Signed-off-by: Tiger Kaovilai <[email protected]> oadp-1.2: Update Makefile.prow to velero-restore-helper
…nshift#280) Signed-off-by: Scott Seago <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Mateus Oliveira <[email protected]>
…penshift#330) * oadp-1.4: OADP-3227: Mark InProgress backup/restore as failed upon requeuing (openshift#315) * Mark InProgress backup/restore as failed upon requeuing Signed-off-by: Tiger Kaovilai <[email protected]> remove uuid, return err to requeue instead of requeue: true Signed-off-by: Tiger Kaovilai <[email protected]> * cleanup to minimize diff from upstream Signed-off-by: Scott Seago <[email protected]> * error message update Signed-off-by: Scott Seago <[email protected]> * requeue on finalize status update. Unlike the InProgress transition, there's no need to fail here, since the Finalize steps can be repeated. * Only run patch once for all backup finalizer return scenarios Signed-off-by: Scott Seago <[email protected]> --------- Signed-off-by: Tiger Kaovilai <[email protected]> Signed-off-by: Scott Seago <[email protected]> Co-authored-by: Scott Seago <[email protected]> * oadp-1.4: OADP-3227: Reconcile To Fail: Add backup/restore trackers (openshift#324) * OADP-4265: Reconcile To Fail: Add backup/restore trackers Signed-off-by: Tiger Kaovilai <[email protected]> * Apply suggestions from code review: backupTracker * Address restoreTracker feedback Signed-off-by: Tiger Kaovilai <[email protected]> * s/delete from/add to/ in the comment * unit test fix Signed-off-by: Tiger Kaovilai <[email protected]> * backup_controller unit test Signed-off-by: Tiger Kaovilai <[email protected]> * restore_controller unit test Signed-off-by: Tiger Kaovilai <[email protected]> * `make update` Signed-off-by: Tiger Kaovilai <[email protected]> * mock patch to fail failure due to connection refused Signed-off-by: Tiger Kaovilai <[email protected]> --------- Signed-off-by: Tiger Kaovilai <[email protected]> * regenerate mocks Signed-off-by: Tiger Kaovilai <[email protected]> --------- Signed-off-by: Tiger Kaovilai <[email protected]> Signed-off-by: Scott Seago <[email protected]> Co-authored-by: Scott Seago <[email protected]>
ctx.log.Infof("the managed fields for %s is patched", kube.NamespaceAndName(obj)) | ||
withoutManagedFields = createdObj.DeepCopy() | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add withoutManagedFields = createdObj.DeepCopy()
here, right before calling setManagedFields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that logic is much cleaner. thanks.
* fix: ARM images Signed-off-by: Mateus Oliveira <[email protected]> * fixup! fix: ARM images Signed-off-by: Mateus Oliveira <[email protected]> --------- Signed-off-by: Mateus Oliveira <[email protected]> (cherry picked from commit 41ad312)
…openshift#334) Introduction of downstream only option to override Kopia default: - hashing algorithm - splitting algorithm - encryption algorithm With introduction of 3 environment variables it is possible to override Kopia algorithms used by Velero: KOPIA_HASHING_ALGORITHM KOPIA_SPLITTER_ALGORITHM KOPIA_ENCRYPTION_ALGORITHM If the env algorithms are not set or they are not within Kopia SupportedAlgorithms, the default algorithm will be used. This behavior is consistent with current behavior without this change. Signed-off-by: Michal Pryc <[email protected]>
Signed-off-by: Michal Pryc <[email protected]>
Signed-off-by: Michal Pryc <[email protected]>
Signed-off-by: Michal Pryc <[email protected]>
Additional changes to the logic for restoring managed fields. Signed-off-by: Michal Pryc <[email protected]>
25a069f
to
74a4f2f
Compare
@mpryc: The following test failed, say
Full PR test history. 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-sigs/prow repository. I understand the commands that are listed here. |
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-sigs/prow repository. |
Draft pull request
http://quay.io/migi/velero:fix_oadp_4289