Repository: argoproj/argo
Pull request #4417 (2020-10-29 13:19:09)

fix: Assume controller is in UTC when calculating NextScheduledRuntime
Fixes: https://github.com/argoproj/argo/issues/4325

**WIP**

Signed-off-by: Simon Behar <simbeh7@gmail.com>

Checklist:

* [ ] Either (a) I've created an [enhancement proposal](https://github.com/argoproj/argo/issues/new/choose) and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
* [ ] The title of the PR is (a) [conventional](https://www.conventionalcommits.org/en/v1.0.0/), (b) states what changed, and (c) suffixes the related issues number. E.g. `"fix(controller): Updates such and such. Fixes #1234"`.  
* [ ] My organization is added to [USERS.md](https://github.com/argoproj/argo/blob/master/USERS.md).
* [ ] I've signed the CLA.
* [ ] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
* [ ] My builds are green. Try syncing with master if they are not. 



Patch:
diff --git a/cmd/argo/commands/cron/get.go b/cmd/argo/commands/cron/get.go
index fcfa0dd89..84c3d9973 100644
--- a/cmd/argo/commands/cron/get.go
+++ b/cmd/argo/commands/cron/get.go
@@ -67,46 +67,46 @@ func printCronWorkflow(wf *wfv1.CronWorkflow, outFmt string) {
 	}
 }
 
-func getCronWorkflowGet(wf *wfv1.CronWorkflow) string {
+func getCronWorkflowGet(cwf *wfv1.CronWorkflow) string {
 	const fmtStr = "%-30s %v\n"
 
 	out := ""
-	out += fmt.Sprintf(fmtStr, "Name:", wf.ObjectMeta.Name)
-	out += fmt.Sprintf(fmtStr, "Namespace:", wf.ObjectMeta.Namespace)
-	out += fmt.Sprintf(fmtStr, "Created:", humanize.Timestamp(wf.ObjectMeta.CreationTimestamp.Time))
-	out += fmt.Sprintf(fmtStr, "Schedule:", wf.Spec.Schedule)
-	out += fmt.Sprintf(fmtStr, "Suspended:", wf.Spec.Suspend)
-	if wf.Spec.Timezone != "" {
-		out += fmt.Sprintf(fmtStr, "Timezone:", wf.Spec.Timezone)
+	out += fmt.Sprintf(fmtStr, "Name:", cwf.ObjectMeta.Name)
+	out += fmt.Sprintf(fmtStr, "Namespace:", cwf.ObjectMeta.Namespace)
+	out += fmt.Sprintf(fmtStr, "Created:", humanize.Timestamp(cwf.ObjectMeta.CreationTimestamp.Time))
+	out += fmt.Sprintf(fmtStr, "Schedule:", cwf.Spec.Schedule)
+	out += fmt.Sprintf(fmtStr, "Suspended:", cwf.Spec.Suspend)
+	if cwf.Spec.Timezone != "" {
+		out += fmt.Sprintf(fmtStr, "Timezone:", cwf.Spec.Timezone)
 	}
-	if wf.Spec.StartingDeadlineSeconds != nil {
-		out += fmt.Sprintf(fmtStr, "StartingDeadlineSeconds:", *wf.Spec.StartingDeadlineSeconds)
+	if cwf.Spec.StartingDeadlineSeconds != nil {
+		out += fmt.Sprintf(fmtStr, "StartingDeadlineSeconds:", *cwf.Spec.StartingDeadlineSeconds)
 	}
-	if wf.Spec.ConcurrencyPolicy != "" {
-		out += fmt.Sprintf(fmtStr, "ConcurrencyPolicy:", wf.Spec.ConcurrencyPolicy)
+	if cwf.Spec.ConcurrencyPolicy != "" {
+		out += fmt.Sprintf(fmtStr, "ConcurrencyPolicy:", cwf.Spec.ConcurrencyPolicy)
 	}
-	if wf.Status.LastScheduledTime != nil {
-		out += fmt.Sprintf(fmtStr, "LastScheduledTime:", humanize.Timestamp(wf.Status.LastScheduledTime.Time))
+	if cwf.Status.LastScheduledTime != nil {
+		out += fmt.Sprintf(fmtStr, "LastScheduledTime:", humanize.Timestamp(cwf.Status.LastScheduledTime.Time))
 	}
 
-	next, err := wf.GetNextRuntime()
+	next, err := GetNextRuntime(cwf)
 	if err == nil {
-		out += fmt.Sprintf(fmtStr, "NextScheduledTime:", humanize.Timestamp(next))
+		out += fmt.Sprintf(fmtStr, "NextScheduledTime:", humanize.Timestamp(next)+" (assumes workflow-controller is in UTC)")
 	}
 
-	if len(wf.Status.Active) > 0 {
+	if len(cwf.Status.Active) > 0 {
 		var activeWfNames []string
-		for _, activeWf := range wf.Status.Active {
+		for _, activeWf := range cwf.Status.Active {
 			activeWfNames = append(activeWfNames, activeWf.Name)
 		}
 		out += fmt.Sprintf(fmtStr, "Active Workflows:", strings.Join(activeWfNames, ", "))
 	}
-	if len(wf.Status.Conditions) > 0 {
-		out += wf.Status.Conditions.DisplayString(fmtStr, map[wfv1.ConditionType]string{wfv1.ConditionTypeSubmissionError: "✖"})
+	if len(cwf.Status.Conditions) > 0 {
+		out += cwf.Status.Conditions.DisplayString(fmtStr, map[wfv1.ConditionType]string{wfv1.ConditionTypeSubmissionError: "✖"})
 	}
-	if len(wf.Spec.WorkflowSpec.Arguments.Parameters) > 0 {
+	if len(cwf.Spec.WorkflowSpec.Arguments.Parameters) > 0 {
 		out += fmt.Sprintf(fmtStr, "Workflow Parameters:", "")
-		for _, param := range wf.Spec.WorkflowSpec.Arguments.Parameters {
+		for _, param := range cwf.Spec.WorkflowSpec.Arguments.Parameters {
 			if param.Value == nil {
 				continue
 			}
diff --git a/cmd/argo/commands/cron/list.go b/cmd/argo/commands/cron/list.go
index f9cd19dd7..5c97e83f6 100644
--- a/cmd/argo/commands/cron/list.go
+++ b/cmd/argo/commands/cron/list.go
@@ -70,23 +70,23 @@ func printTable(wfList []wfv1.CronWorkflow, listArgs *listFlags) {
 	}
 	_, _ = fmt.Fprint(w, "NAME\tAGE\tLAST RUN\tNEXT RUN\tSCHEDULE\tSUSPENDED")
 	_, _ = fmt.Fprint(w, "\n")
-	for _, wf := range wfList {
+	for _, cwf := range wfList {
 		if listArgs.allNamespaces {
-			_, _ = fmt.Fprintf(w, "%s\t", wf.ObjectMeta.Namespace)
+			_, _ = fmt.Fprintf(w, "%s\t", cwf.ObjectMeta.Namespace)
 		}
 		var cleanLastScheduledTime string
-		if wf.Status.LastScheduledTime != nil {
-			cleanLastScheduledTime = humanize.RelativeDurationShort(wf.Status.LastScheduledTime.Time, time.Now())
+		if cwf.Status.LastScheduledTime != nil {
+			cleanLastScheduledTime = humanize.RelativeDurationShort(cwf.Status.LastScheduledTime.Time, time.Now())
 		} else {
 			cleanLastScheduledTime = "N/A"
 		}
 		var cleanNextScheduledTime string
-		if next, err := wf.GetNextRuntime(); err == nil {
+		if next, err := GetNextRuntime(&cwf); err == nil {
 			cleanNextScheduledTime = humanize.RelativeDurationShort(next, time.Now())
 		} else {
 			cleanNextScheduledTime = "N/A"
 		}
-		_, _ = fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%t", wf.ObjectMeta.Name, humanize.RelativeDurationShort(wf.ObjectMeta.CreationTimestamp.Time, time.Now()), cleanLastScheduledTime, cleanNextScheduledTime, wf.Spec.Schedule, wf.Spec.Suspend)
+		_, _ = fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%t", cwf.ObjectMeta.Name, humanize.RelativeDurationShort(cwf.ObjectMeta.CreationTimestamp.Time, time.Now()), cleanLastScheduledTime, cleanNextScheduledTime, cwf.Spec.Schedule, cwf.Spec.Suspend)
 		_, _ = fmt.Fprintf(w, "\n")
 	}
 	_ = w.Flush()
diff --git a/cmd/argo/commands/cron/root.go b/cmd/argo/commands/cron/root.go
index bf2445311..ec67ef680 100644
--- a/cmd/argo/commands/cron/root.go
+++ b/cmd/argo/commands/cron/root.go
@@ -7,7 +7,7 @@ import (
 func NewCronWorkflowCommand() *cobra.Command {
 	var command = &cobra.Command{
 		Use:   "cron",
-		Short: "manage cron workflows",
+		Short: "manage cron workflows\n\nNextScheduledRun assumes that the workflow-controller uses UTC as its timezone",
 		Run: func(cmd *cobra.Command, args []string) {
 			cmd.HelpFunc()(cmd, args)
 		},
diff --git a/cmd/argo/commands/cron/util.go b/cmd/argo/commands/cron/util.go
new file mode 100644
index 000000000..0e50a1582
--- /dev/null
+++ b/cmd/argo/commands/cron/util.go
@@ -0,0 +1,23 @@
+package cron
+
+import (
+	"time"
+
+	"github.com/robfig/cron/v3"
+
+	"github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
+)
+
+// GetNextRuntime returns the next time the workflow should run in local time. It assumes the workflow-controller is in
+// UTC, but nevertheless returns the time in the local timezone.
+func GetNextRuntime(cwf *v1alpha1.CronWorkflow) (time.Time, error) {
+	cronScheduleString := cwf.Spec.Schedule
+	if cwf.Spec.Timezone != "" {
+		cronScheduleString = "CRON_TZ=" + cwf.Spec.Timezone + " " + cronScheduleString
+	}
+	cronSchedule, err := cron.ParseStandard(cronScheduleString)
+	if err != nil {
+		return time.Time{}, err
+	}
+	return cronSchedule.Next(time.Now().UTC()).Local(), nil
+}
diff --git a/docs/cron-workflows.md b/docs/cron-workflows.md
index a3eff667c..d2356b630 100644
--- a/docs/cron-workflows.md
+++ b/docs/cron-workflows.md
@@ -90,15 +90,18 @@ test-cron-wf   56s   2s         * * * * *   false
 $ argo cron get test-cron-wf
 Name:                          test-cron-wf
 Namespace:                     argo
-Created:                       Mon Nov 18 10:17:06 -0800 (4 minutes ago)
+Created:                       Wed Oct 28 07:19:02 -0600 (23 hours ago)
 Schedule:                      * * * * *
 Suspended:                     false
 StartingDeadlineSeconds:       0
 ConcurrencyPolicy:             Replace
-LastScheduledTime:             Mon Nov 18 10:21:00 -0800 (51 seconds ago)
+LastScheduledTime:             Thu Oct 29 06:51:00 -0600 (11 minutes ago)
+NextScheduledTime:             Thu Oct 29 13:03:00 +0000 (32 seconds from now)
 Active Workflows:              test-cron-wf-rt4nf
 ```
 
+**Note**: `NextScheduledRun` assumes that the workflow-controller uses UTC as its timezone
+
 ### `kubectl`
 
 Using `kubectl apply -f` and `kubectl get cwf`
diff --git a/pkg/apis/workflow/v1alpha1/cron_workflow_types.go b/pkg/apis/workflow/v1alpha1/cron_workflow_types.go
index 38121822b..b467fabff 100644
--- a/pkg/apis/workflow/v1alpha1/cron_workflow_types.go
+++ b/pkg/apis/workflow/v1alpha1/cron_workflow_types.go
@@ -1,9 +1,6 @@
 package v1alpha1
 
 import (
-	"time"
-
-	"github.com/robfig/cron/v3"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -20,18 +17,6 @@ type CronWorkflow struct {
 	Status            CronWorkflowStatus `json:"status,omitempty" protobuf:"bytes,3,opt,name=status"`
 }
 
-func (cwf *CronWorkflow) GetNextRuntime() (time.Time, error) {
-	cronScheduleString := cwf.Spec.Schedule
-	if cwf.Spec.Timezone != "" {
-		cronScheduleString = "CRON_TZ=" + cwf.Spec.Timezone + " " + cronScheduleString
-	}
-	cronSchedule, err := cron.ParseStandard(cronScheduleString)
-	if err != nil {
-		return time.Time{}, err
-	}
-	return cronSchedule.Next(time.Now()), nil
-}
-
 // CronWorkflowList is list of CronWorkflow resources
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 type CronWorkflowList struct {
diff --git a/ui/src/app/cron-workflows/components/cron-workflow-summary-panel.tsx b/ui/src/app/cron-workflows/components/cron-workflow-summary-panel.tsx
index f3478b91b..aa35424a7 100644
--- a/ui/src/app/cron-workflows/components/cron-workflow-summary-panel.tsx
+++ b/ui/src/app/cron-workflows/components/cron-workflow-summary-panel.tsx
@@ -35,7 +35,10 @@ export const CronWorkflowSummaryPanel = (props: Props) => {
     const statusAttributes = [
         {title: 'Active', value: props.cronWorkflow.status.active ? getCronWorkflowActiveWorkflowList(props.cronWorkflow.status.active) : <i>No Workflows Active</i>},
         {title: 'Last Scheduled Time', value: props.cronWorkflow.status.lastScheduledTime},
-        {title: 'Next Scheduled Time', value: getNextScheduledTime(props.cronWorkflow.spec.schedule, props.cronWorkflow.spec.timezone)},
+        {
+            title: 'Next Scheduled Time',
+            value: getNextScheduledTime(props.cronWorkflow.spec.schedule, props.cronWorkflow.spec.timezone) + ' (assumes workflow-controller is in UTC)'
+        },
         {title: 'Conditions', value: <ConditionsPanel conditions={props.cronWorkflow.status.conditions} />}
     ];
     return (
@@ -92,7 +95,7 @@ function getNextScheduledTime(schedule: string, tz: string): string {
     let out = '';
     try {
         out = parser
-            .parseExpression(schedule, {tz})
+            .parseExpression(schedule, {utc: !tz, tz})
             .next()
             .toISOString();
     } catch (e) {


Test patch:
diff --git a/cmd/argo/commands/cron/get_test.go b/cmd/argo/commands/cron/get_test.go
index 5204c1576..85564d65e 100644
--- a/cmd/argo/commands/cron/get_test.go
+++ b/cmd/argo/commands/cron/get_test.go
@@ -68,7 +68,7 @@ func TestNextRuntime(t *testing.T) {
 	var cronWf v1alpha1.CronWorkflow
 	err := yaml.Unmarshal([]byte(invalidCwf), &cronWf)
 	if assert.NoError(t, err) {
-		next, err := cronWf.GetNextRuntime()
+		next, err := GetNextRuntime(&cronWf)
 		if assert.NoError(t, err) {
 			assert.LessOrEqual(t, next.Unix(), time.Now().Add(1*time.Minute).Unix())
 			assert.Greater(t, next.Unix(), time.Now().Unix())