OCPBUGS-29479: Add retries to async cache initialization #13610

TheRealJon · 2024-02-15T20:58:08Z

Auth initialization can fail if the API server is not ready. This is especially common during cluster installation.

Thanks to @deads2k for doing the sleuth work on this.

Auth initialization can fail if the API server not ready yet. This is especially common during cluster install.

openshift-ci · 2024-02-15T20:58:12Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2024-02-15T20:58:14Z

@TheRealJon: This pull request references Jira Issue OCPBUGS-29479, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.16.0) matches configured target version for branch (4.16.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @yapei

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Auth initialization can fail if the API server is not ready. This is especially common during cluster installation.

Thanks to @deads2k for doing the sleuth work on this.

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 openshift-eng/jira-lifecycle-plugin repository.

deads2k · 2024-02-15T23:40:23Z

pkg/auth/asynccache.go

@@ -10,6 +10,11 @@ import (
 	"k8s.io/klog"
 )

+const (
+	initializationRetries    = 10
+	initializationRetryDelay = 30 * time.Second


you wan to be under the 150 liveness probe delay, which I think means you want this delay to 11 seconds.

See openshift/console-operator#869 which changes the probes for the console container.

deads2k · 2024-02-15T23:41:52Z

pkg/auth/asynccache.go

+			return c, nil
+		}
+		klog.V(4).Infof("retrying async cache setup - attempt %v of %v", retries, initializationRetries)
+		time.Sleep(initializationRetryDelay)


Suggested change

time.Sleep(initializationRetryDelay)

select{

case <- ctx.Done():

return nil, ctx.Error()

case <-time.After(initializationRetryDelay):

}

perhaps this, so that cancellation is honored?

replace the logic with k8s.io/apimachinery/pkg/util/wait.UntilWithContext()

@deads2k @stlaz @jhadvig I've addressed this feedback.

stlaz · 2024-02-16T09:54:34Z

pkg/auth/asynccache.go

+			return c, nil
+		}
+		klog.V(4).Infof("retrying async cache setup - attempt %v of %v", retries, initializationRetries)
+		time.Sleep(initializationRetryDelay)


replace the logic with k8s.io/apimachinery/pkg/util/wait.UntilWithContext()

…op to retry auth async cache setup

stlaz · 2024-02-20T12:42:39Z

pkg/auth/asynccache.go

@@ -27,13 +31,21 @@ func NewAsyncCache[T any](ctx context.Context, reloadPeriod time.Duration, cachi
 		cachingFunc:  cachingFunc,
 	}

-	item, err := cachingFunc(ctx)
+	var err error
+	ctx, cancel := context.WithTimeout(ctx, 5*time.Minute)


I would expect something in terms of seconds, not minutes 👀
Remember that this blocks proper startup.

This is intentionally long in order to give the API server time to become available. This context gets canceled on the first successful cachingFunc call, so we only wait the full 5 minutes when there is something wrong.

stlaz · 2024-02-20T12:43:30Z

pkg/auth/asynccache.go

+		}
+		c.cachedItem = item
+		cancel()
+	}, initializationRetryDelay)


the delay const is used in place of a check interval here

pkg/auth/asynccache.go

TheRealJon · 2024-02-21T14:07:50Z

/label tide/merge-method-squash

TheRealJon · 2024-02-21T14:08:34Z

@jhadvig @stlaz I've addressed all feedback, PTAL, thanks.

TheRealJon · 2024-02-21T20:40:52Z

/retest

stlaz · 2024-02-22T13:12:21Z

pkg/auth/asynccache.go

@@ -10,6 +10,12 @@ import (
 	"k8s.io/klog"
 )

+const (
+	initializationRetryInterval = 30 * time.Second


What about 10s to match the historical behavior:

console/pkg/auth/auth.go

Line 182 in 057e732

backoff = time.Second * 10

stlaz · 2024-02-22T13:12:29Z

pkg/auth/asynccache.go

@@ -10,6 +10,12 @@ import (
 	"k8s.io/klog"
 )

+const (
+	initializationRetryInterval = 30 * time.Second
+	initializationTimeout       = 5 * time.Minute


@deads2k is this ok?

@stlaz @deads2k The startupProbe for console pod lasts 5m, which also matches the duration console would retry auth config historically (4.14 and before).

pkg/auth/asynccache.go

- Log error on each retry - Do not return error from PollUntilContextTimeout condition func - Reduce retry interval to 10 seconds to match historical behavior

TheRealJon · 2024-02-22T20:39:54Z

@jhadvig @stlaz I've updated according to the last comments. PTAL when you can.

TheRealJon · 2024-02-26T14:41:10Z

/retest

jhadvig

/lgtm
/approve

openshift-ci · 2024-02-27T18:36:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhadvig, TheRealJon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/auth/OWNERS~~ [TheRealJon,jhadvig]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jhadvig · 2024-02-28T13:09:05Z

/retest

openshift-ci-robot · 2024-02-28T20:37:21Z

/retest-required

Remaining retests: 0 against base HEAD 97b74c6 and 2 for PR HEAD 5f03015 in total

openshift-ci · 2024-02-28T22:12:37Z

@TheRealJon: all tests passed!

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/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot · 2024-02-28T22:17:06Z

@TheRealJon: Jira Issue OCPBUGS-29479: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-29479 has been moved to the MODIFIED state.

In response to this:

Auth initialization can fail if the API server is not ready. This is especially common during cluster installation.

Thanks to @deads2k for doing the sleuth work on this.

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-bot · 2024-02-29T00:38:59Z

[ART PR BUILD NOTIFIER]

This PR has been included in build openshift-enterprise-console-container-v4.16.0-202402282339.p0.g0a4e4b1.assembly.stream.el8 for distgit openshift-enterprise-console.
All builds following this will include this PR.

TheRealJon · 2024-03-04T16:45:17Z

/cherry-pick release-4.15

openshift-cherrypick-robot · 2024-03-04T16:46:22Z

@TheRealJon: new pull request created: #13645

In response to this:

/cherry-pick release-4.15

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/test-infra repository.

openshift-merge-robot · 2024-03-30T10:27:56Z

Fix included in accepted release 4.16.0-0.nightly-2024-03-30-033956

OCPBUGS-29479: Add retries to async cache initialization

975cee7

Auth initialization can fail if the API server not ready yet. This is especially common during cluster install.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 15, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 15, 2024

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Feb 15, 2024

openshift-ci bot requested a review from yapei February 15, 2024 20:58

openshift-ci bot added component/backend Related to backend approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 15, 2024

Don't log errors on async cache retries.

6e153d8

deads2k reviewed Feb 15, 2024

View reviewed changes

stlaz suggested changes Feb 16, 2024

View reviewed changes

TheRealJon mentioned this pull request Feb 16, 2024

OCPBUGS-29479: Add startupProbe to console container. openshift/console-operator#869

Merged

Use context.WithTimeout and wait.UntilWithContext instead of a for lo…

1a23fc3

…op to retry auth async cache setup

TheRealJon marked this pull request as ready for review February 19, 2024 18:00

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 19, 2024

openshift-ci bot requested review from florkbr and spadgett February 19, 2024 18:01

stlaz reviewed Feb 20, 2024

View reviewed changes

pkg/auth/asynccache.go Outdated Show resolved Hide resolved

Use wait.PollUntilContextTimeout for async cache initialization

9761d5f

openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 21, 2024

stlaz reviewed Feb 22, 2024

View reviewed changes

Address review comments

5f03015

- Log error on each retry - Do not return error from PollUntilContextTimeout condition func - Reduce retry interval to 10 seconds to match historical behavior

jhadvig approved these changes Feb 27, 2024

View reviewed changes

openshift-ci bot assigned jhadvig Feb 27, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2024

openshift-merge-bot bot merged commit 0a4e4b1 into openshift:master Feb 28, 2024
6 checks passed

TheRealJon deleted the OCPBUGS-29479 branch March 4, 2024 16:45

openshift-cherrypick-robot mentioned this pull request Mar 4, 2024

[release-4.15] OCPBUGS-30208: Add retries to async cache initialization #13645

Closed

openshift-ci-robot mentioned this pull request Mar 29, 2024

OCPBUGS-29479: fix asynccache improper initialization #13703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-29479: Add retries to async cache initialization #13610

OCPBUGS-29479: Add retries to async cache initialization #13610

TheRealJon commented Feb 15, 2024

openshift-ci bot commented Feb 15, 2024

openshift-ci-robot commented Feb 15, 2024

deads2k Feb 15, 2024

TheRealJon Feb 16, 2024

deads2k Feb 15, 2024

stlaz Feb 16, 2024

TheRealJon Feb 19, 2024

stlaz Feb 16, 2024

stlaz Feb 20, 2024

TheRealJon Feb 21, 2024

stlaz Feb 20, 2024

TheRealJon commented Feb 21, 2024

TheRealJon commented Feb 21, 2024

TheRealJon commented Feb 21, 2024

stlaz Feb 22, 2024

TheRealJon Feb 22, 2024

stlaz Feb 22, 2024

TheRealJon Feb 22, 2024

TheRealJon commented Feb 22, 2024

TheRealJon commented Feb 26, 2024

jhadvig left a comment

openshift-ci bot commented Feb 27, 2024

jhadvig commented Feb 28, 2024

openshift-ci-robot commented Feb 28, 2024

openshift-ci bot commented Feb 28, 2024

openshift-ci-robot commented Feb 28, 2024

openshift-bot commented Feb 29, 2024

TheRealJon commented Mar 4, 2024

openshift-cherrypick-robot commented Mar 4, 2024

openshift-merge-robot commented Mar 30, 2024

-		time.Sleep(initializationRetryDelay)
+		select{
+		case <- ctx.Done():
+			return nil, ctx.Error()
+		case <-time.After(initializationRetryDelay):
+		}

OCPBUGS-29479: Add retries to async cache initialization #13610

OCPBUGS-29479: Add retries to async cache initialization #13610

Conversation

TheRealJon commented Feb 15, 2024

openshift-ci bot commented Feb 15, 2024

openshift-ci-robot commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealJon commented Feb 21, 2024

TheRealJon commented Feb 21, 2024

TheRealJon commented Feb 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealJon commented Feb 22, 2024

TheRealJon commented Feb 26, 2024

jhadvig left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 27, 2024

jhadvig commented Feb 28, 2024

openshift-ci-robot commented Feb 28, 2024

openshift-ci bot commented Feb 28, 2024

openshift-ci-robot commented Feb 28, 2024

openshift-bot commented Feb 29, 2024

TheRealJon commented Mar 4, 2024

openshift-cherrypick-robot commented Mar 4, 2024

openshift-merge-robot commented Mar 30, 2024