Fixes #1113 & #1131 and Resolves #1126; improves task creation process in `snapctl` #1127

tjmcs · 2016-08-05T20:07:33Z

Fixes #1113
Fixes #1131
Fixes #285
Resolves #1126

Summary of changes

There are two sets of changes in this pull request. The first, which fix Issue #1113 are fairly simple and don't change the behavior of the system at all. They can be summarized as follows:

Removed a line of code from the core/task.go file that used to set the max-failure value to 10 for all tasks, replacing it with a conditional that set the underlying option only in those cases where a MaxFailures value was included as part of the task creation request.
Modified the scheduler/task.go file so that the value of the DefaultStopOnFailure constant defined in that file is set to 10 (making it consistent with the behavior that used to be defined elsewhere but was inconsistently set to a value of 3 in this file)
Removed the definitions of a similar constant (the DefaultMaxFailures constant) from the cmd/snapctl/flags.go and mgmt/rest/flags.go files
Modified the command-line flag used to set the max-failures parameter so that it is a string flag rather than an integer flag and added code to parse this string value as an integer (and throw an error if the value passed in through this command-line flag cannot be parsed as an integer). This removes the string (default: 0) from the help output for the snapctl task create ... command.

The second set of changes were made to resolve Issue #1126. This set of changes does modify the behavior of the system, changing the snapctl command so that it honors any command-line flags that the user might have set, regardless of whether the user is creating a task using a task manifest or a workflow manifest. In the case where the user is creating a task using a task manifest and has included (new) definitions for parameters in appear that task manifest as command-line arguments, those command-line argument values will take precedence over the values defined in the task manifest. The changes in this second commit can best be summarized as follows:

Added methods to the task interface that can be used to parse any command-line parameters that might have been set by the user so that those command-line parameter values can be used when creating a new task. In the case where a workflow manifest is being used, those values will be used to set the necessary values for the underlying task. When a task manifest is being used those values will override the corresponding values from the task manifest file.
Added code to check the boundaries of a windowed task schedule (if one is defined in the task manifest and/or on the command-line) to ensure that the time window defined is valid.
Modified the functions used to create a task using either a task manifest or a workflow manifest so that they both call into our new methods to apply the command-line parameters defined by the user when creating the task.

Note; this pull request is a replacement for PR #1120, which fixed and resolved the same issues but in a single commit. This PR breaks out the fix into two separate commits, making it much clearer what is being changed and how.

Testing done

Tested the first set of changes manually by running two tests using two different task manifests:

For the first test, created a task using a task manifest that had an undefined value for the max-failures parameter, then unloading the collector plugin(s) that the task depended on and ensuring that the task was disabled after 10 failures (the default)
For the second test, created a task using a task manifest that had a value set for the max-failures parameter (the value we chose was 3, but any value greater than zero would do for this test), then unloading the collector plugin(s) that the task depended on and ensuring that the task was disabled after the chosen number of failures (in our case, 3).

The second set of changes were also tested manually by running a single test:

A task was created using the second a task manifest from our previous tests (which assigned a value of 3 to the max-failures parameter and a value of 1s for the interval), but passing in different values on the command-line for these two parameters (we chose values of 5 and 750ms, respectively, for our test).
For this test, we first ensured that our task was running at a shorter interval than the interval defined in the task manifest (it was, with the interval between task runs matching the 750ms value passed in on the command line; four times every three seconds or 80 times a minute).
We then unloaded the collector plugin(s) that the task depended on, and verified that the task was disabled after 5 failures rather than 3.

Finally, the current set of legacy tests was run to ensure that we hadn't changed the behavior of the system from the previous behavior, we'd only added the ability to override some of the parameters defined in the task manifest using values defined on the command-line.

@intelsdi-x/snap-maintainers

geauxvirtual · 2016-08-05T21:14:38Z

cmd/snapctl/task.go

+	// an error (at least some of the defined window should be in the future)
+	if time.Now().Add(createTaskNowPad).After(stop) {
+		return fmt.Errorf("Usage error (poorly defined time window); when constructing a new 'windowed' schedule, part of that window must be in the future")
+	} else if start.After(stop) {


I would remove the else if and just use if as each statement will return from the function with an error if the if statement is true.

geauxvirtual · 2016-08-08T21:23:01Z

cmd/snapctl/task.go

-	if t.MaxFailures == 0 {
-		fmt.Println("If the number of maximum failures is not specified, use default value of", DefaultMaxFailures)
-		t.MaxFailures = DefaultMaxFailures
+	// merge any CLI optiones specified by the user (if any) into the current task;


optiones -> options

geauxvirtual · 2016-08-08T21:55:31Z

cmd/snapctl/task.go

@@ -100,14 +100,225 @@ func createTask(ctx *cli.Context) error {
 	return err
 }

+func stringValToInt(val string) (int, error) {
+	// parse the input (string) value as an integer (log a Fatal


Maybe just word this comment as return an error instead of logging a fatal error message since we don't actually log a message in snapctl, but just return an error to the user.

…eation

IRCody · 2016-08-11T19:57:37Z

cmd/snapctl/task.go

+}
+
+// Parses the command-line parameters (if any) and uses them to override the underlying
+// sehedule for  this task or to set a schedule for that task (if one is not already defined,


schedule*

edit:
Also looks like there are 2 spaces between for and this in for this.

IRCody · 2016-08-12T04:45:17Z

LGTM. @geauxvirtual do you have any outstanding concerns?

geauxvirtual · 2016-08-12T16:03:53Z

cmd/snapctl/task.go

+	}
+	// if only the start date/time was specified, we will use it to replace
+	// the current schedule's start date/time
+	if start != nil && stop == nil {


Missing a condition if both start and stop are not nil to set StartTime and StopTime to those values.

@geauxvirtual; I just pushed up a change that I think addresses both this line comment and your next one (by ensuring that either value will be set if the corresponding input is non-nil)

geauxvirtual · 2016-08-12T22:34:15Z

cmd/snapctl/task.go

+	}
+	// if a start, stop, or duration value was provided, then it's a 'windowed' schedule
+	isWindowed := (start != nil || stop != nil || duration != nil)
+	// if there was an interval was passed in, then attempt to parse it (first as


if there was an interval passed in or if an interval was passed in

geauxvirtual · 2016-08-12T23:43:03Z

cmd/snapctl/task.go

+		// if start and stop were both defined, then return an error (since all three parameters cannot be used
+		// to define the 'windowed' schedule)
+		if start != nil && stop != nil {
+			return fmt.Errorf("Usage error (too many parameters); only two of the parameters that define the window (start time, stop time and duration) can be specified for a 'windowed' schedule")


This error message needs to be reworded. There are technically 5 parameters that can be specified for a windowed schedule: start-time, start-date, stop-time, stop-date, and duration.

geauxvirtual · 2016-08-15T17:56:07Z

cmd/snapctl/task.go

+		// if start is set and stop is not then use duration to create stop
+		if start != nil && stop == nil {
+			newStop := start.Add(*duration)
+			t.Schedule.StopTime = &newStop


Looks like this needs t.Schedule.StartTime = start

… when creating a task using snapctl

geauxvirtual · 2016-08-15T22:48:26Z

LGTM

geauxvirtual reviewed Aug 5, 2016
View reviewed changes

tjmcs force-pushed the tb/fix-task-create-and-disable branch 5 times, most recently from 8baf9fd to c604a50 Compare August 8, 2016 21:20

geauxvirtual reviewed Aug 8, 2016
View reviewed changes

tjmcs mentioned this pull request Aug 8, 2016

snapctl printing "If the number of maximum failures is not specified, use default value of x" #1131

Closed

lynxbat changed the title ~~Fixes #1113 and Resolves #1126; improves task creation process in snapctl~~ Fixes #1113 & #1131 and Resolves #1126; improves task creation process in snapctl Aug 8, 2016

geauxvirtual reviewed Aug 8, 2016
View reviewed changes

tjmcs force-pushed the tb/fix-task-create-and-disable branch 3 times, most recently from d4b8d54 to ce8cb50 Compare August 10, 2016 19:06

Fixes intelsdi-x#1113: removes fixed 'max-failure' value from task cr…

8dfc2f0

…eation

tjmcs force-pushed the tb/fix-task-create-and-disable branch from ce8cb50 to 7e2913e Compare August 10, 2016 19:20

IRCody reviewed Aug 11, 2016
View reviewed changes

tjmcs force-pushed the tb/fix-task-create-and-disable branch 3 times, most recently from 51a03eb to 24749fd Compare August 11, 2016 23:51

tjmcs force-pushed the tb/fix-task-create-and-disable branch from 24749fd to 667009f Compare August 12, 2016 15:33

geauxvirtual reviewed Aug 12, 2016
View reviewed changes

tjmcs force-pushed the tb/fix-task-create-and-disable branch from 667009f to 6461ec4 Compare August 12, 2016 16:18

geauxvirtual reviewed Aug 12, 2016
View reviewed changes

tjmcs force-pushed the tb/fix-task-create-and-disable branch from 6461ec4 to 86f287e Compare August 12, 2016 23:24

geauxvirtual reviewed Aug 12, 2016
View reviewed changes

tjmcs force-pushed the tb/fix-task-create-and-disable branch from 86f287e to 11fd0da Compare August 13, 2016 00:40

geauxvirtual reviewed Aug 15, 2016
View reviewed changes

tjmcs force-pushed the tb/fix-task-create-and-disable branch from 11fd0da to e8a77d3 Compare August 15, 2016 18:35

Fixes intelsdi-x#1126: Adds code to consistently apply CLI parameters…

e8a77d3

… when creating a task using snapctl

geauxvirtual mentioned this pull request Aug 15, 2016

Fix #285: Allow setting of task name in manifest with override from commandline flag #1123

Closed

tjmcs mentioned this pull request Aug 15, 2016

Fixes #1113: removes fixed max-failure value from task creation #1120

Closed

IRCody merged commit 4027d4a into intelsdi-x:master Aug 15, 2016

tjmcs deleted the tb/fix-task-create-and-disable branch August 15, 2016 23:54

IRCody mentioned this pull request Aug 18, 2016

Update strategy for dealing with failing plugins #708

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1113 & #1131 and Resolves #1126; improves task creation process in `snapctl` #1127

Fixes #1113 & #1131 and Resolves #1126; improves task creation process in `snapctl` #1127

tjmcs commented Aug 5, 2016 •

edited

Loading

geauxvirtual Aug 5, 2016 •

edited

Loading

geauxvirtual Aug 8, 2016

geauxvirtual Aug 8, 2016

IRCody Aug 11, 2016 •

edited

Loading

IRCody commented Aug 12, 2016

geauxvirtual Aug 12, 2016

tjmcs Aug 12, 2016

geauxvirtual Aug 12, 2016

geauxvirtual Aug 12, 2016

geauxvirtual Aug 15, 2016

geauxvirtual commented Aug 15, 2016

Fixes #1113 & #1131 and Resolves #1126; improves task creation process in snapctl #1127

Fixes #1113 & #1131 and Resolves #1126; improves task creation process in snapctl #1127

Conversation

tjmcs commented Aug 5, 2016 • edited Loading

geauxvirtual Aug 5, 2016 • edited Loading

Choose a reason for hiding this comment

geauxvirtual Aug 8, 2016

Choose a reason for hiding this comment

geauxvirtual Aug 8, 2016

Choose a reason for hiding this comment

IRCody Aug 11, 2016 • edited Loading

Choose a reason for hiding this comment

IRCody commented Aug 12, 2016

geauxvirtual Aug 12, 2016

Choose a reason for hiding this comment

tjmcs Aug 12, 2016

Choose a reason for hiding this comment

geauxvirtual Aug 12, 2016

Choose a reason for hiding this comment

geauxvirtual Aug 12, 2016

Choose a reason for hiding this comment

geauxvirtual Aug 15, 2016

Choose a reason for hiding this comment

geauxvirtual commented Aug 15, 2016

Fixes #1113 & #1131 and Resolves #1126; improves task creation process in `snapctl` #1127

Fixes #1113 & #1131 and Resolves #1126; improves task creation process in `snapctl` #1127

tjmcs commented Aug 5, 2016 •

edited

Loading

geauxvirtual Aug 5, 2016 •

edited

Loading

IRCody Aug 11, 2016 •

edited

Loading