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

SophieTech88 · 2024-05-26T05:05:04Z

What is this PR for?

There are many different errors or warnings for make lint.

This PR tries to solve the unchecked error which relates to 5 files, goconst error and unused error in yunikorn-k8shim repo.

There are 11 errcheck looks like below:
pkg/admission/conf/am_conf.go:121:39: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandler is not checked (errcheck)
configMaps.Informer().AddEventHandler(&configMapUpdateHandler{conf: acc})
^
pkg/admission/namespace_cache.go:61:40: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandler is not checked (errcheck)
namespaces.Informer().AddEventHandler(&namespaceUpdateHandler{cache: nsc})
^
pkg/admission/priority_class_cache.go:44:45: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandler is not checked (errcheck)
priorityClasses.Informer().AddEventHandler(&priorityClassUpdateHandler{cache: pcc})
^
test/e2e/framework/helpers/k8s/k8s_utils.go:719:46: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandler is not checked (errcheck)
configMapInformer.Informer().AddEventHandler(eventHandler)
^
pkg/client/apifactory.go:177:35: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandlerWithResyncPeriod is not checked (errcheck)
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
^
pkg/client/apifactory.go:180:35: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandlerWithResyncPeriod is not checked (errcheck)
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
^
pkg/client/apifactory.go:183:35: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandlerWithResyncPeriod is not checked (errcheck)
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
^
pkg/client/apifactory.go:186:35: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandlerWithResyncPeriod is not checked (errcheck)
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
^
pkg/client/apifactory.go:189:35: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandlerWithResyncPeriod is not checked (errcheck)
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
^
pkg/client/apifactory.go:192:35: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandlerWithResyncPeriod is not checked (errcheck)
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
^
pkg/client/apifactory.go:195:35: Error return value of (k8s.io/client-go/tools/cache.SharedInformer).AddEventHandlerWithResyncPeriod is not checked (errcheck)
AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
^
There are 4 goconst errors as below:
pkg/common/si_helper_test.go:71:13: string pod-resource-test-00001 has 3 occurrences, make it a constant (goconst)
podName := "pod-resource-test-00001"
^
pkg/common/si_helper_test.go:72:15: string important has 3 occurrences, make it a constant (goconst)
namespace := "important"
^
pkg/cache/application_test.go:562:13: string task02 has 3 occurrences, make it a constant (goconst)
taskID2 := "task02"
^
pkg/shim/scheduler_test.go:42:16: string `
partitions:
- name: default
  queues:
  - name: root
    submitacl: "*"
    queues:
    - name: a
      resources:
      guaranteed:
      memory: 100000000
      vcore: 10
      max:
      memory: 150000000
      vcore: 20
      has 3 occurrences, make it a constant (goconst) configData :=
      ^
There is 1 unused error as below :
pkg/shim/scheduler_mock_test.go:165:26: func (*MockScheduler).removeApplication is unused (unused)
func (fc *MockScheduler) removeApplication(appId string) error {
^

Actions:

log out the errors for these 11 unchecked errors in 5 files.
When the error exists for RegisterHandler and NewNamespaceCache, this error should be viewed as fatal, pass the error message back to main(), and we should propagate it to stop k8shime. At same time, update the unit test for NewNamespaceCache func.
add min-occurrences = 5 in .golangci.yml file to increase the threshold
delete the function func (fc *MockScheduler) removeApplication(appId string) error{}

What type of PR is it?

Todos

- Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2651

How should this be tested?

It should pass all CI test.
make test passed locally.

Screenshots (if appropriate)

Questions:

- The licenses files need update.
- There is breaking changes for older versions.
- It needs documentation.

chia7712

@SophieTech88 thanks for this patch!

pkg/admission/conf/am_conf.go

chia7712 · 2024-05-26T19:06:49Z

pkg/admission/namespace_cache.go

@@ -58,7 +59,10 @@ func NewNamespaceCache(namespaces informersv1.NamespaceInformer) *NamespaceCache
 		nameSpaces: make(map[string]nsFlags),
 	}
 	if namespaces != nil {
-		namespaces.Informer().AddEventHandler(&namespaceUpdateHandler{cache: nsc})
+		_, err := namespaces.Informer().AddEventHandler(&namespaceUpdateHandler{cache: nsc})


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

Yeah. Good insight. Updated in the new commit.

… error exists.

chia7712 · 2024-05-27T16:06:38Z

pkg/admission/conf/am_conf.go

+	_, err := configMaps.Informer().AddEventHandler(&configMapUpdateHandler{conf: acc})
+	if err != nil {
+		log.Log(log.AdmissionConf).Fatal("Failed to create Register handler", zap.Error(err))
+		os.Exit(1)


Maybe we can return error instead of breaking program here, since the caller may want to do something cleanup before leaving.

Update to return error for NewNamespaceCache and RegisterHandler functions back to main(), and it will stop the process. Check the new commit.
Another question: what about the cleanup here?

Fatal() itself calls os.Exit(), so just a simple return does not change anything.

Another question: what about the cleanup here?

It seems to me propagating the error to callers allow them to do more actions. For example, log with more details or sent event or release resources. At any rate, exiting program in the middle of call chain is anti-pattern to me.

Good insight. Many thanks. Allow NewNamespaceCache, RegisterHandlers functions to pass error back, and deal with the error in main(). The code is updated.

…k to main()

chia7712 · 2024-05-29T22:03:23Z

pkg/client/apifactory.go

 			AddEventHandlerWithResyncPeriod(handler, resyncPeriod)
 	}
+
+	if err != nil {


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

Good catch. Many thanks.

pkg/cmd/admissioncontroller/main.go

pbacsko · 2024-05-30T08:09:46Z

pkg/client/apifactory.go

@@ -166,34 +167,42 @@ func (s *APIFactory) AddEventHandler(handlers *ResourceEventHandlers) {
 	}

 	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 {
+		log.Log(log.AdmissionConf).Fatal("Failed to initialize event handlers", zap.Error(err))


Do we want to exit here? Or delegate to the code in main.go()?

delegate to the code in main.go()?

+1

Good catch. Appreciated. Earlier I only return the error for addEventHandlers, but forgot to check AddEventHandler, when there is error returned, it should not exit in the middle.
I check the AddEventHandler, it is called by 3 functionss: RegisterHandlers, NewPriorityClassCache, NewNamespaceCache in main. When the AddEventHandler pass the error back, those 3 functions will return errors, too, and we can make the decision how to deal with the errors in main(). So it is great.

…s. Fix the typo in main().

pbacsko · 2024-05-31T06:45:55Z

pkg/client/apifactory.go

@@ -53,7 +54,7 @@ func (t Type) String() string {

 type APIProvider interface {
 	GetAPIs() *Clients
-	AddEventHandler(handlers *ResourceEventHandlers)
+	AddEventHandler(handlers *ResourceEventHandlers) error


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

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.

codecov · 2024-05-31T06:46:36Z

Codecov Report

Attention: Patch coverage is 20.31250% with 51 lines in your changes missing coverage. Please review.

Project coverage is 66.95%. Comparing base (5f80f49) to head (30f7429).
Report is 4 commits behind head on master.

❗ Current head 30f7429 differs from pull request most recent head e6b78bb

Please upload reports for the commit e6b78bb to get more accurate results.

Files	Patch %	Lines
pkg/client/apifactory.go	0.00%	16 Missing ⚠️
pkg/cache/context.go	38.88%	6 Missing and 5 partials ⚠️
pkg/cmd/admissioncontroller/main.go	0.00%	11 Missing ⚠️
pkg/admission/conf/am_conf.go	0.00%	5 Missing ⚠️
pkg/admission/priority_class_cache.go	50.00%	2 Missing and 1 partial ⚠️
pkg/client/apifactory_mock.go	0.00%	3 Missing ⚠️
pkg/admission/namespace_cache.go	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #850      +/-   ##
==========================================
- Coverage   67.33%   66.95%   -0.38%     
==========================================
  Files          70       70              
  Lines        7598     7635      +37     
==========================================
- Hits         5116     5112       -4     
- Misses       2271     2303      +32     
- Partials      211      220       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chia7712 · 2024-05-31T19:51:28Z

@SophieTech88 Could you please propagate the error by AddSchedulingEventHandlers?

yunikorn-k8shim/pkg/cache/context.go

Line 114 in 58adfe9

func (ctx *Context) AddSchedulingEventHandlers() {

By that change We can handle the error in the main function

SophieTech88 · 2024-06-01T03:47:37Z

@SophieTech88 Could you please propagate the error by AddSchedulingEventHandlers?

yunikorn-k8shim/pkg/cache/context.go

Line 114 in 58adfe9

func (ctx *Context) AddSchedulingEventHandlers() {

By that change We can handle the error in the main function

Many thanks for your suggestion. It is a good catch. The function AddSchedulingEventHandlers is also affected by AddEventHandler, so when AddEventHandler returns error, the function AddSchedulingEventHandlers should pass the error back, too.

But I see AddSchedulingEventHandlers is not called in main(). It works during the process for func InitializeState(). And when I trace back, the error propagate track looks like AddEventHandler() ->> func (ctx *Context) AddSchedulingEventHandlers() error {} ->> func (ctx *Context) InitializeState() error {} ->> func (ss *KubernetesShim) initSchedulerState() error {} ->> func (ss *KubernetesShim) Run() error{}

I just update the code to fix the return error in func AddSchedulingEventHandlers. Welcome to any suggestions.

chia7712 · 2024-06-01T19:29:39Z

pkg/admission/priority_class_cache.go

-		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))


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

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

chia7712

LGTM

[YUNIKORN-182]Update the unchecked error for make lint warnings

be9d1b3

chia7712 reviewed May 26, 2024

View reviewed changes

SophieTech88 added 3 commits May 26, 2024 21:24

Failed to RegisterHandler and failed to create namespace cache , when…

92c2099

… error exists.

add to increase the threshold

55a6080

remove unused function removeApplication

7e48f93

chia7712 reviewed May 27, 2024

View reviewed changes

Allow NewNamespaceCache, RegisterHandlers functions to pass error bac…

5c1b506

…k to main()

pbacsko assigned SophieTech88 May 28, 2024

chia7712 reviewed May 29, 2024

View reviewed changes

SophieTech88 changed the title ~~[YUNIKORN-182]Update the unchecked error for make lint warnings~~ [YUNIKORN-2851]Update the unchecked error for make lint warnings May 29, 2024

SophieTech88 changed the title ~~[YUNIKORN-2851]Update the unchecked error for make lint warnings~~ [YUNIKORN-2651]Update the unchecked error for make lint warnings May 29, 2024

SophieTech88 added 2 commits May 29, 2024 22:48

Update failure process for addEventHnadlers function

f7478de

Update failure process for NewPriorityClassCache function

df82ab6

pbacsko reviewed May 30, 2024

View reviewed changes

pkg/cmd/admissioncontroller/main.go Outdated Show resolved Hide resolved

pbacsko reviewed May 30, 2024

View reviewed changes

Add an error return for AddEventHandlers and AddEventHandler function…

0113fff

…s. Fix the typo in main().

pbacsko reviewed May 31, 2024

View reviewed changes

pbacsko requested a review from wilfred-s May 31, 2024 06:46

Update AddSchedulingEventHandlers to propagate the error back

30f7429

chia7712 reviewed Jun 1, 2024

View reviewed changes

delete the unusefule log for NewPriorityClassCache func

e6b78bb

chia7712 approved these changes Jun 4, 2024

View reviewed changes

chia7712 closed this in 3ce745e Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SophieTech88 commented May 26, 2024 •

edited

Loading

chia7712 left a comment

chia7712 May 26, 2024

SophieTech88 Jun 1, 2024

chia7712 May 27, 2024

SophieTech88 May 28, 2024

pbacsko May 28, 2024 •

edited

Loading

chia7712 May 28, 2024

SophieTech88 Jun 1, 2024

chia7712 May 29, 2024

SophieTech88 Jun 1, 2024

pbacsko May 30, 2024

chia7712 May 30, 2024

SophieTech88 Jun 1, 2024

pbacsko May 31, 2024

SophieTech88 Jun 1, 2024

codecov bot commented May 31, 2024 •

edited

Loading

chia7712 commented May 31, 2024

SophieTech88 commented Jun 1, 2024 •

edited

Loading

chia7712 Jun 1, 2024

SophieTech88 Jun 4, 2024 •

edited

Loading

chia7712 left a comment

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

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

Conversation

SophieTech88 commented May 26, 2024 • edited Loading

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

chia7712 left a 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

pbacsko May 28, 2024 • edited Loading

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

Choose a reason for hiding this comment

codecov bot commented May 31, 2024 • edited Loading

Codecov Report

chia7712 commented May 31, 2024

SophieTech88 commented Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

SophieTech88 Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

SophieTech88 commented May 26, 2024 •

edited

Loading

pbacsko May 28, 2024 •

edited

Loading

codecov bot commented May 31, 2024 •

edited

Loading

SophieTech88 commented Jun 1, 2024 •

edited

Loading

SophieTech88 Jun 4, 2024 •

edited

Loading