Skip to content

upgrade: making go routine resilient to conflicts (PROJQUAY-2395)#496

Merged
ricardomaraschini merged 1 commit intoquay:masterfrom
ricardomaraschini:version-update-fix
Aug 17, 2021
Merged

upgrade: making go routine resilient to conflicts (PROJQUAY-2395)#496
ricardomaraschini merged 1 commit intoquay:masterfrom
ricardomaraschini:version-update-fix

Conversation

@ricardomaraschini
Copy link
Contributor

The go routine reponsible for updating the current version in status
can't fail otherwise we ended up not fully deploying Quay.

This commit makes it more resilient to transient failures. This go
routine must go away, this is a temporary fix.

The go routine reponsible for updating the current version in status
can't fail otherwise we ended up not fully deploying Quay.

This commit makes it more resilient to transient failures. This go
routine must go away, this is a temporary fix.
log.Error(err, "could not update QuayRegistry status with current version")

return true, err
return false, nil
Copy link
Member

Choose a reason for hiding this comment

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

Why is the error not returned anymore, and how will the controller handle that change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to make the go routine resilient to errors by simply logging them instead of returning them. If we return an error the go routine will stop trying and this is what we are trying to avoid. As a side note, this go routine must go away because it is wrong.

log.Error(err, "could not update QuayRegistry spec to complete upgrade")

return true, err
return false, nil
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer.


updatedQuay, _ := v1.EnsureRegistryEndpoint(quayContext, updatedQuay, userProvidedConfig)
var freshQuay v1.QuayRegistry
if err := r.Client.Get(ctx, req.NamespacedName, &freshQuay); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning for this extra request first, as opposed to using updatedQuay directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the version we have in memory is not the current version (the version in the etcd) we will receive a Conflict error. If we don't fetch a fresh version, once we receive a Conflict back, we will always see Conflicts.

Copy link
Member

@kleesc kleesc left a comment

Choose a reason for hiding this comment

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

LGTM

@ricardomaraschini ricardomaraschini merged commit 9a556c1 into quay:master Aug 17, 2021
harishsurf added a commit to harishsurf/quay-operator that referenced this pull request Aug 17, 2021
backport-fix: (PROJQUAY-2395) (quay#496)

Signed-off-by: harishsurf <hgovinda@redhat.com>
harishsurf added a commit that referenced this pull request Aug 17, 2021
backport-fix: (PROJQUAY-2395) (#496)

Signed-off-by: harishsurf <hgovinda@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants