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

fix: resource already exists #1785

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

imkira
Copy link

@imkira imkira commented May 22, 2024

What issue type does this pull request address? (keep at least one, remove the others)
/kind bugfix

What does this pull request do? Which issues does it resolve? (use resolves #<issue_number> if possible)

During syncing of virtual namespaces to physical, the default service account already exists, which leads to errors during syncing. In this fix we consume the error in case the physical resource already exists.

Please provide a short message that should be published in the vcluster release notes
Fixed an issue where vcluster would not sync due to already existing default service accounts

What else do we need to know?

This is part of a collaborative work at Woven by Toyota with my colleagues @ajithcnambiar and @tauxi

Copy link

netlify bot commented May 22, 2024

Deploy Preview for vcluster-docs canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 07b39e7
🔍 Latest deploy log https://app.netlify.com/sites/vcluster-docs/deploys/668bdee3c531fd00087b5159

During syncing of virtual namespaces to physical, the default service
account already exists, which leads to errors during syncing. In this
fix we consume the error in case the physical resource already exists.

Co-authored-by: Ajith Chandran <ajith.chandran@woven.toyota>
Co-authored-by: Wojtek Koronski <wojtek.koronski@woven.toyota>
@imkira imkira force-pushed the agora-fix-already-exists-service-account branch from e1d7d3d to 07b39e7 Compare July 8, 2024 12:43
@imkira
Copy link
Author

imkira commented Jul 9, 2024

@heiko-braun there's also this one which I rebased. PTAL cheers.

@@ -80,6 +80,10 @@ func (n *namespacedTranslator) SyncToHostCreate(ctx *context.SyncContext, vObj,
ctx.Log.Debugf("error syncing %s %s/%s to physical cluster: %v", n.name, vObj.GetNamespace(), vObj.GetName(), err)
return ctrl.Result{RequeueAfter: time.Second}, nil
}
if kerrors.IsAlreadyExists(err) {
ctx.Log.Debugf("ignoring syncing %s %s/%s to physical cluster as it already exists", n.name, vObj.GetNamespace(), vObj.GetName())
Copy link
Contributor

Choose a reason for hiding this comment

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

Beyond a need to update this PR again with the new generics code, I'll have to give this some thought. It has essentially moved to here but placing it in such a central place means that any resource trying to create on sync that already exists could take over the original object and start making changes to it. It might be better to let it fail here as its already doing, or put checks on specific resources like the service accounts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants