Add tasks creation and activation when some found in auto_discover_path #951
Conversation
|
This functionality should not exist in snapd.go. While yes, today, autoloading of plugins exists in snapd.go, this is not the best location for this functionality nor autoloading of tasks. This type of functionality should be part of the scheduler. Also, this feature should not use the same directory path as plugins for also hosting tasks. There should be a default, such as /etc/snap/tasks, or /opt/snap/tasks since we may be looking to install plugins from packages to /opt/snap/plugins. A user should be able to override this path through a command line flag or config setting in scheduler. |
|
For your 1st comment I can not argue because I have very little background on snap so far so I assume that you are perfectly right for this matter. I will try to figure this out For the second part, I kind of disagree because the auto_discovery_path parameter/env variable is a colon-separated list of directories so it is up to the final user to properly organize plugins on one side and tasks on another one: -a plugin-dir:task-dir Furthermore, even if everything is mixed up in a single repository I have taken good care of making sure that plugins are loaded first and tasks once plugins are done so as to avoid most errors This might therefore be much more like a documentation and usage issue than a real problem from my perspective. |
|
Can one of the admins verify this patch? |
|
Does this PR depend on the commit from #961? |
|
@IRCody : Yes it does |
|
Can one of the admins verify this patch? |
|
Please have a look in scheduler/scheduler.go at TODO comment These were (re)moved from rest part to be exposed at higher level so as to factorize code between rest submitted tasks and the ones which may now exists in auto_discovery path |
|
Sorry for the very basic CI failure in previous submission, was too quick in submitting modified code after removing my debug messages :-( Please note also that I will be trying to write tests very soon for the new code locations and features |
| }).Error("Parsing Yaml file ", err) | ||
| continue | ||
| } | ||
| tfile, err := ioutil.TempFile(os.TempDir(), "yaml2json") |
There was a problem hiding this comment.
Using a temp file seems unnecessary. iirc you can call yaml.Unmarshall() here and the library will handle it appropriately if it is either json or yaml (I think how the library works internally is by always converting yaml to json and then doing the marshal).
I wrote a short test program and the library does seem to work that way unless there are any gotcha's I don't see.
There was a problem hiding this comment.
@IRCody I do not understand fully what you are suggesting here. CreateTaskFromContent is using json library to decode task informations, and if I pass a Yaml file directly to it I get
ERRO[0001] invalid character '-' in numeric literal _block=autoDiscoverTasks _module=scheduler autodiscoverpath=/home/olivier/.gvm/pkgsets/go1.5.4/global/src/github.com/intelsdi-x/snap/build/plugin task=mock-file.yaml
So I do not see how to get around this by not having a temp file and keeping the code as common as possible to what it was (not replacing json by yaml in CreateTaskFromContent).
Thanks for your inputs
There was a problem hiding this comment.
@obourdon I was planning to look at this some more today and I will take another look at this section to wrap my head around it. Thanks for pointing out what I was missing.
|
Tested this and didn't notice any issues other than the above comments. Due to some of the code movements I'd be curious to get feedback from some others. cc: @intelsdi-x/snap-maintainers. |
|
@intelsdi-x/snap-maintainers: any chance some of you had some time to review this change ? |
| @@ -0,0 +1,197 @@ | |||
| /* | |||
There was a problem hiding this comment.
Not clear why this functionality was moved under /pkg. Any code in /pkg cannot have dependencies on anything in the snap framework.
There was a problem hiding this comment.
As I stated in
#951 (comment)
I did not manage to figure out where to properly relocate this code
Therefore I took'this wild guess and was expecting to get advices on where to put it so as to comply with snap/golang code best practices
There was a problem hiding this comment.
I added a commit here that removes the task pkg and moves the logic to core/task.go and core/schedule.go just to look at what a possible alternative looks like.
There was a problem hiding this comment.
Seems perfect to me. @jcooklin Thanks for this work Joel
Is there a way we can substitute your PR instead of mine for this to get integrated ?
There was a problem hiding this comment.
@obourdon you can checkout my branch and cherry-pick the commit to your PR.
On Mon, Jun 13, 2016 at 8:46 AM Olivier Bourdon notifications@github.com
wrote:
In pkg/task/task.go
#951 (comment):@@ -0,0 +1,197 @@
+/*Seems perfect to me. @jcooklin https://github.com/jcooklin Thanks for
this work Joel
Is there a way we can substitute your PR instead of mine for this to get
integrated ?—
You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub
https://github.com/intelsdi-x/snap/pull/951/files/531bdadfe945a51749dbce9976ea7fa1273ea3ea#r66816318,
or mute the thread
https://github.com/notifications/unsubscribe/AA0q-J6U42k_7D2487MngUgqWbvllK3Aks5qLXtngaJpZM4Im5Oj
.
|
@obourdon: What is the state of this? Do you need some help resolving the conflicts? I thought yesterday I saw a build issue with Controlproxy not implementing GetAutodiscoverPaths(since this PR adds that to the interface). I can help with that if that is the blocking issue. |
|
@IRCody : I had a very brief look at the conflicts which seem to be resulting from the GRPC code integration and I must confess that I was not really confident in what needed to be done. (Note however that solving these conflicts is easy whereas having the resulting code to compile and work properly was way more obscure) Furthermore as I noticed you wrote most of the new code I guess it would be much faster for you to help me figure out what needs to be changed. Thanks for stepping forward. |
|
I will look at it today @obourdon. |
|
@IRCody thanks a lot |
|
@obourdon: took a quick stab at it this morning. You can see the work on my branch. I went ahead and rebased on master also. I tested the most basic case of passing both a plugin and task directory to snapd and plugins were loaded/tasks were started. I would appreciate your help in testing other cases since I'm sure you are more familiar with where the edges are. If you want you can cherry-pick that commit also and add it to this PR or if you prefer I can submit it as a separate PR that will block this one until it is merged. Let me know what you decide and about any issues you encounter. Thanks! |
|
@IRCody thanks a lot for all the work. Will have a complete look and go though deep testing tomorrow. |
Adds GetAutodiscoverPaths() to controlproxy to fulfil the managesMetrics interface. Adjust Godeps to correct grpc version.
|
Is it possible to retrigger CI please ? |
|
I think it should've re-triggered when you pushed the latest commit. I'll look into it. |
|
Can one of the admins verify this patch? |
|
Closed/re-opened to trigger a new build. |
|
@obourdon: Could you update the godeps entry for protobuf to the commit below? I think that will fix the CI build issues. |
Updating protobuf revisionId
|
LGTM. @intelsdi-x/snap-maintainers: Does anyone else have thoughts before merging? |
|
LGTM |
Fixes #950
Summary of changes:
Testing done:
different tasks successfully at startup
@intelsdi-x/snap-maintainers