fix: stop discovery errors from bootstrap process #1465

drivebyer · 2023-02-24T07:17:53Z

In test env, we got error sometimes:

We should not affect by extension API server during bootstrap process. The ServerPreferredResources() first returned result still contain recource info although err is not nil, see:

for _, apiGroup := range apiGroups.Groups {
	for _, version := range apiGroup.Versions {
		groupVersion := schema.GroupVersion{Group: apiGroup.Name, Version: version.Version}
		wg.Add(1)
		go func() {
			defer wg.Done()
			defer utilruntime.HandleCrash()


			apiResourceList, err := d.ServerResourcesForGroupVersion(groupVersion.String())


			// lock to record results
			resultLock.Lock()
			defer resultLock.Unlock()


			if err != nil {
				// TODO: maybe restrict this to NotFound errors
				failedGroups[groupVersion] = err
			}
			if apiResourceList != nil {
				// even in case of error, some fallback might have been returned
				groupVersionResources[groupVersion] = apiResourceList
			}
		}()
	}
}

https://github.com/kubernetes/client-go/blob/ba2fdddad18cb9d1822c00ddd5ba8cb0a428ebe7/discovery/discovery_client.go#L512-L536

drivebyer · 2023-02-24T07:21:14Z

PTAL @dvaldivia @harshavardhana @vadmeste @allanrogerr

vadmeste · 2023-02-24T10:41:26Z

@drivebyer do you know when this happens exactly ? old k8s version, a specific k8s flavor

drivebyer · 2023-02-25T06:10:00Z

do you know when this happens exactly ?

hi @vadmeste ,My k8s version is:

[root@g-master1 ~]# kubectl get nodes
NAME        STATUS   ROLES           AGE   VERSION
g-master1   Ready    control-plane   44h   v1.24.7
g-master2   Ready    control-plane   44h   v1.24.7
g-master3   Ready    control-plane   44h   v1.24.7

I think it is not about to k8s version, but about the ServerPreferredResources() behivior in client-go. You can reproduce it by following step:

create api service without api server behind:

[root@mcamel-dev-master ~]# kubectl apply -f /tmp/test.yaml
apiservice.apiregistration.k8s.io/v1alpha1.example.api.com created
[root@mcamel-dev-master ~]# cat /tmp/test.yaml
apiVersion: apiregistration.k8s.io/v1
kind: APIService
metadata:
  name: v1alpha1.example.api.com
spec:
  group: example.api.com
  groupPriorityMinimum: 100
  versionPriority: 15
  service:
    name: example-service
    namespace: default
  version: v1alpha1
[root@mcamel-dev-master ~]# kubectl get apiservices.apiregistration.k8s.io | grep example
v1alpha1.example.api.com                             default/example-service   False (ServiceNotFound)   21s

restart minio operator:

[root@mcamel-dev-master ~]# kubectl -n mcamel-system delete po minio-operator-79cf7b8996-wgn9d
pod "minio-operator-79cf7b8996-wgn9d" deleted
[root@mcamel-dev-master ~]# kubectl -n mcamel-system delete po minio-operator-79cf7b8996-dp87s
pod "minio-operator-79cf7b8996-dp87s" deleted
[root@mcamel-dev-master ~]# kubectl -n mcamel-system get po | grep minio
minio-operator-79cf7b8996-lpfm8                             0/1     Error       2 (17s ago)     22s
minio-operator-79cf7b8996-v9s4v                             1/1     Running     0               13s

then you can see error message:

[root@mcamel-dev-master ~]# kubectl -n mcamel-system logs minio-operator-79cf7b8996-v9s4v
I0225 06:02:56.002944       1 main.go:70] Starting MinIO Operator
I0225 06:02:56.637687       1 main.go:169] caBundle on CRD updated
I0225 06:02:56.638616       1 main-controller.go:244] Setting up event handlers
I0225 06:02:56.638856       1 leaderelection.go:248] attempting to acquire leader lease mcamel-system/minio-operator-lock...
I0225 06:02:56.647804       1 main-controller.go:496] minio-operator-79cf7b8996-lpfm8: is the leader, removing any leader labels that I 'minio-operator-79cf7b8996-v9s4v' might have
[root@mcamel-dev-master ~]# kubectl -n mcamel-system logs minio-operator-79cf7b8996-lpfm8
I0225 06:03:05.763885       1 main.go:70] Starting MinIO Operator
I0225 06:03:06.412054       1 main.go:169] caBundle on CRD updated
I0225 06:03:06.413340       1 main-controller.go:244] Setting up event handlers
I0225 06:03:06.413608       1 leaderelection.go:248] attempting to acquire leader lease mcamel-system/minio-operator-lock...
I0225 06:03:06.428900       1 leaderelection.go:258] successfully acquired lease mcamel-system/minio-operator-lock
I0225 06:03:06.428985       1 main-controller.go:478] minio-operator-79cf7b8996-lpfm8: I am the leader, applying leader labels on myself
I0225 06:03:06.429174       1 main-controller.go:390] Waiting for API to start
I0225 06:03:06.429638       1 main-controller.go:382] Starting HTTP Upgrade Tenant Image server
panic: unable to retrieve the complete list of server APIs: example.api.com/v1alpha1: the server is currently unable to handle the request

goroutine 99 [running]:
github.com/minio/operator/pkg/controller/cluster/certificates.GetCertificatesAPIVersion.func1()
	github.com/minio/operator/pkg/controller/cluster/certificates/csr.go:100 +0x1e5
sync.(*Once).doSlow(0x0?, 0x0?)
	sync/once.go:68 +0xc2
sync.(*Once).Do(...)
	sync/once.go:59
github.com/minio/operator/pkg/controller/cluster/certificates.GetCertificatesAPIVersion({0x1e97480?, 0xc0007a2780?})
	github.com/minio/operator/pkg/controller/cluster/certificates/csr.go:89 +0x5e
github.com/minio/operator/pkg/controller/cluster.(*Controller).Start.func1.1()
	github.com/minio/operator/pkg/controller/cluster/main-controller.go:350 +0x65
created by github.com/minio/operator/pkg/controller/cluster.(*Controller).Start.func1
	github.com/minio/operator/pkg/controller/cluster/main-controller.go:348 +0x10c

harshavardhana · 2023-02-25T20:09:29Z

IMO we shouldn't panic, instead slowdown and retry here. This is a valid scenario, even if things are not running rightfully we must not panic here IMO.

We should keep the operator reconcile this situation and retry for ever.

drivebyer · 2023-02-28T09:09:09Z

We should keep the operator reconcile this situation and retry for ever.

If we retry here, the bootstrap process will hang unless the extension API server be healthy. IMO, the minio operator should not affected by the extension API server.

The apiservice v1.certificates.k8s.io is Local. So we only care about the k8s apiserver's health status.

[root@mcamel-dev-master ~]# kubectl get apiservices.apiregistration.k8s.io  | grep certificates
v1.certificates.k8s.io                               Local                    True        225d

dvaldivia · 2023-02-28T21:22:51Z

We need to slowdown our rate of requests to the k8s api as well

drivebyer · 2023-03-07T06:43:37Z

We need to slowdown our rate of requests to the k8s api as well

The current call trace is main() -> Controller.Start() -> GetCertificatesAPIVersion(). GetCertificatesAPIVersion() core logic is:

func GetCertificatesAPIVersion(clientSet kubernetes.Interface) CSRVersion {
	// we will calculate which CSR version to use only once to avoid having to discover the
	certificateVersionOnce.Do(func() {
                 ...           
		switch version {
                 ...
		default:
			apiVersions, err := clientSet.Discovery().ServerPreferredResources()
                         ...
			for _, api := range apiVersions {
				if api.GroupVersion == "certificates.k8s.io/v1beta1" {
					csrVersion = CSRV1Beta1 // THE MEMORY VAR for later use
					break
				}
			}
		}
	})
	return csrVersion
}

Call ServerPreferredResources() only once. IMO, there is no place to adjust rate.

drivebyer · 2023-03-07T06:49:05Z

There are related case

hi @harshavardhana , is there any suggestion about this pr

vadmeste

LGTM

drivebyer · 2023-06-05T02:23:17Z

hi @harshavardhana @dvaldivia @cniackz @pjuarezd @cesnietor is there any suggestion about this pr

drivebyer force-pushed the fix_discover_client_error branch from 9c4502d to 36d6f52 Compare February 25, 2023 08:14

drivebyer force-pushed the fix_discover_client_error branch from 36d6f52 to 8548982 Compare April 6, 2023 09:01

vadmeste approved these changes Apr 6, 2023

View reviewed changes

jiuker approved these changes Jun 27, 2023

View reviewed changes

jiuker requested review from harshavardhana and pjuarezd June 27, 2023 07:11

fix: stop discovery errors from bootstrap process

35cea2d

drivebyer force-pushed the fix_discover_client_error branch from 8548982 to 35cea2d Compare June 29, 2023 02:05

pjuarezd approved these changes Jun 29, 2023

View reviewed changes

pjuarezd merged commit e1f2a7a into minio:master Jun 29, 2023
23 of 24 checks passed

drivebyer deleted the fix_discover_client_error branch June 30, 2023 01:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: stop discovery errors from bootstrap process #1465

fix: stop discovery errors from bootstrap process #1465

drivebyer commented Feb 24, 2023 •

edited

drivebyer commented Feb 24, 2023

vadmeste commented Feb 24, 2023 •

edited

drivebyer commented Feb 25, 2023

harshavardhana commented Feb 25, 2023

drivebyer commented Feb 28, 2023 •

edited

dvaldivia commented Feb 28, 2023

drivebyer commented Mar 7, 2023

drivebyer commented Mar 7, 2023 •

edited

vadmeste left a comment

drivebyer commented Jun 5, 2023 •

edited

fix: stop discovery errors from bootstrap process #1465

fix: stop discovery errors from bootstrap process #1465

Conversation

drivebyer commented Feb 24, 2023 • edited

drivebyer commented Feb 24, 2023

vadmeste commented Feb 24, 2023 • edited

drivebyer commented Feb 25, 2023

harshavardhana commented Feb 25, 2023

drivebyer commented Feb 28, 2023 • edited

dvaldivia commented Feb 28, 2023

drivebyer commented Mar 7, 2023

drivebyer commented Mar 7, 2023 • edited

vadmeste left a comment

Choose a reason for hiding this comment

drivebyer commented Jun 5, 2023 • edited

drivebyer commented Feb 24, 2023 •

edited

vadmeste commented Feb 24, 2023 •

edited

drivebyer commented Feb 28, 2023 •

edited

drivebyer commented Mar 7, 2023 •

edited

drivebyer commented Jun 5, 2023 •

edited