Skip to content

Commit

Permalink
UPSTREAM: <carry>: skip posting failures to aggregated APIs to avoid …
Browse files Browse the repository at this point in the history
…getting false positives until the server becomes ready

the availability checks depend on fully initialized SDN
OpenShift carries a few reachability checks that affect /readyz protocol
we skip posting failures to avoid getting false positives until the server becomes ready

UPSTREAM: <carry>: skip posting failures to aggregated APIs to avoid getting false positives until the server becomes ready

marks availability of the server before checking the aggregate APIs
as it can change as we are running the checks.
in that case, skip posting failures to avoid false positives.

note on the next rebase please squash with the previous commit

openshift-rebase(v1.24):source=30214940c21

UPSTREAM: <carry>: expose HasBeenReady lifecycle signal

openshift-rebase(v1.24):source=110aaa7c54c

openshift-rebase(v1.24):source=110aaa7c54c

openshift-rebase(v1.24):source=110aaa7c54c
  • Loading branch information
p0lyn0mial authored and soltysh committed Sep 20, 2022
1 parent bf2b5fa commit 8558e88
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 2 deletions.
5 changes: 5 additions & 0 deletions staging/src/k8s.io/apiserver/pkg/server/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,11 @@ func (c *Config) DrainedNotify() <-chan struct{} {
return c.lifecycleSignals.InFlightRequestsDrained.Signaled()
}

// HasBeenReadySignal exposes a server's lifecycle signal which is signaled when the readyz endpoint succeeds for the first time.
func (c *Config) HasBeenReadySignal() <-chan struct{} {
return c.lifecycleSignals.HasBeenReady.Signaled()
}

// Complete fills in any fields not set that are required to have valid data and can be derived
// from other fields. If you're going to `ApplyOptions`, do that first. It's mutating the receiver.
func (c *Config) Complete(informers informers.SharedInformerFactory) CompletedConfig {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ func (c completedConfig) NewWithDelegate(delegationTarget genericapiserver.Deleg
(func() ([]byte, []byte))(s.proxyCurrentCertKeyContent),
s.serviceResolver,
c.GenericConfig.EgressSelector,
c.GenericConfig.HasBeenReadySignal(),
)
if err != nil {
return nil, err
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ type AvailableConditionController struct {

// metrics registered into legacy registry
metrics *availabilityMetrics

// hasBeenReady is signaled when the readyz endpoint succeeds for the first time.
hasBeenReady <-chan struct{}
}

type tlsTransportCache struct {
Expand Down Expand Up @@ -152,6 +155,7 @@ func NewAvailableConditionController(
proxyCurrentCertKeyContent certKeyFunc,
serviceResolver ServiceResolver,
egressSelector *egressselector.EgressSelector,
hasBeenReady <-chan struct{},
) (*AvailableConditionController, error) {
c := &AvailableConditionController{
apiServiceClient: apiServiceClient,
Expand All @@ -171,6 +175,7 @@ func NewAvailableConditionController(
proxyCurrentCertKeyContent: proxyCurrentCertKeyContent,
tlsCache: &tlsTransportCache{transports: make(map[tlsCacheKey]http.RoundTripper)},
metrics: newAvailabilityMetrics(),
hasBeenReady: hasBeenReady,
}

if egressSelector != nil {
Expand Down Expand Up @@ -233,6 +238,18 @@ func (c *AvailableConditionController) sync(key string) error {
return err
}

// the availability checks depend on fully initialized SDN
// OpenShift carries a few reachability checks that affect /readyz protocol
// record availability of the server so that we can
// skip posting failures to avoid getting false positives until the server becomes ready
hasBeenReady := false
select {
case <-c.hasBeenReady:
hasBeenReady = true
default:
// continue, we will skip posting only potential failures
}

// if a particular transport was specified, use that otherwise build one
// construct an http client that will ignore TLS verification (if someone owns the network and messes with your status
// that's not so bad) and sets a very short timeout. This is a best effort GET that provides no additional information
Expand Down Expand Up @@ -427,6 +444,11 @@ func (c *AvailableConditionController) sync(key string) error {
}

if lastError != nil {
if !hasBeenReady {
// returning an error will requeue the item in an exponential fashion
return fmt.Errorf("the server hasn't been ready yet, skipping updating availability of the aggreaged API until the server becomes ready to avoid false positives, lastError = %v", lastError)
}

availableCondition.Status = apiregistrationv1.ConditionFalse
availableCondition.Reason = "FailedDiscoveryCheck"
availableCondition.Message = lastError.Error()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ func setupAPIServices(apiServices []*apiregistration.APIService) (*AvailableCond
for _, o := range apiServices {
apiServiceIndexer.Add(o)
}
alwaysReadyChan := make(chan struct{})
close(alwaysReadyChan)

c := AvailableConditionController{
apiServiceClient: fakeClient.ApiregistrationV1(),
Expand All @@ -134,8 +136,9 @@ func setupAPIServices(apiServices []*apiregistration.APIService) (*AvailableCond
// the maximum disruption time to a minimum, but it does prevent hot loops.
workqueue.NewItemExponentialFailureRateLimiter(5*time.Millisecond, 30*time.Second),
"AvailableConditionController"),
tlsCache: &tlsTransportCache{transports: make(map[tlsCacheKey]http.RoundTripper)},
metrics: newAvailabilityMetrics(),
tlsCache: &tlsTransportCache{transports: make(map[tlsCacheKey]http.RoundTripper)},
metrics: newAvailabilityMetrics(),
hasBeenReady: alwaysReadyChan,
}
for _, svc := range apiServices {
c.addAPIService(svc)
Expand Down Expand Up @@ -400,6 +403,8 @@ func TestSync(t *testing.T) {
w.WriteHeader(http.StatusForbidden)
}))
defer testServer.Close()
alwaysReadyChan := make(chan struct{})
close(alwaysReadyChan)

c := AvailableConditionController{
apiServiceClient: fakeClient.ApiregistrationV1(),
Expand All @@ -410,6 +415,7 @@ func TestSync(t *testing.T) {
proxyCurrentCertKeyContent: func() ([]byte, []byte) { return emptyCert(), emptyCert() },
tlsCache: &tlsTransportCache{transports: make(map[tlsCacheKey]http.RoundTripper)},
metrics: newAvailabilityMetrics(),
hasBeenReady: alwaysReadyChan,
}
c.sync(tc.apiServiceName)

Expand Down

0 comments on commit 8558e88

Please sign in to comment.