Implementing a reload() function (or foo_reload() ?) is of course the correct choice.
Out of the two variable based alternatives, I find foo_reload=1 as somewhat understandable. We originally set out to find the easiest way to implement something, so yes, it is the hackish way. henrik On Sun, Apr 29, 2012 at 5:34 PM, Daniel Nichter <dan...@percona.com> wrote: > Henrik, > > Do you think that foo_reload=1 is redundant because SET GLOBAL foo=@@foo will > always work in any case? > > I think variables like foo_reload are more an abuse of a side effect than > foo=@@foo because foo_reload uses a variable as a toggle switch (vs. just a > simple on/off switch like the _active variables that I have proposed). I > suppose this is ok if we document its semantics, which might be: > > * foo_reload = 0 when the toggle switch is inactive (i.e. foo is not being > reloaded) > * foo_reload = 1 while foo is being reloaded (due to SET GLOBAL foo_reload=1) > * foo_reload returns to 0 once foo has reloaded, whether successfully or not > > Still this seems hackish to me. Perhaps we should do the effort of > implementing a RELOAD function? This might be a better longterm solution > because then we provide reload ability at a higher level so plugin writers > don't have to implement special _reload vars. > > -Daniel > > Le 29 avr. 2012 à 01:51, Henrik Ingo a écrit : > >> Daniel, >> >> I don't like the end user semantics of what you are proposing. It >> looks like you are abusing a side effect of something else. >> >> "something_reload=1" is quite clear what it means. Note that for an >> average mysql/drizzle user even the meaning of the @@var_name syntax >> might be unclear. >> >> henrik >> >> >> On Sat, Apr 28, 2012 at 8:56 AM, Daniel Nichter <dan...@percona.com> wrote: >>> Anshu, >>> >>> My reply is quite late, but the conference and Drizzle Day were both >>> enlightening. I look forward to next year. >>> >>> As for updating files, I think there's a simpler, more direct approach: >>> >>> SET GLOBAL regex_policy_policy=@@regex_policy_policy; >>> >>> That sets the var to its current value which triggers the plugin's update >>> hook and since the new value == the old value, the result is simply to >>> reload the file. >>> >>> This avoid having two code paths: one for SET >>> GLOBAL regex_policy_policy="new-file" and another for SET >>> GLOBAL regex_policy_reload=1 (which would eventually call the same code as >>> the first statement). >>> >>> Thoughts on this (you and anyone else)? >>> >>> -Daniel >>> >>> Le 17 avr. 2012 à 15:09, Anshu Kumar a écrit : >>> >>> Hi Daniel, >>> How was the conference and drizzle day? >>> Coming to the point, after brainstorming the possibilities that I have in >>> the issue of making regex policy plugin dynamic, I think that the solution I >>> proposed in my last mail is best for now. And using that we wont need any >>> issue handling with if the policy file was not correct or change in the look >>> and feel of some specific plugins. From coding point of view, we can do this >>> by adding an option of reload variable which will default be set to false. >>> Upon making it true, it will trigger a function which will create a separate >>> instance of policy and will call loadfile on it. If the call is a success, >>> we will remove all old policies and reload the system with new ones. And if >>> the call gives some error message then do nothing. After both the cases, the >>> function will change the reload variable to false. This way we wont need any >>> separate threads also. >>> Waiting for your review comments to start working on this. >>> >>> On Wed, Apr 4, 2012 at 2:55 PM, Anshu Kumar <ansharyan...@gmail.com> wrote: >>>> >>>> Hey Daniel, >>>> After reading your views, I too think now that auto reloading is not the >>>> ideal solution to this scenario. In this solution I have used the idea of >>>> thread for reloading the policy files. You put a point that what if admin >>>> is >>>> using a ext editor which auto saves the changes after some fixed time. In >>>> that case incorrect policies may be read. >>>> >>>> For this issue, instead of using a thread, we can come up with a solution >>>> in which the reloading won't be dynamic. So it would be like, if you are an >>>> admin and you want to change the policy file, do it. After all changes are >>>> done, just change the value of regex_policy_reload to true and policies >>>> will >>>> then be reloaded. After policies are reloaded, the value of >>>> regex_policy_reload will again be set to false from code itself. In this >>>> way, it can be ensured that only the correct policies are loaded there. It >>>> is true that here too anyone can change the status of regex_policy_reload >>>> to >>>> true, so either don't let him access the policy file or we will be needing >>>> an authorization system for changing global variables too. >>>> >>>> Brainstorming further for the most optimal solution. Comment and >>>> suggestions are welcome. >>>> >>>> About the name concern, I use Ansh as my nick. Both are correct. You can >>>> call me anything you like. However changing my sign to 'Anshu', so that it >>>> don't cause any further confusion to anyone. :) >>>> >>>> On Wed, Apr 4, 2012 at 1:40 PM, Henrik Ingo <henrik.i...@avoinelama.fi> >>>> wrote: >>>>> >>>>> Daniel >>>>> >>>>> FWIW, I think you might be correct. I originally thought that >>>>> auto-reloading would be an easy way to implement a way to re-read the >>>>> policy >>>>> file. For instance, it could be implemented crudely without any new >>>>> configuration options and no new threads needing to launch. However, as >>>>> we've gone deeper into the subject it became clear that a good >>>>> implementation will need those things, hence the "easy" attribute is no >>>>> longer there. I support going back to explore your original idea. >>>>> >>>>> henrik >>>>> >>>>> >>>>> On Wed, Apr 4, 2012 at 8:12 AM, Daniel Nichter <dan...@percona.com> >>>>> wrote: >>>>>> >>>>>> Anshu, Henrik, Clint, >>>>>> >>>>>> First: Anshu: I like to use people's names correctly, so I noticed you >>>>>> sign your emails "Ansh" but we've been writing "Anshu". Is one or the >>>>>> other, or both, correct? >>>>>> >>>>>> Now on to business. First, thanks for the code. This is a good start >>>>>> and I'm happy to see that you've been able to jump into the code with >>>>>> apparent ease. >>>>>> >>>>>> Second, I disagree that the auto-reloading approach is the ideal >>>>>> solution. I think we all need to debate the merits of this approach >>>>>> further. So let's do that... >>>>>> >>>>>> In my humble opinion, this approach has the following drawbacks: >>>>>> >>>>>> 1) It's "gold plating" because in the real world the policy file >>>>>> won't/shouldn't change so frequently as to make autp-reloading a valuable >>>>>> feature. It's not too much to ask the user to execute one simple >>>>>> command, >>>>>> and chances are they will expect to do this. Thinking of Drizzle and >>>>>> MySQL, >>>>>> no auto-(re)loading features come immediately to my mind, so we won't be >>>>>> depriving the user of functionality they're used to. Of course, I'm all >>>>>> for >>>>>> new types of functionality when it's clear that the functionality will >>>>>> be a >>>>>> "big win" for users, but given the nature of this issue, I don't think >>>>>> auto-reloading is a big win and certainly not worth the extra engineering >>>>>> effort. >>>>>> >>>>>> 2) It's a bit of magic and magic usually leads to problems down the >>>>>> road. Sure, it's just one little thread that sleeps, checks, and maybe >>>>>> does >>>>>> something, but one immediate "problem" comes to mind: testing. Anything >>>>>> dependent on time or time-based is inherently more difficult to test. >>>>>> It's >>>>>> possible, of course, but (related to #1) is it worth the effort? Another >>>>>> problem comes to mind: what if the admin is editing the policy file and >>>>>> his >>>>>> editor auto-saves every N minutes, and auto-reload is on? Drizzle may >>>>>> read >>>>>> the policy file before the admin intends. Magic can be very difficult to >>>>>> wield safely. >>>>>> >>>>>> 3) It doesn't address the original security concern. Henrik raised the >>>>>> problem that anyone could execute the command to reload the policy file, >>>>>> but >>>>>> anyone could enable the auto-reload, too. The real solution lies in the >>>>>> authorization modules. Right now Drizzle has only very basic >>>>>> authorization: >>>>>> schema and table access (via this plugin nonetheless). What Drizzle >>>>>> needs >>>>>> is authorization for setting/changing global variables. I think we can >>>>>> make >>>>>> still make this dynamic and leave the authorization solution for another >>>>>> time/person/project. >>>>>> >>>>>> 4) It introduces a concept that will either become a norm or become an >>>>>> exception. Given the aforementioned points, I wouldn't like to see >>>>>> auto-reloading before a norm, and we certainly don't want more exceptions >>>>>> because one of our high-level goals is to make the "look and feel" of all >>>>>> plugins consistent. So if we do this here, then why not >>>>>> --auth-file.auto-reload too? But then we're neck deep in gold plating >>>>>> and >>>>>> magic. >>>>>> >>>>>> 5) It seems contrary to "the Drizzle way". Brian, Stewart, and the >>>>>> original core developers have more of a sense of what the Drizzle way is, >>>>>> but I'm pretty certain is centers on removing the extraneous and the >>>>>> gotchas. Drizzle prides itself it what it doesn't have: no views, no >>>>>> triggers, no timezones, etc. I think auto-reloading is extraneous (gold >>>>>> plating) and prone to becoming a gotcha. To modify my previous example: >>>>>> what if the senior admin enables auto-reloading but forgets to tell the >>>>>> junior admin who changes the policy file on Friday and plans to review >>>>>> the >>>>>> changes with the senior admin on Monday only for both of them to get a >>>>>> call >>>>>> during the weekend by the CTO because Sally in HR can't access the >>>>>> employees >>>>>> database and she's trying to do payroll so now everyone's paychecks will >>>>>> be >>>>>> late? Gotcha. >>>>>> >>>>>> So, in conclusion: your code is a good start, but I think we need >>>>>> another solution. Other opinions? >>>>>> >>>>>> -Daniel >>>>>> >>>>>> >>>>>> Le 3 avr. 2012 à 18:03, Anshu Kumar a écrit : >>>>>> >>>>>> Hi Daniel, >>>>>> For making all the plugins dynamic, as the earlier discussion for >>>>>> auth_file could not come to a conclusion, I started off with making the >>>>>> regex_policy plugin dynamic. Here is the complete process which am >>>>>> proposing >>>>>> for this plugin >>>>>> >>>>>> 1. There would be a autoreload variable, default set to false. This >>>>>> variable will determine if policy file needs to be reloaded. >>>>>> >>>>>> 2. Upon setting this variable to true (SET GLOBAL >>>>>> regex_policy_autoreload=ON), a new thread will be created which will >>>>>> handle >>>>>> the reload issues of policy file. >>>>>> >>>>>> 3. In the thread created, on every one minute, using the stat command to >>>>>> find the last modified time and taking the difference from current time, >>>>>> it >>>>>> will be checked that if the file was modified in last one minute. And if >>>>>> it >>>>>> was, reload the file. If it wasn't modified, continue to the next poll. >>>>>> >>>>>> 4. This thing will go on till you again set the variable value to false >>>>>> (SET GLOBAL regex_policy_autoreload=OFF) >>>>>> >>>>>> Scenario would be, if you want to change the policy file and want those >>>>>> changes to be reflected, just change the variable to true, do your >>>>>> modifications, and the changes will be reflected the next minute in the >>>>>> system. >>>>>> >>>>>> I have implemented this whole thing. Here is the link of the branch. It >>>>>> would be great if you can go through it and give your comments. >>>>>> https://code.launchpad.net/~ansharyan015/drizzle/dynamic_regex_policy >>>>>> >>>>>> P.S. Thanks to Henrik for being the continuous help. :) >>>>>> Comments and suggestions are welcome. >>>>>> >>>>>> >>>>>> On Mon, Apr 2, 2012 at 11:01 PM, Henrik Ingo <henrik.i...@avoinelama.fi> >>>>>> wrote: >>>>>>> >>>>>>> Anshu: To create a background thread, you need to use class >>>>>>> drizzled::plugin::Daemon >>>>>>> >>>>>>> Look at plugin/json_server/json_server.cc (class JsonServer at the >>>>>>> end) for an example. >>>>>>> >>>>>>> henrik >>>>>>> >>>>>>> On Mon, Apr 2, 2012 at 4:16 PM, Henrik Ingo <henrik.i...@avoinelama.fi> >>>>>>> wrote: >>>>>>>> Anshu >>>>>>>> >>>>>>>> You are right. Creating a thread is more correct. I was just trying >>>>>>>> to >>>>>>>> avoid it since you originally asked for "low-hanging-fruit" bug. >>>>>>>> >>>>>>>> Functionally, ignoring the performance hit, you could do this also >>>>>>>> from the restrict* methods and then you don't need to create your own >>>>>>>> thread. >>>>>>>> >>>>>>>> henrik >>>>>>>> >>>>>>>> On Mon, Apr 2, 2012 at 3:45 PM, Anshu Kumar <ansharyan...@gmail.com> >>>>>>>> wrote: >>>>>>>>> Hey Henrik, >>>>>>>>> What I was thinking is we can have a system like this. When >>>>>>>>> autoreload >>>>>>>>> variable is set to true, we can create a thread which will check >>>>>>>>> either by >>>>>>>>> inotify or stat() that the corresponding regex policy file is >>>>>>>>> changed or >>>>>>>>> not. And it of does then only reload the file. This is just like >>>>>>>>> creating a >>>>>>>>> handler for change in policy file. The performance will be better >>>>>>>>> than >>>>>>>>> polling for a min and reloading the policy file. >>>>>>>>> The implementation is actually the same in this, and the way you >>>>>>>>> suggested >>>>>>>>> by Policy::restrict methods. So did you want to say that when >>>>>>>>> checking for >>>>>>>>> regex policy rights (if access is allowed or denied), we can check >>>>>>>>> if file >>>>>>>>> needs to be refreshed? >>>>>>>>> >>>>>>>>> On Mon, Apr 2, 2012 at 6:02 PM, Henrik Ingo >>>>>>>>> <henrik.i...@avoinelama.fi> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Anshu >>>>>>>>>> >>>>>>>>>> Actually, now that I re-read this, I don't know if it was a smart >>>>>>>>>> idea >>>>>>>>>> and maybe there needs to be a thread that does the polling, but my >>>>>>>>>> original idea was that you could reload the file during an >>>>>>>>>> authorization request. So basically when any of the >>>>>>>>>> Polixy::restrict... methods are called, you would first check if >>>>>>>>>> the >>>>>>>>>> file needs to be refreshed and then re-read it (or not). This way >>>>>>>>>> you >>>>>>>>>> don't need to have a loop or do any polling or such. >>>>>>>>>> >>>>>>>>>> This approach is not good because it would introduce a performance >>>>>>>>>> hit >>>>>>>>>> into random requests every N seconds. But you could still do it >>>>>>>>>> this >>>>>>>>>> way as a proof of concept, then we could work on making it a >>>>>>>>>> background thread later. >>>>>>>>>> >>>>>>>>>> henrik >>>>>>>>>> >>>>>>>>>> On Mon, Apr 2, 2012 at 2:51 PM, Anshu Kumar >>>>>>>>>> <ansharyan...@gmail.com> >>>>>>>>>> wrote: >>>>>>>>>>> Hey Guys, >>>>>>>>>>> I have tried to implement this dynamic thing to regex_policy >>>>>>>>>>> plugin. >>>>>>>>>>> In reference to my talk with Henrik yesterday, we discussed that >>>>>>>>>>> there >>>>>>>>>>> could >>>>>>>>>>> be a autoreload variable, and when made true it will continuously >>>>>>>>>>> poll >>>>>>>>>>> for >>>>>>>>>>> changes in default regex policy file. And it it is made false, it >>>>>>>>>>> will >>>>>>>>>>> stop >>>>>>>>>>> polling, checking for modifications. Sticking to the discussion, >>>>>>>>>>> the >>>>>>>>>>> code I >>>>>>>>>>> wrote is >>>>>>>>>>> >>>>>>>>>>> void autoReload_Regex_Policy(Session *, sql_var_t) >>>>>>>>>>> { >>>>>>>>>>> if(policy->sysvar_autoreload) >>>>>>>>>>> { >>>>>>>>>>> while(1) >>>>>>>>>>> { >>>>>>>>>>> if (not policy->loadFile()) >>>>>>>>>>> { >>>>>>>>>>> errmsg_printf(error::ERROR, _("Could >>>>>>>>>>> not >>>>>>>>>>> load >>>>>>>>>>> regex policy file: %s\n"), >>>>>>>>>>> (policy ? >>>>>>>>>>> policy->getError().str().c_str() : _("Unknown"))); >>>>>>>>>>> return; >>>>>>>>>>> } >>>>>>>>>>> sleep(60); >>>>>>>>>>> if(!policy->sysvar_autoreload) >>>>>>>>>>> break; >>>>>>>>>>> } >>>>>>>>>>> } >>>>>>>>>>> else >>>>>>>>>>> { >>>>>>>>>>> } >>>>>>>>>>> } >>>>>>>>>>> This function handles the case when you change the value of >>>>>>>>>>> autoreload >>>>>>>>>>> from >>>>>>>>>>> drizzle client. The problem is as the function is using sleep >>>>>>>>>>> recursively, >>>>>>>>>>> when trying to change autoreload value by "SET GLOBAL", the >>>>>>>>>>> client >>>>>>>>>>> hangs. >>>>>>>>>>> This is obviously due to function recursive structure. Initially, >>>>>>>>>>> I >>>>>>>>>>> thought >>>>>>>>>>> that it would change the variable and then polls. >>>>>>>>>>> >>>>>>>>>>> Now, coming to the discussion earlier in this mail, even if I >>>>>>>>>>> create a >>>>>>>>>>> pthread from this function which will check for modification >>>>>>>>>>> using >>>>>>>>>>> stat() or >>>>>>>>>>> inotify(), this function won't exit untill its thread stop >>>>>>>>>>> working. And >>>>>>>>>>> as >>>>>>>>>>> thread will continuously polls for changes, it wont exit. >>>>>>>>>>> >>>>>>>>>>> Is there any solution for this scenario, except adding the >>>>>>>>>>> refresh >>>>>>>>>>> command >>>>>>>>>>> for refreshing the policy file? >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Fri, Mar 30, 2012 at 10:34 PM, Clint Byrum <cl...@fewbar.com> >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> Excerpts from Henrik Ingo's message of Thu Mar 29 21:16:26 -0700 >>>>>>>>>>>> 2012: >>>>>>>>>>>>> Daniel: >>>>>>>>>>>>> >>>>>>>>>>>>> Have you thought about authorization for this? I mean we >>>>>>>>>>>>> wouldn't >>>>>>>>>>>>> want >>>>>>>>>>>>> any old logged in user to be able to >>>>>>>>>>>>> >>>>>>>>>>>>> SET GLOBAL >>>>>>>>>>>>> auth_file.users=/home/hingo/igivemyselfrootpowers.users >>>>>>>>>>>>> >>>>>>>>>>>>> (Making the plugin reload the existing file will be helpful. >>>>>>>>>>>>> But it >>>>>>>>>>>>> might not be a good idea to allow to change that value.) >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Agreed. I'd like to see plugins like auth_file and regex_policy >>>>>>>>>>>> given >>>>>>>>>>>> a generic way to "watch" their files. There are a number of ways >>>>>>>>>>>> to do >>>>>>>>>>>> this, but I don't think each plugin should implement its own >>>>>>>>>>>> method. >>>>>>>>>>>> >>>>>>>>>>>> Thoughts I've had on this: >>>>>>>>>>>> >>>>>>>>>>>> * A thread which uses either inotify or falls back to polling >>>>>>>>>>>> with stat(), and whenever there is a change, calls any >>>>>>>>>>>> registered code >>>>>>>>>>>> to update that file's effect. >>>>>>>>>>>> >>>>>>>>>>>> * An admin command like REFRESH '/etc/drizzle/regex.policy' >>>>>>>>>>>> which does >>>>>>>>>>>> the same thing as the thread without the inotify/polling. >>>>>>>>>>>> >>>>>>>>>>>> * Cache the stat() call on the file and periodically expire the >>>>>>>>>>>> cache >>>>>>>>>>>> and refresh the contents if stat() indicates that it has >>>>>>>>>>>> changed. >>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> henrik.i...@avoinelama.fi >>>>> +358-40-8211286 skype: henrik.ingo irc: hingo >>>>> www.openlife.cc >>>>> >>>>> My LinkedIn profile: http://www.linkedin.com/profile/view?id=9522559 >>>> >>>> >>>> >>>> >>>> -- >>>> Regards, >>>> Anshu >>>> >>> >>> >>> >>> -- >>> Regards, >>> Anshu >>> >>> >> >> >> >> -- >> henrik.i...@avoinelama.fi >> +358-40-8211286 skype: henrik.ingo irc: hingo >> www.openlife.cc >> >> My LinkedIn profile: http://www.linkedin.com/profile/view?id=9522559 > -- henrik.i...@avoinelama.fi +358-40-8211286 skype: henrik.ingo irc: hingo www.openlife.cc My LinkedIn profile: http://www.linkedin.com/profile/view?id=9522559 _______________________________________________ Mailing list: https://launchpad.net/~drizzle-discuss Post to : drizzle-discuss@lists.launchpad.net Unsubscribe : https://launchpad.net/~drizzle-discuss More help : https://help.launchpad.net/ListHelp
