Merge pull request #61 from kinvolk/invidian/more-fixes

Updates and improvements

Author: invidian

.github/workflows/ci.yaml

@@ -27,7 +27,7 @@ jobs:
   lint:
     name: Lint
     runs-on: ubuntu-latest
-    container: golangci/golangci-lint:v1.32.2
+    container: golangci/golangci-lint:v1.35.2
     timeout-minutes: 10
     steps:
 

.golangci.yml

@@ -22,6 +22,13 @@ issues:
     - linters:
         - lll
       source: "http(s)://"
+    # Allow use of fmt.Print* functions.
+    - linters:
+      - forbidigo
+      text: "use of `fmt.Printf` forbidden by pattern"
+    - linters:
+      - forbidigo
+      text: "use of `fmt.Println` forbidden by pattern"
 
 linters-settings:
   exhaustive:
@@ -42,6 +49,7 @@ linters:
     - errorlint
     - exhaustive
     - exportloopref
+    - forbidigo
     - funlen
     - gochecknoinits
     - gocognit
@@ -60,19 +68,23 @@ linters:
     - ineffassign
     - interfacer
     - lll
+    - makezero
     - misspell
     - nakedret
     - nestif
     - nlreturn
     - noctx
     - nolintlint
+    - paralleltest
     - prealloc
+    - predeclared
     - rowserrcheck
     - scopelint
     - sqlclosecheck
     - structcheck
     - stylecheck
     - testpackage
+    - thelper
     - tparallel
     - typecheck
     - unconvert
@@ -82,9 +94,15 @@ linters:
     - wrapcheck
     - wsl
   disable:
+    # We use different import grouping style than gci forces.
     - gci
+    # We still have many globals to get rid of.
     - gochecknoglobals
+    # TODOs in the code are OK.
     - godox
+    # We do not have clearly defined error types yet.
     - goerr113
+    # We do not require all used structs to have all fields initialized.
     - exhaustivestruct
+    # We do not care about the memory efficiency really.
     - maligned

pkg/agent/agent.go

@@ -37,7 +37,10 @@ type Klocksmith struct {
 	reapTimeout time.Duration
 }
 
-const defaultPollInterval = 10 * time.Second
+const (
+	defaultPollInterval     = 10 * time.Second
+	maxOperatorResponseTime = 24 * time.Hour
+)
 
 var shouldRebootSelector = fields.Set(map[string]string{
 	constants.AnnotationOkToReboot:   constants.True,
@@ -354,7 +357,7 @@ func (k *Klocksmith) waitForOkToReboot() error {
 
 	// Hopefully 24 hours is enough time between indicating we need a
 	// reboot and the controller telling us to do it.
-	ctx, _ := watchtools.ContextWithOptionalTimeout(context.Background(), time.Hour*24)
+	ctx, _ := watchtools.ContextWithOptionalTimeout(context.Background(), maxOperatorResponseTime)
 
 	ev, err := watchtools.UntilWithoutRetry(ctx, watcher, k8sutil.NodeAnnotationCondition(shouldRebootSelector))
 	if err != nil {
@@ -400,7 +403,7 @@ func (k *Klocksmith) waitForNotOkToReboot() error {
 	// true' vs '== False'; due to the operator matching on '== True', and not
 	// going out of its way to convert '' => 'False', checking the exact inverse
 	// of what the operator checks is the correct thing to do.
-	ctx, _ := watchtools.ContextWithOptionalTimeout(context.Background(), time.Hour*24)
+	ctx, _ := watchtools.ContextWithOptionalTimeout(context.Background(), maxOperatorResponseTime)
 
 	ev, err := watchtools.UntilWithoutRetry(ctx, watcher, watchtools.ConditionFunc(func(event watch.Event) (bool, error) {
 		switch event.Type {

pkg/k8sutil/metadata_internal_test.go

@@ -6,6 +6,8 @@ import (
 )
 
 func Test_slitNewlineEnv_retain_map_values_when_given_empty_input(t *testing.T) {
+	t.Parallel()
+
 	expected := map[string]string{"foo": "bar"}
 
 	input := map[string]string{}

pkg/k8sutil/metadata_test.go

@@ -36,6 +36,8 @@ func atomicCounterIncrement(n *corev1.Node) {
 }
 
 func TestUpdateNodeRetryHandlesConflict(t *testing.T) {
+	t.Parallel()
+
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
 	mockNi := mock_v1.NewMockNodeInterface(ctrl)

pkg/operator/operator.go

@@ -276,8 +276,7 @@ func (k *Kontroller) process() {
 	// fix them.
 	klog.V(4).Info("Cleaning up node state")
 
-	err := k.cleanupState()
-	if err != nil {
+	if err := k.cleanupState(); err != nil {
 		klog.Errorf("Failed to cleanup node state: %v", err)
 
 		return
@@ -289,8 +288,7 @@ func (k *Kontroller) process() {
 	// the reboot has completed.
 	klog.V(4).Info("Checking if configured after-reboot annotations are set to true")
 
-	err = k.checkAfterReboot()
-	if err != nil {
+	if err := k.checkAfterReboot(); err != nil {
 		klog.Errorf("Failed to check after reboot: %v", err)
 
 		return
@@ -300,8 +298,7 @@ func (k *Kontroller) process() {
 	// remove after-reboot annotations and add the after-reboot=true label.
 	klog.V(4).Info("Labeling rebooted nodes with after-reboot label")
 
-	err = k.markAfterReboot()
-	if err != nil {
+	if err := k.markAfterReboot(); err != nil {
 		klog.Errorf("Failed to update recently rebooted nodes: %v", err)
 
 		return
@@ -313,8 +310,7 @@ func (k *Kontroller) process() {
 	// time to reboot.
 	klog.V(4).Info("Checking if configured before-reboot annotations are set to true")
 
-	err = k.checkBeforeReboot()
-	if err != nil {
+	if err := k.checkBeforeReboot(); err != nil {
 		klog.Errorf("Failed to check before reboot: %v", err)
 
 		return
@@ -324,8 +320,7 @@ func (k *Kontroller) process() {
 	// annotations and add the before-reboot=true label.
 	klog.V(4).Info("Labeling rebootable nodes with before-reboot label")
 
-	err = k.markBeforeReboot()
-	if err != nil {
+	if err := k.markBeforeReboot(); err != nil {
 		klog.Errorf("Failed to update rebootable nodes: %v", err)
 
 		return