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

upgrade: cleanup CreateWithRetry usage #1145

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions pkg/cluster/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,11 +206,34 @@ func CreateWithRetry(ctx context.Context, cli client.Client, obj client.Object,
timeout := time.Duration(timeoutMin) * time.Minute

return wait.PollUntilContextTimeout(ctx, interval, timeout, true, func(ctx context.Context) (bool, error) {
err := cli.Create(ctx, obj)
if err != nil {
fmt.Printf("Error creating object: %v. Retrying...\n", err)
return false, err
// Create can return:
// If webhook enabled:
// - no error (err == nil)
// - 500 InternalError likely if webhook is not available (yet)
// - 403 Forbidden if webhook blocks creation (check of existence)
// - some problem (real error)
// else, if webhook disabled:
// - no error (err == nil)
// - 409 AlreadyExists if object exists
// - some problem (real error)
errCreate := cli.Create(ctx, obj)
if errCreate == nil {
return true, nil
}
return true, nil

// check existence, success case for the function, covers 409 and 403 (or newly created)
errGet := cli.Get(ctx, client.ObjectKeyFromObject(obj), obj)
if errGet == nil {
return true, nil
}

// retry if 500, assume webhook is not available
if k8serr.IsInternalError(errCreate) {
fmt.Printf("Error creating object: %v. Retrying...\n", errCreate)
return false, nil
}

// some other error
return false, errCreate
})
}
14 changes: 2 additions & 12 deletions pkg/upgrade/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"errors"
"fmt"
"reflect"
"time"

"github.com/hashicorp/go-multierror"
operatorv1 "github.com/openshift/api/operator/v1"
Expand Down Expand Up @@ -104,17 +103,9 @@ func CreateDefaultDSC(ctx context.Context, cli client.Client) error {
},
}
err := cluster.CreateWithRetry(ctx, cli, releaseDataScienceCluster, 1) // 1 min timeout
switch {
case err == nil:
fmt.Printf("created DataScienceCluster resource\n")
case k8serr.IsAlreadyExists(err):
// Do not update the DSC if it already exists
fmt.Printf("DataScienceCluster resource already exists. It will not be updated with default DSC.\n")
return nil
default:
if err != nil {
return fmt.Errorf("failed to create DataScienceCluster custom resource: %w", err)
}

Copy link
Member

Choose a reason for hiding this comment

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

on line 122: If there exists an instance already, it patches the DSCISpec with default values

can we first clarify what we want: if exist then patch back to default or leave it as-is.
from code (even before this PR) looks like it is not gonna patch to default, because if one DSCI CR exist, then dont do anything regardless what is the name or if the spec is different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great eye! I missed the comment. In the original patch 1ccbe05 ("Unset Tech Preview components by default (#708)") it patched with the default values. But then f756e40 ("Update incubation with downstream changes (#783)") changes it with the message I borrowed without changing the comment.

(Sorry, but due to squashing it's not obvious where the change came from, I could dig it to a4788f3 ("fix(mm-monitoring): revert the code logic but set to disable as delete (#153)") but it squashed again and has only line Retain existing DSCI values with no description or commit reference around.)

Copy link
Member

Choose a reason for hiding this comment

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

interesting. i will need to have a follow-up on this. probably talk with @VaishnaviHire when she is back.

return nil
}

Expand Down Expand Up @@ -167,9 +158,8 @@ func CreateDefaultDSCI(ctx context.Context, cli client.Client, _ cluster.Platfor
return nil
case len(instances.Items) == 0:
fmt.Println("create default DSCI CR.")
time.Sleep(10 * time.Second) // put 10 seconds sleep for webhook to fully functional before request first creation
err := cluster.CreateWithRetry(ctx, cli, defaultDsci, 1) // 1 min timeout
if err != nil && !k8serr.IsAlreadyExists(err) {
if err != nil {
return err
}
}
Expand Down
Loading