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

[YUNIKORN-2651]Update the unchecked error for make lint warnings #850

Closed
wants to merge 10 commits into from
Closed
2 changes: 2 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ linters-settings:
local-prefixes: github.com/apache/yunikorn
govet:
check-shadowing: true
goconst:
min-occurrences: 5
funlen:
lines: 120
statements: 80
Expand Down
9 changes: 7 additions & 2 deletions pkg/admission/conf/am_conf.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,13 @@
return acc
}

func (acc *AdmissionControllerConf) RegisterHandlers(configMaps informersv1.ConfigMapInformer) {
configMaps.Informer().AddEventHandler(&configMapUpdateHandler{conf: acc})
func (acc *AdmissionControllerConf) RegisterHandlers(configMaps informersv1.ConfigMapInformer) error {
_, err := configMaps.Informer().AddEventHandler(&configMapUpdateHandler{conf: acc})
chia7712 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return fmt.Errorf("failed to create register handlers: %w", err)

Check warning on line 123 in pkg/admission/conf/am_conf.go

View check run for this annotation

Codecov / codecov/patch

pkg/admission/conf/am_conf.go#L120-L123

Added lines #L120 - L123 were not covered by tests
}

return nil

Check warning on line 126 in pkg/admission/conf/am_conf.go

View check run for this annotation

Codecov / codecov/patch

pkg/admission/conf/am_conf.go#L126

Added line #L126 was not covered by tests
}

func (acc *AdmissionControllerConf) GetNamespace() string {
Expand Down
11 changes: 8 additions & 3 deletions pkg/admission/namespace_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
package admission

import (
"fmt"

v1 "k8s.io/api/core/v1"
informersv1 "k8s.io/client-go/informers/core/v1"
"k8s.io/client-go/tools/cache"
Expand Down Expand Up @@ -53,14 +55,17 @@
}

// NewNamespaceCache creates a new cache and registers the handler for the cache with the Informer.
func NewNamespaceCache(namespaces informersv1.NamespaceInformer) *NamespaceCache {
func NewNamespaceCache(namespaces informersv1.NamespaceInformer) (*NamespaceCache, error) {
nsc := &NamespaceCache{
nameSpaces: make(map[string]nsFlags),
}
if namespaces != nil {
namespaces.Informer().AddEventHandler(&namespaceUpdateHandler{cache: nsc})
_, err := namespaces.Informer().AddEventHandler(&namespaceUpdateHandler{cache: nsc})
Copy link
Member

Choose a reason for hiding this comment

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

ditto. I don't think admission controller should start with those failures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Good insight. Updated in the new commit.

if err != nil {
return nil, fmt.Errorf("failed to create namespace cache: %w", err)

Check warning on line 65 in pkg/admission/namespace_cache.go

View check run for this annotation

Codecov / codecov/patch

pkg/admission/namespace_cache.go#L65

Added line #L65 was not covered by tests
}
}
return nsc
return nsc, nil
}

// enableYuniKorn returns the value for the enableYuniKorn flag (tri-state UNSET, TRUE or FALSE) for the namespace.
Expand Down
6 changes: 4 additions & 2 deletions pkg/admission/namespace_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ import (
const testNS = "test-ns"

func TestFlags(t *testing.T) {
cache := NewNamespaceCache(nil)
cache, nsErr := NewNamespaceCache(nil)
assert.NilError(t, nsErr)
cache.nameSpaces["notset"] = nsFlags{
enableYuniKorn: UNSET,
generateAppID: UNSET,
Expand Down Expand Up @@ -69,7 +70,8 @@ func TestNamespaceHandlers(t *testing.T) {
kubeClient := client.NewKubeClientMock(false)

informers := NewInformers(kubeClient, "default")
cache := NewNamespaceCache(informers.Namespace)
cache, nsErr := NewNamespaceCache(informers.Namespace)
assert.NilError(t, nsErr)
informers.Start()
defer informers.Stop()

Expand Down
13 changes: 10 additions & 3 deletions pkg/admission/priority_class_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
package admission

import (
"fmt"

"go.uber.org/zap"
schedulingv1 "k8s.io/api/scheduling/v1"
informersv1 "k8s.io/client-go/informers/scheduling/v1"
"k8s.io/client-go/tools/cache"
Expand All @@ -36,14 +39,18 @@
}

// NewPriorityClassCache creates a new cache and registers the handler for the cache with the Informer.
func NewPriorityClassCache(priorityClasses informersv1.PriorityClassInformer) *PriorityClassCache {
func NewPriorityClassCache(priorityClasses informersv1.PriorityClassInformer) (*PriorityClassCache, error) {
pcc := &PriorityClassCache{
priorityClasses: make(map[string]bool),
}
if priorityClasses != nil {
priorityClasses.Informer().AddEventHandler(&priorityClassUpdateHandler{cache: pcc})
_, err := priorityClasses.Informer().AddEventHandler(&priorityClassUpdateHandler{cache: pcc})
if err != nil {
log.Log(log.AdmissionConf).Error("Error adding event handler", zap.Error(err))
Copy link
Member

Choose a reason for hiding this comment

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

We don't need it as caller has log, right?

Copy link
Contributor Author

@SophieTech88 SophieTech88 Jun 4, 2024

Choose a reason for hiding this comment

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

Good catch. Many Thanks. Update the code to delete the useless log.

return nil, fmt.Errorf("failed to create a new cache and register the handler: %w", err)

Check warning on line 50 in pkg/admission/priority_class_cache.go

View check run for this annotation

Codecov / codecov/patch

pkg/admission/priority_class_cache.go#L49-L50

Added lines #L49 - L50 were not covered by tests
}
}
return pcc
return pcc, nil
}

// isPreemptSelfAllowed returns the preemption value. Only returns false if configured.
Expand Down
6 changes: 4 additions & 2 deletions pkg/admission/priority_class_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ import (
const testPC = "test-pc"

func TestIsPreemptSelfAllowed(t *testing.T) {
cache := NewPriorityClassCache(nil)
cache, pcErr := NewPriorityClassCache(nil)
assert.NilError(t, pcErr)
cache.priorityClasses["yes"] = true
cache.priorityClasses["no"] = false

Expand All @@ -49,7 +50,8 @@ func TestPriorityClassHandlers(t *testing.T) {
kubeClient := client.NewKubeClientMock(false)

informers := NewInformers(kubeClient, "default")
cache := NewPriorityClassCache(informers.PriorityClass)
cache, pcErr := NewPriorityClassCache(informers.PriorityClass)
assert.NilError(t, pcErr)
informers.Start()
defer informers.Stop()

Expand Down
33 changes: 27 additions & 6 deletions pkg/cache/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,33 +111,50 @@
return ctx
}

func (ctx *Context) AddSchedulingEventHandlers() {
ctx.apiProvider.AddEventHandler(&client.ResourceEventHandlers{
func (ctx *Context) AddSchedulingEventHandlers() error {
err := ctx.apiProvider.AddEventHandler(&client.ResourceEventHandlers{
Type: client.ConfigMapInformerHandlers,
FilterFn: ctx.filterConfigMaps,
AddFn: ctx.addConfigMaps,
UpdateFn: ctx.updateConfigMaps,
DeleteFn: ctx.deleteConfigMaps,
})
ctx.apiProvider.AddEventHandler(&client.ResourceEventHandlers{
if err != nil {
return err

Check warning on line 123 in pkg/cache/context.go

View check run for this annotation

Codecov / codecov/patch

pkg/cache/context.go#L123

Added line #L123 was not covered by tests
}

err = ctx.apiProvider.AddEventHandler(&client.ResourceEventHandlers{
Type: client.PriorityClassInformerHandlers,
FilterFn: ctx.filterPriorityClasses,
AddFn: ctx.addPriorityClass,
UpdateFn: ctx.updatePriorityClass,
DeleteFn: ctx.deletePriorityClass,
})
ctx.apiProvider.AddEventHandler(&client.ResourceEventHandlers{
if err != nil {
return err

Check warning on line 134 in pkg/cache/context.go

View check run for this annotation

Codecov / codecov/patch

pkg/cache/context.go#L134

Added line #L134 was not covered by tests
}

err = ctx.apiProvider.AddEventHandler(&client.ResourceEventHandlers{
Type: client.NodeInformerHandlers,
AddFn: ctx.addNode,
UpdateFn: ctx.updateNode,
DeleteFn: ctx.deleteNode,
})
ctx.apiProvider.AddEventHandler(&client.ResourceEventHandlers{
if err != nil {
return err

Check warning on line 144 in pkg/cache/context.go

View check run for this annotation

Codecov / codecov/patch

pkg/cache/context.go#L144

Added line #L144 was not covered by tests
}

err = ctx.apiProvider.AddEventHandler(&client.ResourceEventHandlers{
Type: client.PodInformerHandlers,
AddFn: ctx.AddPod,
UpdateFn: ctx.UpdatePod,
DeleteFn: ctx.DeletePod,
})
if err != nil {
return err

Check warning on line 154 in pkg/cache/context.go

View check run for this annotation

Codecov / codecov/patch

pkg/cache/context.go#L154

Added line #L154 was not covered by tests
}

return nil
}

func (ctx *Context) IsPluginMode() bool {
Expand Down Expand Up @@ -1449,7 +1466,11 @@

// Step 5: Start scheduling event handlers. At this point, initialization is mostly complete, and any existing
// objects will show up as newly added objects. Since the add/update event handlers are idempotent, this is fine.
ctx.AddSchedulingEventHandlers()
err = ctx.AddSchedulingEventHandlers()
if err != nil {
log.Log(log.Admission).Error("failed to add scheduling event handlers", zap.Error(err))
return err

Check warning on line 1472 in pkg/cache/context.go

View check run for this annotation

Codecov / codecov/patch

pkg/cache/context.go#L1471-L1472

Added lines #L1471 - L1472 were not covered by tests
}

// Step 6: Finalize priority classes. Between the start of initialization and when the informer event handlers are
// registered, it is possible that a priority class object was deleted. Process them again and remove
Expand Down
32 changes: 21 additions & 11 deletions pkg/client/apifactory.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package client

import (
"fmt"
"time"

"go.uber.org/zap"
Expand Down Expand Up @@ -53,7 +54,7 @@

type APIProvider interface {
GetAPIs() *Clients
AddEventHandler(handlers *ResourceEventHandlers)
AddEventHandler(handlers *ResourceEventHandlers) error
Copy link
Contributor

Choose a reason for hiding this comment

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

We have an API change here. It's probably OK but this type has been there for a long time.
cc @wilfred-s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. When we change the AddEventHandler to return error, it affects the APIProvider interface, too. If there is any suggestion for this part, please let me know. Many thanks.

Start()
Stop()
WaitForSync()
Expand Down Expand Up @@ -143,7 +144,7 @@
return s.testMode
}

func (s *APIFactory) AddEventHandler(handlers *ResourceEventHandlers) {
func (s *APIFactory) AddEventHandler(handlers *ResourceEventHandlers) error {

Check warning on line 147 in pkg/client/apifactory.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory.go#L147

Added line #L147 was not covered by tests
s.lock.Lock()
defer s.lock.Unlock()
// register all handlers
Expand All @@ -166,34 +167,43 @@
}

log.Log(log.ShimClient).Info("registering event handler", zap.Stringer("type", handlers.Type))
s.addEventHandlers(handlers.Type, h, 0)
if err := s.addEventHandlers(handlers.Type, h, 0); err != nil {
return fmt.Errorf("failed to initialize event handlers: %w", err)

Check warning on line 171 in pkg/client/apifactory.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory.go#L170-L171

Added lines #L170 - L171 were not covered by tests
}
return nil

Check warning on line 173 in pkg/client/apifactory.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory.go#L173

Added line #L173 was not covered by tests
}

func (s *APIFactory) addEventHandlers(
handlerType Type, handler cache.ResourceEventHandler, resyncPeriod time.Duration) {
handlerType Type, handler cache.ResourceEventHandler, resyncPeriod time.Duration) error {
var err error

Check warning on line 178 in pkg/client/apifactory.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory.go#L177-L178

Added lines #L177 - L178 were not covered by tests
switch handlerType {
case PodInformerHandlers:
s.GetAPIs().PodInformer.Informer().
_, err = s.GetAPIs().PodInformer.Informer().

Check warning on line 181 in pkg/client/apifactory.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory.go#L181

Added line #L181 was not covered by tests
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
case NodeInformerHandlers:
s.GetAPIs().NodeInformer.Informer().
_, err = s.GetAPIs().NodeInformer.Informer().

Check warning on line 184 in pkg/client/apifactory.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory.go#L184

Added line #L184 was not covered by tests
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
case ConfigMapInformerHandlers:
s.GetAPIs().ConfigMapInformer.Informer().
_, err = s.GetAPIs().ConfigMapInformer.Informer().

Check warning on line 187 in pkg/client/apifactory.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory.go#L187

Added line #L187 was not covered by tests
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
case StorageInformerHandlers:
s.GetAPIs().StorageInformer.Informer().
_, err = s.GetAPIs().StorageInformer.Informer().

Check warning on line 190 in pkg/client/apifactory.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory.go#L190

Added line #L190 was not covered by tests
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
case PVInformerHandlers:
s.GetAPIs().PVInformer.Informer().
_, err = s.GetAPIs().PVInformer.Informer().

Check warning on line 193 in pkg/client/apifactory.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory.go#L193

Added line #L193 was not covered by tests
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
case PVCInformerHandlers:
s.GetAPIs().PVCInformer.Informer().
_, err = s.GetAPIs().PVCInformer.Informer().

Check warning on line 196 in pkg/client/apifactory.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory.go#L196

Added line #L196 was not covered by tests
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
case PriorityClassInformerHandlers:
s.GetAPIs().PriorityClassInformer.Informer().
_, err = s.GetAPIs().PriorityClassInformer.Informer().

Check warning on line 199 in pkg/client/apifactory.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory.go#L199

Added line #L199 was not covered by tests
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
}

if 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.

If we fail to add handler, the initialization should be viewed as "failure"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Many thanks.

return fmt.Errorf("failed to add event handlers: %w", err)

Check warning on line 204 in pkg/client/apifactory.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory.go#L203-L204

Added lines #L203 - L204 were not covered by tests
}
return nil

Check warning on line 206 in pkg/client/apifactory.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory.go#L206

Added line #L206 was not covered by tests
}

func (s *APIFactory) WaitForSync() {
Expand Down
7 changes: 5 additions & 2 deletions pkg/client/apifactory_mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package client

import (
"fmt"
"time"

"go.uber.org/zap"
Expand Down Expand Up @@ -219,16 +220,18 @@
return true
}

func (m *MockedAPIProvider) AddEventHandler(handlers *ResourceEventHandlers) {
func (m *MockedAPIProvider) AddEventHandler(handlers *ResourceEventHandlers) error {

Check warning on line 223 in pkg/client/apifactory_mock.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory_mock.go#L223

Added line #L223 was not covered by tests
m.Lock()
defer m.Unlock()

if !m.running {
return
return fmt.Errorf("mocked API provider is not running")

Check warning on line 228 in pkg/client/apifactory_mock.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory_mock.go#L228

Added line #L228 was not covered by tests
}

m.eventHandler <- handlers
log.Log(log.Test).Info("registering event handler", zap.Stringer("type", handlers.Type))

return nil

Check warning on line 234 in pkg/client/apifactory_mock.go

View check run for this annotation

Codecov / codecov/patch

pkg/client/apifactory_mock.go#L234

Added line #L234 was not covered by tests
}

func (m *MockedAPIProvider) RunEventHandler() {
Expand Down
18 changes: 15 additions & 3 deletions pkg/cmd/admissioncontroller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,21 @@
kubeClient := client.NewKubeClient(amConf.GetKubeConfig())

informers := admission.NewInformers(kubeClient, amConf.GetNamespace())
amConf.RegisterHandlers(informers.ConfigMap)
pcCache := admission.NewPriorityClassCache(informers.PriorityClass)
nsCache := admission.NewNamespaceCache(informers.Namespace)

if hadlerErr := amConf.RegisterHandlers(informers.ConfigMap); hadlerErr != nil {
log.Log(log.Admission).Fatal("Failed to register handlers", zap.Error(hadlerErr))
return

Check warning on line 68 in pkg/cmd/admissioncontroller/main.go

View check run for this annotation

Codecov / codecov/patch

pkg/cmd/admissioncontroller/main.go#L66-L68

Added lines #L66 - L68 were not covered by tests
}
pcCache, pcErr := admission.NewPriorityClassCache(informers.PriorityClass)
if pcErr != nil {
log.Log(log.Admission).Fatal("Failed to create new priority class cache", zap.Error(pcErr))
return

Check warning on line 73 in pkg/cmd/admissioncontroller/main.go

View check run for this annotation

Codecov / codecov/patch

pkg/cmd/admissioncontroller/main.go#L70-L73

Added lines #L70 - L73 were not covered by tests
}
nsCache, nsErr := admission.NewNamespaceCache(informers.Namespace)
if nsErr != nil {
log.Log(log.Admission).Fatal("Failed to create namespace cache", zap.Error(nsErr))
return

Check warning on line 78 in pkg/cmd/admissioncontroller/main.go

View check run for this annotation

Codecov / codecov/patch

pkg/cmd/admissioncontroller/main.go#L75-L78

Added lines #L75 - L78 were not covered by tests
}
informers.Start()

wm, err := admission.NewWebhookManager(amConf)
Expand Down
4 changes: 0 additions & 4 deletions pkg/shim/scheduler_mock_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,6 @@ func (fc *MockScheduler) waitAndAssertApplicationState(t *testing.T, appID, expe
}
}

func (fc *MockScheduler) removeApplication(appId string) error {
return fc.context.RemoveApplication(appId)
}

func (fc *MockScheduler) waitAndAssertTaskState(t *testing.T, appID, taskID, expectedState string) {
app := fc.context.GetApplication(appID)
assert.Equal(t, app != nil, true)
Expand Down
8 changes: 7 additions & 1 deletion test/e2e/framework/helpers/k8s/k8s_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"github.com/onsi/ginkgo/v2"
"github.com/onsi/gomega"

"go.uber.org/zap"
appsv1 "k8s.io/api/apps/v1"
batchv1 "k8s.io/api/batch/v1"
v1 "k8s.io/api/core/v1"
Expand All @@ -58,6 +59,7 @@ import (
"github.com/apache/yunikorn-k8shim/pkg/common/constants"
"github.com/apache/yunikorn-k8shim/pkg/common/utils"
"github.com/apache/yunikorn-k8shim/pkg/locking"
"github.com/apache/yunikorn-k8shim/pkg/log"
"github.com/apache/yunikorn-k8shim/test/e2e/framework/configmanager"
"github.com/apache/yunikorn-k8shim/test/e2e/framework/helpers/common"
)
Expand Down Expand Up @@ -716,7 +718,11 @@ func (k *KubeCtl) StartConfigMapInformer(namespace string, stopChan <-chan struc
informerFactory := informers.NewSharedInformerFactoryWithOptions(k.clientSet, 0, informers.WithNamespace(namespace))
informerFactory.Start(stopChan)
configMapInformer := informerFactory.Core().V1().ConfigMaps()
configMapInformer.Informer().AddEventHandler(eventHandler)
_, err := configMapInformer.Informer().AddEventHandler(eventHandler)
if err != nil {
log.Log(log.AdmissionConf).Error("Error adding event handler", zap.Error(err))
return err
}
go configMapInformer.Informer().Run(stopChan)
if err := utils.WaitForCondition(func() bool {
return configMapInformer.Informer().HasSynced()
Expand Down