Fixes #1134, fixed support for windowed schedule

[katarzyna-z]

Fixes #1134

Summary of changes:

• Fixed support for windowed schedule:
  • Added tags for JSON parser in Schedule struct (mgmt/rest/client/task.go). JSON parser requires tags to deal with unmarshalling into Schedule struct,
  • unified structures for schedule configuration,
• Added user friendly errors to inform about missing parameters in configuration of schedule,
• Tests to cover changes,
• Updated documentation for windowed schedule.

Testing done:

• small, medium, legacy tests,
• manual tests

@intelsdi-x/snap-maintainers

[candysmurf]

a couple of issues:

• when not giving the start time or stop time, errors.
  From the code, they both should work when the interval is giving.

Error creating task:missing parameter/parameters in configuration of windowed schedule,start_timestamp: 2016-10-27 16:50:00 +0000 UTC, stop_timestamp: <nil>, interval: 1s

Error creating task:missing parameter/parameters in configuration of windowed schedule,start_timestamp: <nil>, stop_timestamp: 2016-10-27 17:50:00 +0000 UTC, interval: 1s

• When only the interval is specified, errored out. From the code, it should work. As it uses the current time + 1 second as the start time.
• When both start & stop time specified but no interval, errored out. It should work from the code.

Usage error (missing interval value); when constructing a new task schedule an interval must be provided

• If stop time is current but the start time is in the past, it ran.
• timestamp without timezone info went into error. Probably highlight this point to users.

[candysmurf]

Not commenting on the code change here but the overall logic. This method did all the hard work to define schedule.StartTimestamp and StopTimestamp, but at the end it uses whatever start and stop time no matter if the duration is specified or not. Do I miss anything?

[katarzyna-z]

@candysmurf
setWindowedSchedule method is only called from setScheduleFromCliOptions method which is in use when task is create with -w option and schedule is created using snapct options and the schedule type is not explicitly given by user.

See the following information:

snapctl task create -w 
Incorrect Usage.

NAME:
   snapctl task create - There are two ways to create a task.
    1) Use a task manifest with [--task-manifest]
    2) Provide a workflow manifest and schedule details.

    * Note: Start and stop date/time are optional.


USAGE:
   snapctl task create [command options] [arguments...]

DESCRIPTION:
   Creates a new task in the snap scheduler

OPTIONS:
   --task-manifest value, -t value  File path for task manifest to use for task creation.
   --workflow-manifest value, -w value  File path for workflow manifest to use for task creation
   --interval value, -i value       Interval for the task schedule [ex (simple schedule): 250ms, 1s, 30m (cron schedule): "0 * * * * *"]
   --start-date value           Start date for the task schedule [defaults to today]
   --start-time value           Start time for the task schedule [defaults to now]
   --stop-date value            Stop date for the task schedule [defaults to today]
   --stop-time value            Start time for the task schedule [defaults to now]
   --name value, -n value       Optional requirement for giving task names
   --duration value, -d value       The amount of time to run the task [appends to start or creates a start time before a stop]
   --no-start               Do not start task on creation [normally started on creation]
   --deadline value         The deadline for the task to be killed after started if the task runs too long (All tasks default to 5s)
   --max-failures value         The number of consecutive failures before snap disables the task

flag needs an argument: -w

Take into account that task could be created using

• snapclt with -t option
• snapctl with -w option
• using API request, e.g: curl -vXPOST http://localhost:8181/v1/tasks -d @mock-file.json --header "Content-Type: application/json"

It seems that to analyze in the code the expected behavior for tasks create with -t option or using API requests you should take a look at core/schedule.go and makeSchedule method.

Could you verify it?

If after verification you still see issues please provide snapctl commands or API requests with task manifests. It will be easier to see the same behaviour and correct if needed.

[candysmurf]

can you give another alternative by specifying Z?

[katarzyna-z]

I added example with 'Z', please check if now it looks good.

[candysmurf]

startTime and stopTime only used once. It seems using v.StartTime and v.StopTime should suffice.

[katarzyna-z]

Thanks for catching it. Corrected.

[katarzyna-z]

@candysmurf

If stop time is current but the start time is in the past, it ran.

I see the following test case in the code

Convey("start time in past is ok (as long as window ends in the future)", func() {
            start := time.Now().Add(time.Second * -10)
            stop := time.Now().Add(time.Second * 10)
            w := NewWindowedSchedule(time.Millisecond*100, &start, &stop)
            err := w.Validate()
            So(err, ShouldEqual, nil)
})

but the boundary for the now and the past could be thin if you set current time in the task manifest as stop time... I don't know how you set the stop time as current. Please check if it works as it was desired.

[candysmurf]

@katarzyna-z, thanks for your work. I tried a few scenarios. Issues are not necessary caused by this PR.

• When 2 tasks are already running, when posting a new task, the newly created task is in the "stopped" state:

 curl -vPOST http://localhost:8181/v1/tasks -d @task-mock-file.json --header "Content-Type: application/json"

• Error messages are different depending on the way to create tasks.

When creating from snapctl with options:

▶ snapctl task create -t ~/task/task-mock-file-1.json -i 2s --start-time 2016-11-01T16:39:57Z
Using task manifest to create task
Error creating task:
parsing time "2016-11-01T16:39:57Z": hour out of range

When creating from task manifest only

▶ snapctl task create -t ~/task/task-mock-file-1.json
Using task manifest to create task
Usage error (missing interval value); when constructing a new task schedule an interval must be provided

The task manifest used:

 "schedule": {
        "type": "windowed",
        "start_timestamp": "2016-10-17T16:39:57Z"
    },

Same for the interval. E.g:

▶ snapctl task create -t ~/task/task-mock-file-1.json -i 2s
Using task manifest to create task
Error creating task:missing parameter/parameters in configuration of windowed schedule,start_timestamp: <nil>, stop_timestamp: <nil>, interval: 2s

• When using interval only from CLI as below, it gave a weird message. There is no indication of what's a good format.

▶ snapctl task create -t ~/task/task-mock-file-1.json -i 2s --start-date 2016-10-31
Using task manifest to create task
Error creating task:
parsing time "2016-10-31": month out of range

There is an inconsistency between the existing code base and your PR. How do you like to address this? Should we open a new issue to address these?

[katarzyna-z]

For these errors :

snapctl task create -t ~/task/task-mock-file-1.json -i 2s --start-date 2016-10-31
Using task manifest to create task
Error creating task:
parsing time "2016-10-31": month out of range

and

snapctl task create -t ~/task/task-mock-file-1.json -i 2s --start-time 2016-11-01T16:39:57Z
Using task manifest to create task
Error creating task:
parsing time "2016-11-01T16:39:57Z": hour out of range

please take a look at mergeDataTime function which is called from setScheduleFromCliOptions method, see:

var (
    // padding to picking a time to start a "NOW" task
    createTaskNowPad = time.Second * 1
    timeParseFormat  = "3:04PM"
    dateParseFormat  = "1-02-2006"
    unionParseFormat = timeParseFormat + " " + dateParseFormat
)

and

func mergeDateTime(tm, dt string) *time.Time {
    reTm := time.Now().Add(createTaskNowPad)
    if dt == "" && tm == "" {
        return nil
    }
    if dt != "" {
        t, err := time.Parse(dateParseFormat, dt)
        if err != nil {
            fmt.Printf("Error creating task:\n%v\n", err)
            os.Exit(1)
        }
        reTm = t
    }

    if tm != "" {
        _, err := time.ParseInLocation(timeParseFormat, tm, time.Local)
        if err != nil {
            fmt.Printf("Error creating task:\n%v\n", err)
            os.Exit(1)
        }
        reTm, err = time.ParseInLocation(unionParseFormat, fmt.Sprintf("%s %s", tm, reTm.Format(dateParseFormat)), time.Local)
        if err != nil {
            fmt.Printf("Error creating task:\n%v\n", err)
            os.Exit(1)
        }
    }
    return &reTm
}

you can quickly reproduce issues with this example:

package main

import (
    "fmt"
    "os"
    "time"
)

var (
    createTaskNowPad = time.Second * 1
    timeParseFormat  = "3:04PM"
    dateParseFormat  = "1-02-2006"
    unionParseFormat = timeParseFormat + " " + dateParseFormat
)

func mergeDateTime(tm, dt string) *time.Time {
    reTm := time.Now().Add(createTaskNowPad)
    if dt == "" && tm == "" {
        return nil
    }
    if dt != "" {
        t, err := time.Parse(dateParseFormat, dt)
        if err != nil {
            fmt.Printf("Error creating task:\n%v\n", err)
            os.Exit(1)
        }
        reTm = t
    }

    if tm != "" {
        _, err := time.ParseInLocation(timeParseFormat, tm, time.Local)
        if err != nil {
            fmt.Printf("Error creating task:\n%v\n", err)
            os.Exit(1)
        }
        reTm, err = time.ParseInLocation(unionParseFormat, fmt.Sprintf("%s %s", tm, reTm.Format(dateParseFormat)), time.Local)
        if err != nil {
            fmt.Printf("Error creating task:\n%v\n", err)
            os.Exit(1)
        }
    }
    return &reTm
}

func main() {
    fmt.Println(mergeDateTime("", "2016-10-31"))
}

so snapctl uses specific format for start/stop time and date but it seems there is nowhere information about it. I think that this should be addressed as a new issue.

I've opened #1328.

[katarzyna-z]

Error messages are different depending on the way to create tasks.

snapctl builds POST request which could be send also using for example curl. snapctl gives the user possibility to create API request using options. There is specific logic implemented to check options and give user some tips, see setScheduleFromCliOptions method in cmd/snapctl/tasks.go.
If user sends the raw request (for example using curl) then the layer which is created through snapctl is omitted so it seems that errors messages could be different depending on the way to create tasks because parameters are checked on different level.

But the logic implemented to check options and build schedule from cli options is quite complicated, please see setScheduleFromCliOptions method in cmd/snapctl/tasks.go especially this part of code:

// if a start, stop, or duration value was provided, or if the existing schedule for this task
// is 'windowed', then it's a 'windowed' schedule
isWindowed := (start != nil || stop != nil || duration != nil || t.Schedule.Type == "windowed")

There were some intentions and it must be checked carefully so I think this should be also addressed through a new issue.

[katarzyna-z]

When 2 tasks are already running, when posting a new task, the newly created task is in the "stopped" state:
curl -vPOST http://localhost:8181/v1/tasks -d @task-mock-file.json --header "Content-T

I checked this with the latest snap.
if task is created through raw API request then task is "Stopped" when the same task is created through snapctl then the state of task is "Running". I tested with this task manifest:

{
    "version": 1,
    "schedule": {
        "type": "simple",
        "interval": "1s"
    },
    "max-failures": 10,
    "workflow": {
        "collect": {
            "metrics": {
                "/intel/mock/foo": {},
                "/intel/mock/bar": {},
                "/intel/mock/*/baz": {}
            },
            "config": {
                "/intel/mock": {
                    "name": "root",
                    "password": "secret"
                }
            },
            "process": [
                {
                    "plugin_name": "passthru",
                    "process": null,
                    "publish": [
                        {
                            "plugin_name": "mock-file",
                            "config": {
                                "file": "/tmp/snap_published_mock_file.log"
                            }
                        }
                    ]
                }
            ]
        }
    }
}

When user uses raw API request to create task and wants to start this task then the user must define start parameter in the task manifest and set it's value to true, e.g.:

{
    "version": 1,
    "schedule": {
        "type": "simple",
        "interval": "1s"
    },
    "start": true,
    "max-failures": 10,
    "workflow": {
        "collect": {
            "metrics": {
                "/intel/mock/foo": {},
                "/intel/mock/bar": {},
                "/intel/mock/*/baz": {}
            },
            "config": {
                "/intel/mock": {
                    "name": "root",
                    "password": "secret"
                }
            },
            "process": [
                {
                    "plugin_name": "passthru",
                    "process": null,
                    "publish": [
                        {
                            "plugin_name": "mock-file",
                            "config": {
                                "file": "/tmp/snap_published_mock_file.log"
                            }
                        }
                    ]
                }
            ]
        }
    }
}