Fix status.lastRegistrationCheckTime in body must be of type string: \"null\" errors (#407)

Follow-up for #398 and #404

api/v1alpha1/runner_types.go

@@ -121,13 +121,17 @@ func (rs *RunnerSpec) ValidateRepository() error {
 
 // RunnerStatus defines the observed state of Runner
 type RunnerStatus struct {
+	// +optional
 	Registration RunnerStatusRegistration `json:"registration"`
-	Phase        string                   `json:"phase"`
-	Reason       string                   `json:"reason"`
-	Message      string                   `json:"message"`
-
-	//+optional
-	LastRegistrationCheckTime *metav1.Time `json:"lastRegistrationCheckTime"`
+	// +optional
+	Phase string `json:"phase,omitempty"`
+	// +optional
+	Reason string `json:"reason,omitempty"`
+	// +optional
+	Message string `json:"message,omitempty"`
+	// +optional
+	// +nullable
+	LastRegistrationCheckTime *metav1.Time `json:"lastRegistrationCheckTime,omitempty"`
 }
 
 // RunnerStatusRegistration contains runner registration status

charts/actions-runner-controller/Chart.yaml

@@ -15,7 +15,7 @@ type: application
 # This is the chart version. This version number should be incremented each time you make changes
 # to the chart and its templates, including the app version.
 # Versions are expected to follow Semantic Versioning (https://semver.org/)
-version: 0.10.2
+version: 0.10.4
 
 home: https://github.com/summerwind/actions-runner-controller
 

charts/actions-runner-controller/crds/actions.summerwind.dev_runners.yaml

@@ -1543,6 +1543,7 @@ spec:
           properties:
             lastRegistrationCheckTime:
               format: date-time
+              nullable: true
               type: string
             message:
               type: string
@@ -1572,11 +1573,6 @@ spec:
                 - expiresAt
                 - token
               type: object
-          required:
-            - message
-            - phase
-            - reason
-            - registration
           type: object
       type: object
   version: v1alpha1

config/crd/bases/actions.summerwind.dev_runners.yaml

@@ -1543,6 +1543,7 @@ spec:
           properties:
             lastRegistrationCheckTime:
               format: date-time
+              nullable: true
               type: string
             message:
               type: string
@@ -1572,11 +1573,6 @@ spec:
                 - expiresAt
                 - token
               type: object
-          required:
-            - message
-            - phase
-            - reason
-            - registration
           type: object
       type: object
   version: v1alpha1

controllers/runner_controller.go

@@ -255,15 +255,25 @@ func (r *RunnerReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 			// achieves that.
 			if lastCheckTime := runner.Status.LastRegistrationCheckTime; lastCheckTime != nil {
 				nextCheckTime := lastCheckTime.Add(registrationCheckInterval)
-				if nextCheckTime.After(time.Now()) {
+				now := time.Now()
+				if nextCheckTime.After(now) {
+					requeueAfter := nextCheckTime.Sub(now)
+
 					log.Info(
-						fmt.Sprintf("Skipping registration check because it's deferred until %s", nextCheckTime),
+						fmt.Sprintf("Skipped registration check because it's deferred until %s. Retrying in %s at latest", nextCheckTime, requeueAfter),
+						"lastRegistrationCheckTime", lastCheckTime,
+						"registrationCheckInterval", registrationCheckInterval,
 					)
 
-					// Note that we don't need to explicitly requeue on this reconcilation because
-					// the requeue should have been already scheduled previsouly
-					// (with `return ctrl.Result{RequeueAfter: registrationRecheckDelay}, nil` as noted above and coded below)
-					return ctrl.Result{}, nil
+					// Without RequeueAfter, the controller may not retry on scheduled. Instead, it must wait until the
+					// next sync period passes, which can be too much later than nextCheckTime.
+					//
+					// We need to requeue on this reconcilation even though we have already scheduled the initial
+					// requeue previously with `return ctrl.Result{RequeueAfter: registrationRecheckDelay}, nil`.
+					// Apparently, the workqueue used by controller-runtime seems to deduplicate and resets the delay on
+					// other requeues- so the initial scheduled requeue may have been reset due to requeue on
+					// spec/status change.
+					return ctrl.Result{RequeueAfter: requeueAfter}, nil
 				}
 			}