[Off-topic] Python pattern matching proposal accepted!
Basically: https://lwn.net/Articles/845480/. Examples/tutorial in https://www.python.org/dev/peps/pep-0636/. Don't know in which Python version will land, but it looks to improve code safety quite a bit (in some cases). Then it needs to land in Debian Stable, etc. etc. :) iustin -- You received this message because you are subscribed to the Google Groups "ganeti-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to ganeti-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/ganeti-devel/YCMOu%2B1WiFd3qbDG%40teal.hq.k1024.org.
Re: Forward-porting vs back-porting fixes
On 2020-12-30 11:50:10, Apollon Oikonomopoulos wrote: > Hi all, > > Now that we've officially released 3.0, it's time to look a bit at > stable release management. What I'd like to discuss in particular is the > possibility of switching from a forward-porting workflow for stable > updates to a back-porting one. > > Ganeti traditionally relied on a forward-porting, merging workflow for > the stable branches: a bugfix would always target the *earliest* stable > branch it should be applied on, and stable branches would subsequently > be merged upwards and finally into master¹. So, to fix a bug that was > introduced for instance in 2.15, we would commit the fix on stable-2.15, > and then successively merge stable-2.15 into stable-2.16, stable-2.16 > into stable-3.0 and stable-3.0 into master. Minor correction: Ganeti originally used back-porting, then switched to forward-porting during the 2.1 release or so. It is not technically relevant, but since we're discussing history anyway… Yes, back-porting was very painful during the 1.2/2.0 era. Forward-porting was not chosen arbitrarily, but rather to solve a real problem (that resulted in many production bugs). [snip rest] > I think we could try a back-porting strategy for the 3.0 stable release > cycle, in the hope that it allows for faster development on master and > simpler PR workflows. If we end up not liking it, we can always revert > to the forward-porting strategy without a problem. What do you think? I think back-porting is more practical, but if we do it, we should ensure that the original commit ID is recorded (`git cherry-pick -x`), so that at least we have a weak trail of record. regards, iustin -- You received this message because you are subscribed to the Google Groups "ganeti-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to ganeti-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/ganeti-devel/X/BYvDVyN/j/9aCa%40teal.hq.k1024.org.
Re: resurrecting 3.0 release
On 2020-12-01 18:30:45, Apollon Oikonomopoulos wrote: > Hi Sascha, > > Apologies for not responding earlier, end-November is always terribly > busy. Same here. Especially as Debian freeze approaches… > Sascha Lucas writes: > > > Hi, > > > > after some weeks of silence I like to ask if someone else is willing to > > finalize the 3.0 Release? I don't know if someone else has open PRs, > > but I like to get #1529 in. So please review. > > > > Any opinions on release date and doings etc. are welcome. > > I think we are mostly good to go. I'll try to go through #1529 tomorrow > and aim for a release on Sunday. Thanks, I didn't manage to get to it (or any Ganeti work). If done in the next weeks, then we can still get 3.0 final in next stable :) -- You received this message because you are subscribed to the Google Groups "ganeti-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to ganeti-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/ganeti-devel/X8Z6PSgECqrJN827%40teal.hq.k1024.org.
Registered for github security scan beta
Just FYI, as I enabled one of my own repositories for the security scan beta, I realised I can enable ganeti too, so I went ahead and did that for the entire org. It's s closed beta, so we're in the queue only, not yet enabled, we'll see how it goes. iustin -- You received this message because you are subscribed to the Google Groups "ganeti-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to ganeti-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/ganeti-devel/20200721070731.GC3776616%40teal.hq.k1024.org.
Re: GHC versions for 3.0?
On 2020-07-08 15:21:32, Ganeti Development List wrote: > Hi, > > i just ask myself, if we try to cover GHC 8.8 do we still have an > upgrade path and can build ganeti 3.0 for ubuntu 18.04? Ah, good point. 18.04/bionic (which is not that old) ships with GHC 8.0.1, which means we should keep 8.0 compatibility. It's doable, but not pleasant :) I'll reply on bug too. iustin > Am Mi., 8. Juli 2020 um 10:19 Uhr schrieb Iustin Pop : > > > > On 2020-07-08 08:22:22, Rudolph Bott wrote: > > > Hi Iustin, > > > > > > I can not add anything directly to the topic, but it seems Apollon already > > > did or is doing some work in this direction: > > > > > > https://github.com/ganeti/ganeti/issues/1493 > > > https://github.com/ganeti/ganeti/issues/1492 > > > > Ouch, I missed those. > > > > I have Ganeti now working with GHC 8.8, but cleaning up the patches > > depends on what the minimum version will be. > > > > Thanks, I'll reply on those bugs. > > > > iustin > > > > > On Tue, Jul 7, 2020 at 11:20 PM Iustin Pop wrote: > > > > > > > Hi all, > > > > > > > > I made the mistake of attempting to build ganeti 3.0 beta on a sid box, > > > > with GHC 8.8, which causes no end of fun… > > > > > > > > To solve the fun and make this actually built, would be good to settle > > > > which versions of GHC we support (if we didn't already and I missed the > > > > email or simply forgot). > > > > > > > > IMO, anything older than 8.0 will generate huge amounts of pain. Debian > > > > oldstable is on 8.0.1, and stable is on 8.4. I would of course prefer as > > > > new as possible, but what do other people think? > > > > > > > > Fun things: > > > > > > > > * cabal new-style vs. old-style builds > > > > * cabal API removals as we're using unstable APIs > > > > * MonadFail final move in GHC 8.8 > > > > > > > > and I still haven't finished to get it to compile :) > > > > > > > > thanks, > > > > iustin > > > > > > > > -- > > > > You received this message because you are subscribed to the Google > > > > Groups > > > > "ganeti-devel" group. > > > > To unsubscribe from this group and stop receiving emails from it, send > > > > an > > > > email to ganeti-devel+unsubscr...@googlegroups.com. > > > > To view this discussion on the web visit > > > > https://groups.google.com/d/msgid/ganeti-devel/20200707212051.GB1450192%40teal.hq.k1024.org > > > > . > > > > > > > > > > > > > -- > > > Rudolph Bott - b...@sipgate.de > > > Telefon: +49 (0)211-63 55 56-41 > > > Telefax: +49 (0)211-63 55 55-22 > > > > > > sipgate GmbH - Gladbacher Str. 74 - 40219 Düsseldorf > > > HRB Düsseldorf 39841 - Geschäftsführer: Thilo Salmon, Tim Mois > > > Steuernummer: 106/5724/7147, Umsatzsteuer-ID: DE219349391 > > > > > > www.sipgate.de - www.sipgate.co.uk > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "ganeti-devel" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > > email to ganeti-devel+unsubscr...@googlegroups.com. > > > To view this discussion on the web visit > > > https://groups.google.com/d/msgid/ganeti-devel/CAPG4N%3DbqFSmds_bZ-0qWY%2B6uFhSj0dWfR%3Dyvdy9kDRzbzRTnmA%40mail.gmail.com. > > > > -- > > You received this message because you are subscribed to the Google Groups > > "ganeti-devel" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to ganeti-devel+unsubscr...@googlegroups.com. > > To view this discussion on the web visit > > https://groups.google.com/d/msgid/ganeti-devel/20200708081859.GC1450192%40teal.hq.k1024.org. > > -- > You received this message because you are subscribed to the Google Groups > "ganeti-devel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to ganeti-devel+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/ganeti-devel/CAEMVdVx-3CUqMV%2BbkZLiwXEW3V6ZRjUmZ7aRZjwYC6CNPZYYuw%40mail.gmail.com. -- You received this message because you are subscribed to the Google Groups "ganeti-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to ganeti-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/ganeti-devel/20200708132716.GD1450192%40teal.hq.k1024.org.
Re: GHC versions for 3.0?
On 2020-07-08 08:22:22, Rudolph Bott wrote: > Hi Iustin, > > I can not add anything directly to the topic, but it seems Apollon already > did or is doing some work in this direction: > > https://github.com/ganeti/ganeti/issues/1493 > https://github.com/ganeti/ganeti/issues/1492 Ouch, I missed those. I have Ganeti now working with GHC 8.8, but cleaning up the patches depends on what the minimum version will be. Thanks, I'll reply on those bugs. iustin > On Tue, Jul 7, 2020 at 11:20 PM Iustin Pop wrote: > > > Hi all, > > > > I made the mistake of attempting to build ganeti 3.0 beta on a sid box, > > with GHC 8.8, which causes no end of fun… > > > > To solve the fun and make this actually built, would be good to settle > > which versions of GHC we support (if we didn't already and I missed the > > email or simply forgot). > > > > IMO, anything older than 8.0 will generate huge amounts of pain. Debian > > oldstable is on 8.0.1, and stable is on 8.4. I would of course prefer as > > new as possible, but what do other people think? > > > > Fun things: > > > > * cabal new-style vs. old-style builds > > * cabal API removals as we're using unstable APIs > > * MonadFail final move in GHC 8.8 > > > > and I still haven't finished to get it to compile :) > > > > thanks, > > iustin > > > > -- > > You received this message because you are subscribed to the Google Groups > > "ganeti-devel" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to ganeti-devel+unsubscr...@googlegroups.com. > > To view this discussion on the web visit > > https://groups.google.com/d/msgid/ganeti-devel/20200707212051.GB1450192%40teal.hq.k1024.org > > . > > > > > -- > Rudolph Bott - b...@sipgate.de > Telefon: +49 (0)211-63 55 56-41 > Telefax: +49 (0)211-63 55 55-22 > > sipgate GmbH - Gladbacher Str. 74 - 40219 Düsseldorf > HRB Düsseldorf 39841 - Geschäftsführer: Thilo Salmon, Tim Mois > Steuernummer: 106/5724/7147, Umsatzsteuer-ID: DE219349391 > > www.sipgate.de - www.sipgate.co.uk > > -- > You received this message because you are subscribed to the Google Groups > "ganeti-devel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to ganeti-devel+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/ganeti-devel/CAPG4N%3DbqFSmds_bZ-0qWY%2B6uFhSj0dWfR%3Dyvdy9kDRzbzRTnmA%40mail.gmail.com. -- You received this message because you are subscribed to the Google Groups "ganeti-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to ganeti-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/ganeti-devel/20200708081859.GC1450192%40teal.hq.k1024.org.
GHC versions for 3.0?
Hi all, I made the mistake of attempting to build ganeti 3.0 beta on a sid box, with GHC 8.8, which causes no end of fun… To solve the fun and make this actually built, would be good to settle which versions of GHC we support (if we didn't already and I missed the email or simply forgot). IMO, anything older than 8.0 will generate huge amounts of pain. Debian oldstable is on 8.0.1, and stable is on 8.4. I would of course prefer as new as possible, but what do other people think? Fun things: * cabal new-style vs. old-style builds * cabal API removals as we're using unstable APIs * MonadFail final move in GHC 8.8 and I still haven't finished to get it to compile :) thanks, iustin -- You received this message because you are subscribed to the Google Groups "ganeti-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to ganeti-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/ganeti-devel/20200707212051.GB1450192%40teal.hq.k1024.org.
Haskell environment for 2.16.1
Hi Apollon, I just realised that all the fixes from the Debian packaging are on master. This means two thing for the stable-2.16 branch: - building in a recent environment is tricky - the travis-ci builds for it are still failing Any reason why not to update stable-2.16 to be fully buildable? The cherry-picks should be easy, and given how old the dependencies of 2.16.0 are, bumping them up should be much better than leaving as-is. Thoughts? iustin
Re: Continuing development
On 2019-02-12 17:02:35, Apollon Oikonomopoulos wrote: > Immediate steps > --- > > In light of the above, I propose the following: > > - Dismiss all unreleased work and archive the current master and >stable-2.17 branches under attic/ (I've already pushed a ref for >attic/stable-2.17). > > - Declare 2.15 EOL > > - git push --force origin v2.16.0:master and continue development from >there. I don't expect the force-push to be an issue, and it's much >cleaner than a `git merge -s ours' or messing up our versioning. We >*will* be actually throwing away that development history — at least >for the time being. For the record: neither me nor Apollon had rights for a force-push, and after discussion that was not much better than `git merge -s ours`; the later doesn't throw away history. So we did a merge, and now master is basically v2.16.0. Please everyone rebase and resent your pull requests. Many thanks Apollon for moving things forward! iustin signature.asc Description: PGP signature
Re: Continuing development
On 2019-02-13 00:56:49, Apollon Oikonomopoulos wrote: > On 18:38 Tue 12 Feb , Iustin Pop wrote: > > On 2019-02-12 17:02:35, Apollon Oikonomopoulos wrote: > > > Hi everyone, > > > > One specific question below: > > > > > - git push --force origin v2.16.0:master and continue development from > > >there. I don't expect the force-push to be an issue, and it's much > > >cleaner than a `git merge -s ours' or messing up our versioning. We > > >*will* be actually throwing away that development history — at least > > >for the time being. > > > > There are a number of github pull requests against master. They might > > need manual rebase or even re-creation. > > They can be rebased, and those that were already merged can be > cherry-picked to the new master branch. Some of them can even be merged > to stable-2.16 for 2.16.1, since they're addressing long-standing bugs. > > > > > Have you thought of actually just taking master as is, and relabelling > > it as 2.17~pre0 or such? Do you know what's the incomplete work on it? > > The problem with master is that it also contains all stable-2.17 > unreleased work as well. I see. > To get an idea: > > Commits on stable-2.17 that were not in v2.16.0 > > $ git shortlog -sn --no-merges v2.16.0..origin/stable-2.17 >126 Klaus Aehlig > 30 Helga Velroyen > 28 Oleg Ponomarev > 17 Bhimanavajjula Aditya > 10 BSRK Aditya > 8 Lisa Velden > 3 Brian Foley > 3 Federico Morg Pareschi > 3 Viktor Bachraty > 1 Daniil Leshchev > 1 Dimitris Bliablias > > From stable-2.17, there's additionally the following commits to reach > master: > > $ git shortlog -sn --no-merges origin/stable-2.17..origin/master > 27 Brian Foley > 21 Oleg Ponomarev > 17 Klaus Aehlig > 14 Iustin Pop > 8 Thomas Vander Stichele > 7 Daniil Leshchev > 7 Federico Morg Pareschi > 7 Yiannis Tsiouris > 5 Calum Calder > 4 Bhimanavajjula Aditya > 4 Hrvoje Ribicic > 4 Lisa Velden > 3 Viktor Bachraty > 2 David Mohr > 1 Alexandros Kosiaris > 1 Arnd Hannemann > 1 Brian Candler > 1 Dimitris Aragiorgis > 1 Emily Bragg > 1 Federico 'Morg' Pareschi > 1 Igor Vuk > 1 Jun Futagawa > 1 Nicolas Avrutin > 1 Nikos Skalkotos > 1 Phil Regnauld > > > The delta from stable-2.17 to master is small, but from 2.16.0 to master > it includes all the stable-2.17 changes. Indeed, that's a large body of untested (as in unreleased, and not run in production) code. > I think it's best to just > cherry-pick individual commits of interest directly from master. I'll be a bit busy then, regarding the few fixes I have in master :) thanks, iustin
Re: Continuing development
On 2019-02-12 17:02:35, Apollon Oikonomopoulos wrote: > Hi everyone, One specific question below: > - git push --force origin v2.16.0:master and continue development from >there. I don't expect the force-push to be an issue, and it's much >cleaner than a `git merge -s ours' or messing up our versioning. We >*will* be actually throwing away that development history — at least >for the time being. There are a number of github pull requests against master. They might need manual rebase or even re-creation. Have you thought of actually just taking master as is, and relabelling it as 2.17~pre0 or such? Do you know what's the incomplete work on it? iustin
Re: Continuing development
And now correcting the -devel mailing list address. On 2019-02-12 18:26:04, Iustin Pop wrote: > On 2019-02-12 17:02:35, Apollon Oikonomopoulos wrote: > > Hi everyone, > > Hi Apollon, > > > With 2.16.0 released last fall, I think it's time to discuss how we can > > kick the project back to life. I don't want to tire you with too many > > details, so I'll try to make it short. > > > > Picking a starting point > > > > > > As discussed in the last GanetiCon, continuing development - especially > > in face of the lack of resources - requires some hard decisions, > > namely should we scrap all unreleased work on the master and stable-2.17 > > branches (aside from bugfixes) and continue development from 2.16.0? > > > > Personally I'd answer "yes" here: I think it is vital to focus on > > modernizing the code first, and spending energy to stabilize unreleased > > work acts against this. We can still save important features (e.g. > > maintd/harepd) and try to merge them at some point in the future once > > we're running smoothly again. > > +1. > > > Simplifying the workflow > > > > > > Given the currently available resources, I think we need to simplify the > > workflow to the extent possible. It doesn't make sense to support 5 > > different > > release branches, nor do we have the energy to develop 2 future releases > > and a > > master branch simultaneously. > > I can only speak to my time in Ganeti, but not even with a full team it > didn't make sense to support 5 release braches, nor 2 future releases. > > > I feel we need a simpler approach, with all development happening on > > master and maintenance branches being created for each minor version > > released from master. As always, maintenance branches will receive only > > bug and compatibility fixes, which will be upmerged. On the course we'll > > also get to decide how many stable releases we are going to be > > supporting. > > > > Immediate steps > > --- > > > > In light of the above, I propose the following: > > > > - Dismiss all unreleased work and archive the current master and > >stable-2.17 branches under attic/ (I've already pushed a ref for > >attic/stable-2.17). > > SGTM. > > > - Declare 2.15 EOL > > What does it mean EOL here? Current Debian stable still has 2.15 (+2.16 > in backports), so does this mean dropping all support? > > > - git push --force origin v2.16.0:master and continue development from > >there. I don't expect the force-push to be an issue, and it's much > >cleaner than a `git merge -s ours' or messing up our versioning. We > >*will* be actually throwing away that development history — at least > >for the time being. > > How would it break versioning? Not sure I understand. > > > - Work on stable-2.16 to release 2.16.1 with all bugfixes currently > >available in PRs or elsewhere (e.g. Debian package patches) > > +1. > > > - Work on master to prepare 2.17.0, with the following goals: > >+ Single-source compatibility for all GHC versions >= 8 (or even 8.2) > >+ Have a working CI setup with all tests passing > >+ More TBD; we can set a Github milestone for that > > +1. > > > The goal here is to release early, and release often™. > > > > From there on we can start discussing about bigger things, like > > switching to Python 3 and modernizing our hypervisor support. > > I would be definitely interested in the Python 3 move, but honestly on a > theortical level - how feasible is to move a large codebase. > > > Comments, thoughts or anything else welcome :) I think this is also the > > best time for a show of hands from people willing to participate in any > > way (patches, code reviews, testing, documentation etc). > > I'm happy to review code/designs, but with high latency (usually on the > weekend). > > regards, > iustin
Re: Status of migrated issues?
On 2018-02-04 14:18:51, Ganeti Development List wrote: > As an FYI all, I have closed all Github issues that had the label > Status:Fixed. Awesome, thanks Morg! iustin
Re: Status of migrated issues?
On 29 January 2018 at 11:08, 'Federico Pareschi' via ganeti-devel <
ganeti-devel@googlegroups.com> wrote:
> Sorry, this email had completely flown under my radar.
>
No worries.
The issues have all been migrated to the github repository, however there
> is a difference between 'fixed' and 'released' in the old issue tracker on
> code.google.com. By importing these issues in github, these states turned
> into tags. I assume 'released' meant the issue could be closed, but 'fixed'
> simply added the tag but the issue stayed open.
>
Sorry, not sure I understand here. Do I read that code.google.com had two
"fixed" state - one fix committed ("fixed"), and one fix committed and also
released ("released")? If so, and if Github only has fixed, I propose to
drop the released state.
I had been thinking about going through all of them and closing the ones
> marked as fixed but I wanted to review them first and is a lot of
> time/effort that I haven't invested yet (verifying the fix + closing the
> issue rather than just automatically closing all of them).
>
I would suggest to simply fix all of them, based on:
- we should trust the code.google.com "fixed" tag;
- there are a lot of very old issues that are for sure fixed, and verifying
all of them is time better spent on fixing new issues
If some issue is still open, and if users still hit it, let's crowdsource
the effort of reopening (against current versions) to the users :)
iustin
On Fri, 26 Jan 2018 at 19:53, Iustin Pop wrote:
>
>> On 2018-01-14 02:15:41, Iustin Pop wrote:
>> > Hi all,
>> >
>> > I did a quick look at the GitHub issue tracker today, and it seems that
>> > the migrator did not close the migrated issues that were closed.
>> >
>> > More specifically, a search of:
>> >
>> > is:issue is:open label:Status:Fixed
>> >
>> > Shows 98 such open issues. If I'm not mistaken, "Status:Fixed" is a
>> > googlecode imported label, which tells me that the migrator had some
>> > issue (ha) importing the bug list.
>>
>> Friendly ping? Mostly for my own curiosity.
>>
>> iustin
>>
>
Re: Status of migrated issues?
On 2018-01-14 02:15:41, Iustin Pop wrote: > Hi all, > > I did a quick look at the GitHub issue tracker today, and it seems that > the migrator did not close the migrated issues that were closed. > > More specifically, a search of: > > is:issue is:open label:Status:Fixed > > Shows 98 such open issues. If I'm not mistaken, "Status:Fixed" is a > googlecode imported label, which tells me that the migrator had some > issue (ha) importing the bug list. Friendly ping? Mostly for my own curiosity. iustin
Status of migrated issues?
Hi all, I did a quick look at the GitHub issue tracker today, and it seems that the migrator did not close the migrated issues that were closed. More specifically, a search of: is:issue is:open label:Status:Fixed Shows 98 such open issues. If I'm not mistaken, "Status:Fixed" is a googlecode imported label, which tells me that the migrator had some issue (ha) importing the bug list. Am I mistaken? thanks! iustin
Re: [RFH] Building with GHC 8 and snap-server 1.0
On 2016-12-10 21:42:12, Apollon Oikonomopoulos wrote:
> Hi all,
>
> I'm trying to sort out the situation with Ganeti 2.15.2 in Debian
> unstable. Right now we have GHC 8 and snap-server 1.0 which cause
> Ganeti's build to fail and Ganeti is due to be removed from testing. I have
> (probably) fixed all GHC 8 compatibility issues (mostly template
> Haskell-related stuff) with the attached patch, however it looks as if
> snap-server 1.0 requires substantial changes in metad's error handling to
> work[1]:
>
> [GHC]: src/Ganeti/Metad/WebServer.o <- cabal_macros.h
> src/Ganeti/Metad/WebServer.hs src/Ganeti/Metad/Types.hi
> src/Ganeti/Metad/Types.o src/Ganeti/Metad/Config.hi src/Ganeti/Metad/Config.o
> src/Ganeti/Runtime.hi src/Ganeti/Runtime.o src/Ganeti/Logging.hi
> src/Ganeti/Logging.o src/Ganeti/Constants.hi src/Ganeti/Constants.o
> src/Ganeti/Daemon.hi src/Ganeti/Daemon.o
>
> src/Ganeti/Metad/WebServer.hs:96:20: error:
> * No instance for (MonadError String Snap)
> arising from a use of `lookupInstanceParams'
> * In a stmt of a 'do' block:
> instParams <- lookupInstanceParams inst params
> In the expression:
> do { instParams <- lookupInstanceParams inst params;
>maybeResult (getOsParamsWithVisibility instParams)
>$ \ osParams
>-> writeBS . ByteString.pack . JSON.encode $ osParams }
> In an equation for `serveOsParams':
> serveOsParams inst params
> = do { instParams <- lookupInstanceParams inst params;
> maybeResult (getOsParamsWithVisibility instParams)
> $ \ osParams
> -> writeBS . ByteString.pack . JSON.encode $
> osParams }
> Makefile:4415: recipe for target 'src/Ganeti/Metad/WebServer.o' failed
>
> I'm at a loss here, as my Haskell skills are virtually non-existent, so
> any help is appreciated :)
Hi Apollon,
I saw a new package with a patch, so is this issue closed (modulo
forwarding and integrating the snap-1.0 patch upstream)?
P.S.: the non-trivial TH fixes tell me your Haskell skills are
definitely existent and non-null :)
thanks,
iustin
Re: [PATCH stable-2.15 01/17] Cleanup: Use new-style classes everywhere
On 2016-12-06 21:51:49, Ganeti Development List wrote: > On Tue, Dec 06, 2016 at 09:34:39PM +0100, Iustin Pop wrote: > > On 2016-12-06 17:29:56, Ganeti Development List wrote: > > > This quells pylint's old-style-class warning. For all classes changed by > > > this commit, none of the differences between new-style and old classes > > > matter: we don't subclass any of these classes, or use super()/type(). > > > > I don't know anymore in Python 2.6/2.7 what the status is, but very old > > Python version had the issue that the internals of the class were > > different, e.g. __slots__ only works for new-style classes. Is that > > still the case, or not? > > It's still the case. __slots__ was introduced in 2.2 (along with new-style > classes), and only works with new-style classes. Assigning to __class__ > breaks with Python <2.6 for classes with __slots__. > > See https://docs.python.org/2/reference/datamodel.html#slots > > However the classes in this patch are just used for simple encapsulation, and > there's nothing tricky going on with them -- no subclassing, no metaclass > games, > no overriding of any of the __dunder__ methods, so I think we're pretty safe. LGTM, although honestly given Python, I'm not sure this is best committed on 2.15. With all your other cleanups though, next 2.15 looks to be a more significant "internal cleanup" release though, so fine :) iustin
Re: [PATCH stable-2.15 17/17] Disable pylint too-many-nested-blocks in _RunCmdPipe
On 2016-12-06 17:30:26, Ganeti Development List wrote: > There doesn't appear to be any easy way of reducing the complexity > of this function without moving it elsewhere or completely reorganising > the function, so disable this warning for the time being. LGTM.
Re: [PATCH stable-2.15 16/17] Reduce nesting in import-export ProcessChildIO
On 2016-12-06 17:30:24, Ganeti Development List wrote: > This avoids some pylint too-many-nested-blocks warnings. Do this by > flattening some 'arrow antipattern code' inside the polling loop to use > guards instead. Was not sure what arrow antipattern is, but after squinting at the diff I'll remember it :) LGTM.
Re: [PATCH stable-2.15 15/17] Reduce nesting in LUOobCommand.Exec
On 2016-12-06 17:30:22, Ganeti Development List wrote: > This avoids a pylint too-many-nested-blocks warning. > > NB it's hard to see from the diff because of all the whitespace, but > this just turns the if result.fail_msg check into a guard that continues > the next iteration of the loop, and dedends the rest of the loop body > rather than having it as an else: Actually this is almost readable, because of the empty lines that break the patch into multiple chunks. LGTM.
Re: [PATCH stable-2.15 14/17] Reduce nesting in LUInstanceCreate.RunOsScripts
On 2016-12-06 17:30:20, Ganeti Development List wrote: > This avoids a pylint too-many-nested-blocks warning. > > NB it's hard to see from the diff because of all the whitespace, but > this just replaces the top "if iobj.disk and not self.adopt_disks" with > a pair of guards that return early, and dedents the rest of the fn. The only question is why: > +if not iobj.disks: > + return > + > +if self.adopt_disks: > + return Instead of a single if? LGTM either way (didn't validate git -w though). iustin
Re: [PATCH stable-2.15 13/17] Reduce nesting in RemoveNodeSshKeyBulk key calculation
On 2016-12-06 17:30:18, Ganeti Development List wrote: > This avoids a pylint too-many-nested-blocks warning. This removes the > 6th level of nesting in the function, but may also be marginally faster > by turning the calculation into the set difference operation it really > is. > > No functional change. I had to read the code to confirm this, as this replaces keys[node_info.uuid] rather than updating it. And while reading I was wondering why this isn't a set in the first place, but that's another issue. LGTM
Re: [PATCH stable-2.15 12/17] Reduce nesting in RemoveNodeSshKeyBulk ssh logic
On 2016-12-06 17:30:17, Ganeti Development List wrote: > This avoids a pylint too-many-nested-blocks warning. It also removes > some copy & paste code, showing that the master candidate and ordinary > node case are the same apart from the logging. As common with cleanup, one has to wonder how come it got to that much duplication in the first place :) LGTM, thanks.
Re: [PATCH stable-2.15 11/17] Reduce nesting in gnt-cluster VerifyDisks missing disk loop
On 2016-12-06 17:30:15, Ganeti Development List wrote: > To avoid pylint's too-many-nested-blocks warning. Also improves readability a bit, LGTM.
Re: [PATCH stable-2.15 10/17] Reduce nesting in _CheckVLANArguments
On 2016-12-06 17:30:13, Ganeti Development List wrote: > This avoids a pylint too-many-nested-blocks warning. It also has the > happy side effect of removing some duplicated code. > > No functional change. Nice cleanup, LGTM.
Re: [PATCH stable-2.15 09/17] Reduce nesting in StartDaemon
On 2016-12-06 17:30:11, Ganeti Development List wrote: > This avoids a pylint too-many-nested-blocks warning. > > The extra try: finally: os._exit(1) is unnecessary as _StartDaemonChild > already catches all its exceptions and if it ever finishes, calls > os._exit(1) anyways. LGTM, although this wording: > +# Child process, can't return. Seems to me a bit misleanding - I was confused why can't return, as exec can fail. I'd suggest "Try to start child process, will either execve or exit". But nitpicking, LGTM either way. iustin
Re: [PATCH stable-2.15 08/17] Disable pylint bad-continuation warning
On 2016-12-06 17:30:09, Ganeti Development List wrote: > pylint is much more strict than pep8, and it would be too invasive > (and arguably pointless) to update these right now. LGTM.
Re: [PATCH stable-2.15 07/17] Disable pylint superfluous-parens warning
On 2016-12-06 17:30:08, Ganeti Development List wrote: > There are too many cases where we deliberately wrap expressions in > parens, either to indicate comparisons, or to allow multiline > expressions without line continuation chars, or to clarify confusing > precedence. > > While here, clean up a few unpythonic cases. Nice cleanup, LGTM but one bug below: > diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py > index 3eaf292..b32e62e 100644 > --- a/lib/client/gnt_cluster.py > +++ b/lib/client/gnt_cluster.py > @@ -195,7 +195,7 @@ def InitCluster(opts, args): ># check the disk template types here, as we cannot rely on the type check > done ># by the opcode parameter types >diskparams_keys = set(diskparams.keys()) > - if not (diskparams_keys <= constants.DISK_TEMPLATES): > + if not diskparams_keys > constants.DISK_TEMPLATES: You probably wanted to drop the 'not' but missed it. iustin
Re: [PATCH stable-2.15 03/17] Disable incorrect pylint assigning-non-slot warning
On 2016-12-06 17:30:00, Ganeti Development List wrote: > This occurs pretty heavily in lib/objects.py, where pylint isn't > correctly detecting the __slots__ assignment. This appears to be > a known issue: https://github.com/PyCQA/pylint/issues/379 LGTM.
Re: [PATCH stable-2.15 06/17] Disable pylint redefined-variable-type warning
On 2016-12-06 17:30:06, Ganeti Development List wrote: > There are a large number of cases where Ganeti assigns multiple types > (eg set/list, float/int) to the same variable at different times, and > although these would make a type checking tool very unhappy, they are > benign here (and besides, cleaning all this up would be too invasive). Sadly, true. LGTM.
Re: [PATCH stable-2.15 05/17] Disable pylint too-many-branches warnings
On 2016-12-06 17:30:04, Ganeti Development List wrote: > This is useful, but in some cases is a little too conservative. A fn > can have a lot of branches, but very little nesting, and can still be > easy to understand. This occurs in, eg, XenPvmHypervisor._GetConfig. LGTM.
Re: [PATCH stable-2.15 04/17] Disable pylint broad-except warnings
On 2016-12-06 17:30:02, Ganeti Development List wrote: > These are all deliberate top-level catch-any-unhandled-exception cases, > used for logging and error handling so just get pylint to ignore them. LGTM.
Re: [PATCH stable-2.15 02/17] Quell pylint unbalanced-tuple-unpacking warning
On 2016-12-06 17:29:58, Ganeti Development List wrote: > Both of these functions return a list, not a tuple, and by manually > tracing the code, we can see they always return non-empty lists. > > Change the callers to get the first element of the list rather than > using destructuring. Someday this will break at runtime, but that was true before as well, so LGTM. iustin
Re: [PATCH stable-2.15 01/17] Cleanup: Use new-style classes everywhere
On 2016-12-06 17:29:56, Ganeti Development List wrote: > This quells pylint's old-style-class warning. For all classes changed by > this commit, none of the differences between new-style and old classes > matter: we don't subclass any of these classes, or use super()/type(). I don't know anymore in Python 2.6/2.7 what the status is, but very old Python version had the issue that the internals of the class were different, e.g. __slots__ only works for new-style classes. Is that still the case, or not? iustin
Re: [PATCH stable-2.15 13/37] Fix pylint >1.4 pycurl no-member warnings
On 5 December 2016 at 14:56, 'Brian Foley' via ganeti-devel < ganeti-devel@googlegroups.com> wrote: > On Mon, Dec 05, 2016 at 01:43:57PM +, Federico Pareschi wrote: > > to avoid arbitrary code injection. > > > >Is this safe? Should we be looking more into this or is it something > >that does not affect us? > > The attack vector is someone writing their own pycurl.so with malicious > code > in it, puting that on the python search path, and using that to perform > arbitrary actions as the user running pylint when pylint loads pycurl. > > I think this is a legitimate concern for developers running pylint as > themselves and potentially accepting arbitary patches from the internet > without > checking what's in them, but it shouldn't be much of a problem for a > sandboxed > buildbot. > Even for developers, random patches can't easily ship a .so, so while this is a valid concern for some cases, I think it's less likely for accepting source diffs (that show no binary files) and linting them. I wonder if it's possible to write a git config that rejects binary deltas… The only alternatives I can think of are to explicitly disable the warning > for > any ganeti modules that use pycurl directly, or to explicitly add disables > to > each use of pycurl.foo but I don't much like those. > I tend to agree, this seems a worthwhile tradeoff. iustin
Re: [PATCH stable-2.15 00/37] Cleanup for pylint 1.6.4
On 5 December 2016 at 13:04, 'Brian Foley' via ganeti-devel < ganeti-devel@googlegroups.com> wrote: > On Mon, Dec 05, 2016 at 12:01:14PM +0100, Iustin Pop wrote: > >Quick question: is there a reason to keep that compat, as opposed to > >switching the "blessed" (and required) pylint version to 1.6.4? > >(If this is addressed in the patch series, sorry, didn't read the > >patches) > >iustin > > Hi Iustin, > > no, there isn't any very good reason, I was just playing it safe, and kinda > assumed that make dist or the debian build process might run pylint, but it > appears tht neither do. > As long as we require a fixed version, it's not really feasible to do so. Similarly how -Werror is only enabled in developer builds, I guess. In that case, I'll move us to requiring 1.6.4, and remove a couple of the > compatibility workarounds. > Yes please, that sounds cleaner. thanks, iustin
Re: [PATCH stable-2.15] Fix coexistence of location tags and non-DRBD instances
On 4 December 2016 at 18:44, Brian Foley wrote: > On Fri, Dec 02, 2016 at 11:03:55PM +0100, Iustin Pop wrote: > > From: Iustin Pop > > > > This addresses issue 1185, “hbal: IntMap.!: key -1 is not an element of > > the map”. The issue is that the location tags code presumed all > > instances have a primary and a secondary (i.e., they are DRBD). > > > > The fix is to set the location score for non-DRBD instances to zero, > > since this is what I understand the correct value: such an instance > > cannot be optimized by moving, so its score is "perfect" (zero). > > > > Signed-off-by: Iustin Pop > > LGTM, and thank you. I'm going to do a merge forward of the recent (and a > batch of upcoming) 2.15 patches in the next few days, and this is a nice > fix > to include. > Note: someone with edit rights will need to update the issue once this is committed (or released). Thanks for the review! iustin
Re: [PATCH stable-2.15 00/37] Cleanup for pylint 1.6.4
On 5 December 2016 at 11:35, 'Brian Foley' via ganeti-devel < ganeti-devel@googlegroups.com> wrote: > Ganeti's python code passes the pylint checks in the version of pylint > included with Debian Jessie. Unfortunately this is a really old pylint > (0.26 from 2012) and the latest stable pylint (1.6.4 from 2016) has a > lot of additional checks. Ganeti produces many many warnings and errors > when run against 1.6.4. > > This patchset is a first cut at fixing these, while still maintaining > compatibility with pylint 0.26. > Quick question: is there a reason to keep that compat, as opposed to switching the "blessed" (and required) pylint version to 1.6.4? (If this is addressed in the patch series, sorry, didn't read the patches) iustin
Re: [PATCH stable-2.15] Fix invalid variable error for file-based disks
On 2016-12-04 17:54:41, Ganeti Development List wrote: > On Fri, Dec 02, 2016 at 08:28:13PM +0100, Iustin Pop wrote: > > On 2016-12-02 18:03:33, Ganeti Development List wrote: > > > Introduced by 94e252a33. Found by pylint's undefined-loop-variable > > > warning. > > > > > [snip] > > > Good pylint! LGTM. > > Thanks. Submitted. > > robb...@gentoo.org pointed out to me that the current stable pylint (1.6.4 > from > July 2016) is *much* newer than the version shipped with Jessie (0.26 from > October 2012!) and Ganeti generates a huge number of pylint warnings under > newer > pylints. Interesting. I would have expected moving to a newer pylint to detect some more real issues, but I'm surprised that it actually finds significant numbers of real issues. Python begin easy to write is both its advantage and its nemesis... > I'm trying to sort this mess out, since pylint does sometimes find real > issues. > I've a patchset I'm hoping to send out in the next couple of days which should > fix a large set of these. Sounds good, thanks. iustin
[PATCH stable-2.15] Fix coexistence of location tags and non-DRBD instances
From: Iustin Pop
This addresses issue 1185, “hbal: IntMap.!: key -1 is not an element of
the map”. The issue is that the location tags code presumed all
instances have a primary and a secondary (i.e., they are DRBD).
The fix is to set the location score for non-DRBD instances to zero,
since this is what I understand the correct value: such an instance
cannot be optimized by moving, so its score is "perfect" (zero).
Signed-off-by: Iustin Pop
---
src/Ganeti/HTools/Cluster.hs | 4 ++--
src/Ganeti/HTools/Cluster/Moves.hs | 19 ---
src/Ganeti/HTools/Container.hs | 1 +
src/Ganeti/HTools/Loader.hs| 2 +-
4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/src/Ganeti/HTools/Cluster.hs b/src/Ganeti/HTools/Cluster.hs
index 662f150..fcf5c08 100644
--- a/src/Ganeti/HTools/Cluster.hs
+++ b/src/Ganeti/HTools/Cluster.hs
@@ -360,7 +360,7 @@ allocateOnPair opts stats nl inst new_pdx new_sdx =
in do
Instance.instMatchesPolicy inst (Node.iPolicy tgt_p)
(Node.exclStorage tgt_p)
-let new_inst = Instance.setBoth (setInstanceLocationScore inst tgt_p tgt_s)
+let new_inst = Instance.setBoth (setInstanceLocationScore inst tgt_p (Just
tgt_s))
new_pdx new_sdx
new_p <- Node.addPriEx force tgt_p new_inst
new_s <- Node.addSec tgt_s new_inst new_pdx
@@ -1056,7 +1056,7 @@ getMoves (Table _ initial_il _ initial_plc, Table
final_nl _ _ final_plc) =
(_, cmds) = computeMoves inst inst_name mv np ns
in (affected, idx, mv, cmds)
in map plctoMoves . reverse . drop (length initial_plc) $ reverse final_plc
-
+
-- | Inner function for splitJobs, that either appends the next job to
-- the current jobset, or starts a new jobset.
mergeJobs :: ([JobSet], [Ndx]) -> MoveJob -> ([JobSet], [Ndx])
diff --git a/src/Ganeti/HTools/Cluster/Moves.hs
b/src/Ganeti/HTools/Cluster/Moves.hs
index 5fca893..0150918 100644
--- a/src/Ganeti/HTools/Cluster/Moves.hs
+++ b/src/Ganeti/HTools/Cluster/Moves.hs
@@ -60,11 +60,12 @@ instanceNodes nl inst =
-- | Sets the location score of an instance, given its primary
-- and secondary node.
setInstanceLocationScore :: Instance.Instance -- ^ the original instance
- -> Node.Node -- ^ the primary node of the instance
- -> Node.Node -- ^ the secondary node of the instance
+ -> Node.Node -- ^ the primary node of the
instance
+ -> Maybe Node.Node -- ^ the secondary node of the
instance
-> Instance.Instance -- ^ the instance with the
-- location score updated
-setInstanceLocationScore t p s =
+setInstanceLocationScore t _ Nothing = t { Instance.locationScore = 0 }
+setInstanceLocationScore t p (Just s) =
t { Instance.locationScore =
Set.size $ Node.locationTags p `Set.intersection` Node.locationTags s
}
@@ -105,7 +106,8 @@ applyMoveEx force nl inst (ReplacePrimary new_pdx) =
tgt_n = Container.find new_pdx nl
int_p = Node.removePri old_p inst
int_s = Node.removeSec old_s inst
- new_inst = Instance.setPri (setInstanceLocationScore inst tgt_n int_s)
+ new_inst = Instance.setPri (setInstanceLocationScore inst tgt_n
+ (Just int_s))
new_pdx
force_p = Node.offline old_p || force
new_nl = do -- OpResult
@@ -132,7 +134,8 @@ applyMoveEx force nl inst (ReplaceSecondary new_sdx) =
pnode' = Node.removePri pnode inst
int_s = Node.removeSec old_s inst
force_s = Node.offline old_s || force
- new_inst = Instance.setSec (setInstanceLocationScore inst pnode tgt_n)
+ new_inst = Instance.setSec (setInstanceLocationScore inst pnode
+ (Just tgt_n))
new_sdx
new_nl = do
new_s <- Node.addSecEx force_s tgt_n new_inst old_pdx
@@ -148,7 +151,8 @@ applyMoveEx force nl inst (ReplaceAndFailover new_pdx) =
tgt_n = Container.find new_pdx nl
int_p = Node.removePri old_p inst
int_s = Node.removeSec old_s inst
- new_inst = Instance.setBoth (setInstanceLocationScore inst tgt_n int_p)
+ new_inst = Instance.setBoth (setInstanceLocationScore inst tgt_n
+ (Just int_p))
new_pdx old_pdx
force_s = Node.offline old_s || force
new_nl = do -- OpResult
@@ -167,7 +171,8 @@ applyMoveEx force nl inst (FailoverAndReplace new_sdx) =
int_p = Node.removePri old_p inst
int_s = Node.removeSec old_s inst
force_p = Node.offline old_p || force
- new_inst = Instance.setBoth (setInstanceLocationScore inst int_s tgt_n)
+ new_inst = Instance.setBoth (setInstanceLocationScore inst int_s
+
Re: [PATCH stable-2.15] Fix invalid variable error for file-based disks
On 2016-12-02 18:03:33, Ganeti Development List wrote: > Introduced by 94e252a33. Found by pylint's undefined-loop-variable > warning. > > Signed-off-by: Brian Foley > --- > lib/cmdlib/instance_storage.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/cmdlib/instance_storage.py b/lib/cmdlib/instance_storage.py > index d92a9e8..6919ccf 100644 > --- a/lib/cmdlib/instance_storage.py > +++ b/lib/cmdlib/instance_storage.py > @@ -648,7 +648,7 @@ def GenerateDiskTemplate( > > elif template_name in constants.DTS_FILEBASED: # Gluster handled above >logical_id_fn = \ > -lambda _, disk_index, disk: (file_driver, > +lambda idx, disk_index, disk: (file_driver, > "%s/%s" % (file_storage_dir, > names[idx])) >if template_name == constants.DT_FILE: Good pylint! LGTM. thanks, iustin
Re: [PATCH stable-2.15] Fix for incorrect parsing of DRBD versions
On Thu, Dec 01, 2016 at 04:09:56AM -0800, Ganeti Development List wrote: > > On Thursday, December 1, 2016 at 11:53:43 AM UTC, Iustin Pop wrote: > > > > On Thu, Dec 01, 2016 at 11:38:35AM +, Ganeti Development List wrote: > > > Following issue #1194, this patch allows Ganeti to correctly > > > parse drbd versions that also include a dash in their k-fix > > > version component. > > > > This means 8.4.8-1 and 8.4.8.1 will be treated the same. Is this the > > correct behaviour? > > > > Yes, you are correct. I'm not sure to be honest. I've been looking around > the drbd and > also the debian packaging site but I'm not 100% sure what logic follows > their 'k-fix' > numbering scheme. In drbd, from what I can see, they always use x.y.z-k > except > in one particular (old) version where they do x.y.z.k. > > Honestly, though, as somebody correctly pointed out on the ganeti mailing > list, does it > really matter? Shouldn't we mostly only care about major and minor version > as that > should be what might break compatibility? All in all I'd say we can treat > these as > interchangeable but if somebody disagrees or knows more about this, I'd be > happy to > fix it. No, it doesn't really matter, I was just asking to confirm this is the plan. I'd suggest updating the commit message to record this explicit decision (the current one will not be very clear in 2 years whether it was intended or not). iustin
Re: [PATCH stable-2.15] Fix for incorrect parsing of DRBD versions
On Thu, Dec 01, 2016 at 11:38:35AM +, Ganeti Development List wrote:
> Following issue #1194, this patch allows Ganeti to correctly
> parse drbd versions that also include a dash in their k-fix
> version component.
This means 8.4.8-1 and 8.4.8.1 will be treated the same. Is this the
correct behaviour?
> Signed-off-by: Federico Morg Pareschi
> ---
> lib/storage/drbd_info.py| 17 +++--
> test/py/ganeti.storage.drbd_unittest.py | 10 ++
> 2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/lib/storage/drbd_info.py b/lib/storage/drbd_info.py
> index 99605f1..469ed7f 100644
> --- a/lib/storage/drbd_info.py
> +++ b/lib/storage/drbd_info.py
> @@ -164,13 +164,15 @@ class DRBD8Info(object):
>
>"""
>
> - _VERSION_RE = re.compile(r"^version: (\d+)\.(\d+)\.(\d+)(?:\.(\d+))?"
> + _VERSION_RE = re.compile(r"^version: (\d+)\.(\d+)\.(\d+)(?:[.-](\d+))?"
> r" \(api:(\d+)/proto:(\d+)(?:-(\d+))?\)")
>_VALID_LINE_RE = re.compile("^ *([0-9]+): cs:([^ ]+).*$")
> + _K_FIX_DASH_SEPARATOR_RE = re.compile(r"^version:
> (\d+)\.(\d+)\.(\d+)(?:-)")
>
> + def _GetKFixSeparator(self, lines):
> +"""Check, in case of a K-fix version, if the separator is a dash or
> dot."""
> +
> +first_line = lines[0].strip()
> +match = self._K_FIX_DASH_SEPARATOR_RE.match(first_line)
> +if match is None:
> + return "."
> +else:
> + return "-"
This seems to be done in two steps. Would it be simpler to have
K_FIX_DASH_SEPARATOR itself extract the separator, instead of it having
to match - and if not, return . vs -?
You could even get rid of _K_FIX_DASH_SEPARATOR_RE, and extract the
separator from the _VERSION_RE, after changing the RE to have the
separator a capturing group.
thanks,
iustin
Re: [PATCH master] Fix for instance reinstall not updating config
On 2016-11-28 14:10:39, Ganeti Development List wrote: > From the modified log line it looks like it was intentional, but as we > discussed I don't see a good reason for this. Reinstalling OS image without > changing the configuration creates an inconsistency between the state of > the world and state of record and we want to _always_ minimise this. The current way would allow reinstalling an instance without changing the parameters (for whatever reason); it's always possible to change first and then reinstall if things needs to be kept in sync. Not saying this makes sense, just that it might be the reason behind it. The commit message (83f54ca) doesn't mention anything. > Also > is it happening only on master ? Other than that LGTM. It was introduced in 2.12 beta 1, so somewhat old. Might need a NEWS file entry for changed behaviour. regards, iustin
Re: pylint of master, on Gentoo
On 2016-10-01 23:18:59, Ganeti Development List wrote: > On Sat, Oct 01, 2016 at 11:40:47PM +0200, Iustin Pop wrote: > > On 2016-10-01 19:55:30, Robin H. Johnson wrote: > > > If you're running pylint on jessie as you say, then it's a really old > > > pylint, > > > so I suspect much of this may just be newer checks then. I did think > > > jessie-backports had a relatively modern pylint however. > > > > Some historical context: this is not about jessie being old, it's the > > fact that Ganeti depends on an exact pylint version. > > > > Pylint checks change significantly between versions (sometimes even for > > minor versions), so one cannot do automated checks at commit time unless > > the checks are stable; hence the requirement for an exact version, and > > not what the current distro has, so that all devs see the same output as > > buildbot/commit checks. > > > > It would be very good if someone spent the time to cleanup the codebase > > and bump up the pylint version, but as I see it from your output it's a > > lot of style changes (of almost no value) instead of actual Python > > fixes. > > That said, when the codebase doesn't trigger lots of spurious warnings, > pylint is good at picking up errors (variable name typos etc) that it's > all too easy to make with Python. It's almost like having a compiler! :-P Correct, it's the closest thing to that in Python, and it is indeed very useful. I just wished that it is more stable. > I wouldn't mind investing some effort into a mix of turning off newer > pylint messages if we think they're unhelpful, and fixing the rest. I don't think they need turning off; I guess most likely there are more granular indentation/style settings, which need to be set according to Ganeti's style; presuming, of course, that for those new things (e.g. hanging continuations after '|' , etc.) the style is consistent throughout the codebase :) regards, iustin
Re: pylint of master, on Gentoo
On 2016-10-01 19:55:30, Robin H. Johnson wrote: > If you're running pylint on jessie as you say, then it's a really old pylint, > so I suspect much of this may just be newer checks then. I did think > jessie-backports had a relatively modern pylint however. Some historical context: this is not about jessie being old, it's the fact that Ganeti depends on an exact pylint version. Pylint checks change significantly between versions (sometimes even for minor versions), so one cannot do automated checks at commit time unless the checks are stable; hence the requirement for an exact version, and not what the current distro has, so that all devs see the same output as buildbot/commit checks. It would be very good if someone spent the time to cleanup the codebase and bump up the pylint version, but as I see it from your output it's a lot of style changes (of almost no value) instead of actual Python fixes. regards, iustin
Re: [PATCH stable-2.15] Update NEWS file for 2.15.3
On 2016-10-01 08:56:02, Ganeti Development List wrote: > (Will update with actual release date on release) A few wording improvements below, but otherwise LGTM. Feel free to apply or discard any/all of them, they are just suggestions. > +Version 2.15.3 > +-- > +*(Released %a, %d %b %Y)* > + > +Compatibility improvements > +~~ > +- Use socat method string compatible with <1.73 & >=1.73 > +- Fixup compatibility with GHC 7.4/base 4.5 > +- Update hv_kvm to handle output from qemu >= 1.6.0 > +- KVM: handle gracefully too old/too new psutil versions > + > +Performance fixes > +~ > +- Bulk-removal of SSH keys to reduce renew-crypto from O(n^2) to to ? > +- Don't deepcopy the config if the old value is not needed This line is correct, but a bit opaque for users. Maybe "Improve speed of configuration update by eliminating redundant copies"? > +- Optimise codegen for Python OpCode classes > +- Optimize LXC hypervisor GetAllInstancesInfo > +- Fix ClusterVerifyConfig() causing high mem usage > +- Fix memory/perf bug in gnt-cluster verify > +- Prevent InstanceShutdown from waiting on success > +- Improve luxid QueryInstances performance for large clusters > +- luxid/confd: Do not add a new Inotify watchers on timer > +- luxid/confd: Reduce heap when parsing & storing ConfigData 10% "Reduce memory usage for the configuration database by ~10%"? > +- luxid: Make JQScheduler queues more strict to avoid leaks "Fix memory leaks in the luxid job queue scheduler"? > +- Fix ganeti-rapi/noded exit-under-load bug > + > +Minor changes > +~ > +- Misc docs/logging/error message improvements > +- mcpu: Raise caught OpPrereqErrors with too few args Maybe instead, "fix handling of OpPrereqErrors when too few arguments are passed"? > +- Cancel RAPI job if the client drops the connection > +- Give atomicWriteFile temp filenames a more distinct pattern > +- Misc linting fixes > +- Misc test improvements > + > +Fixes inherited from 2.14 branch > + > +- Allow attaching disks to diskless instances > +- Support userspace disk URIs for OS import/export scripts > +- iallocator: only adjust memory usage for up instances > +- Fix failover in case the source node is offline > + > +Fixes inherited from 2.13 branch > + > +- Migrate needs HypervisorClass, not an instance > + > +Fixes inherited from 2.12 branch > + > +- Avoid breaking clusters at master failover (#1163/#1159) > +- Increase logging during gnt-cluster upgrade > + > +Fixes inherited from 2.10 branch > + > +- KVM: explicitly configure routed NICs late > + > > Version 2.15.2 > -- > -- > 2.8.0.rc3.226.g39d4020 >
Re: RFC: Releasing Ganeti 2.15.3
On 30 September 2016 at 12:30, 'Brian Foley' via ganeti-devel < ganeti-devel@googlegroups.com> wrote: > On Fri, Sep 30, 2016 at 11:47:24AM +0200, Iustin Pop wrote: > >On 29 September 2016 at 19:21, 'Brian Foley' via ganeti-devel > ><[1]ganeti-devel@googlegroups.com> wrote: > > > > Hi all, > > since December 2015 quite a large number of commits have been made > > to the > > stable-2.15 branch (87 to be exact). These cover the gamut from > > performance > > improvements, to compatibility fixes, to error handling, and even > > some minor > > feature improvements. There should be no compatibility breaks > > though. > > You can see the full list with git log --oneline > > v2.15.2..stable-2.15 > > I've tried to categorise them all below. I'd like us to do a > > (final?) patch > > release of 2.15.3, as the patches includes an important fix for > > socat 1.7.3 > > compatibility that's affected quite a few users. > > Does anyone else have anything they'd really like to see fixed in > > 2.15, or does > > anyone have any objections to including any of the below in a point > > release? > > > >As far as my investigations on > >[2]https://code.google.com/p/ganeti/issues/detail?id=1185 went, it > >seems that the location setup tags completely broke htools on clusters > >with non-DRBD instances. It's a trivial breakage, not a design one, so > >fixing it shouldn't be too invasive. > >Day work has been crazy busy so I didn't manage to finish up a patch, > >but I think this should be fixed in 2.15 stable. When were you > planning > >the release for? > > Hi Iustin, > > I completely forgot about #1185. Yeah, it would definitely be worth fixing > this if it's not too hard. > > I was hoping to get a release done in the next 2-3 weeks if we could, but > I've no fixed schedule in mind and I'll go with whatever suits people. > Ah, OK, that should very doable. I thought you're planning for the next few days; I'll try to send a patch then sometimes next week. thanks! iustin
Re: RFC: Releasing Ganeti 2.15.3
On 29 September 2016 at 19:21, 'Brian Foley' via ganeti-devel < ganeti-devel@googlegroups.com> wrote: > Hi all, > > since December 2015 quite a large number of commits have been made to the > stable-2.15 branch (87 to be exact). These cover the gamut from performance > improvements, to compatibility fixes, to error handling, and even some > minor > feature improvements. There should be no compatibility breaks though. > > You can see the full list with git log --oneline v2.15.2..stable-2.15 > > I've tried to categorise them all below. I'd like us to do a (final?) patch > release of 2.15.3, as the patches includes an important fix for socat 1.7.3 > compatibility that's affected quite a few users. > > Does anyone else have anything they'd really like to see fixed in 2.15, or > does > anyone have any objections to including any of the below in a point > release? > As far as my investigations on https://code.google.com/p/ganeti/issues/detail?id=1185 went, it seems that the location setup tags completely broke htools on clusters with non-DRBD instances. It's a trivial breakage, not a design one, so fixing it shouldn't be too invasive. Day work has been crazy busy so I didn't manage to finish up a patch, but I think this should be fixed in 2.15 stable. When were you planning the release for? thanks, iustin
Re: [PATCH stable-2.16] Add missing cluster modify --modify-etc-hosts to man
On 2016-08-16 18:35:32, Ganeti Development List wrote: > Tweak formatting while here. LGTM, thanks. iustin
Re: [PATCH stable-2.16] Fix some typos/poor phrasing in gnt-node man page
On 2016-08-16 16:17:06, Ganeti Development List wrote: > No change in meaning/options. LGTM, thanks.
Re: [PATCH stable-2.16] Fix typos in gnt-cluster man page
On Fri, Aug 12, 2016 at 03:03:18PM +0100, Ganeti Development List wrote: > Luckily, nothing that changes the meaning anywhere. > > Signed-off-by: Brian Foley LGTM, thanks.
Re: [PATCH stable-2.16] Hide errors for expected inotify failures in unittest
On 12 August 2016 at 15:38, Brian Foley wrote:
> On Fri, Aug 12, 2016 at 03:13:53PM +0200, Iustin Pop wrote:
> >On 12 August 2016 at 15:04, 'Brian Foley' via ganeti-devel
> ><[1]ganeti-devel@googlegroups.com> wrote:
> >
> > This makes it a little easier to eyeball the output of make
> > py-tests.
> >
> >Ooh, nice, this is a very old bug, thanks!
> >
> > +logger = logging.getLogger('pyinotify')
> > +logger.propagate = False
> > self.assertRaises(errors.InotifyError, handler.enable)
> > self.assertRaises(errors.InotifyError, handler.enable)
> > handler.disable()
> > self.assertRaises(errors.InotifyError, handler.enable)
> > +logger.propagate = True
> >
> >Do you want to put a try/finally around the asserts that ensure
> >propagate is restored?
> >iustin
>
> Good point. There are paths in pyinotify's add_watch that can throw
> exceptions
> (although I don't think we use them). Submitted with your change.
>
Excellent, LGTM :)
iustin
Re: [PATCH stable-2.16] Hide errors for expected inotify failures in unittest
On 12 August 2016 at 15:04, 'Brian Foley' via ganeti-devel <
ganeti-devel@googlegroups.com> wrote:
> This makes it a little easier to eyeball the output of make py-tests.
>
Ooh, nice, this is a very old bug, thanks!
+logger = logging.getLogger('pyinotify')
> +logger.propagate = False
> self.assertRaises(errors.InotifyError, handler.enable)
> self.assertRaises(errors.InotifyError, handler.enable)
> handler.disable()
> self.assertRaises(errors.InotifyError, handler.enable)
> +logger.propagate = True
>
Do you want to put a try/finally around the asserts that ensure propagate
is restored?
iustin
Re: [PATCH stable-2.16] Make 'make pep8' happy
On 2016-07-08 17:26:39, Ganeti Development List wrote: > Signed-off-by: Brian Foley > --- > lib/cmdlib/cluster/verify.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/cmdlib/cluster/verify.py b/lib/cmdlib/cluster/verify.py > index 8785fbc..9ad598d 100644 > --- a/lib/cmdlib/cluster/verify.py > +++ b/lib/cmdlib/cluster/verify.py > @@ -780,7 +780,7 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors): > if constants.NV_MASTERIP not in nresult: >self._ErrorMsg(constants.CV_ENODENET, ninfo.name, > "node hasn't returned node master IP reachability data") > -elif nresult[constants.NV_MASTERIP] == False: # be explicit, could be > None > +elif nresult[constants.NV_MASTERIP] is False: # be explicit, could be > None LGTM for this fix, but this looks somewhat anti-pattern in Python. If we care about None vs. False, should the check for None be done first, and then false/true could follow naturally? thanks, iustin
Re: [PATCH stable-2.16] Fix some trivial pep8/pylint errors
On 7 July 2016 at 13:05, 'Brian Foley' via ganeti-devel < ganeti-devel@googlegroups.com> wrote: > Whitespace and an unused variable. > LGTM ,thanks. iustin
Re: [PATCH stable-2.16 2/2] Make executeRpcCall only compute rpcCallData once
On 5 July 2016 at 11:44, 'Brian Foley' via ganeti-devel < ganeti-devel@googlegroups.com> wrote: > On Mon, Jul 04, 2016 at 08:32:12PM +0200, Iustin Pop wrote: > > On 2016-07-04 16:57:24, Ganeti Development List wrote: > > > This is important for distMCsAsyncTask, because currently every time > > > config.data is updated, wconfd generates a separate copy of the Base64 > > > encoded, zlib compressed config.data payload string for the > > > /upload_file_single call sent to each master candidate's noded. > > > > > > This patch causes it to generate one copy of the string and send that > > > in parallel to all MCs. > > > > > > Signed-off-by: Brian Foley > > > > LGTM, with the mention that the 3 different names for executing a rpc > > call are too close together. I don't have good suggestions though, so > > good as is. > > > > thanks! > > iustin > > I agree, the names aren't wonderful and I've probably made them worse. I'll > clean them up in a later patch. > > How about sendSingleRpc, sendMultipleRpcs, sendRpcsUsingData? > I though about such naming too but it's still misleading. All calls send multiple RPCs, the different is whether it's the same payload or not. What about sendSameRpc, sendVaryingRpcs, sendRpcsUsingData (in the latter case it's up to the caller whether it's the same or not). iustin
Re: [PATCH stable-2.16 2/2] Make executeRpcCall only compute rpcCallData once
On 2016-07-04 16:57:24, Ganeti Development List wrote: > This is important for distMCsAsyncTask, because currently every time > config.data is updated, wconfd generates a separate copy of the Base64 > encoded, zlib compressed config.data payload string for the > /upload_file_single call sent to each master candidate's noded. > > This patch causes it to generate one copy of the string and send that > in parallel to all MCs. > > Signed-off-by: Brian Foley LGTM, with the mention that the 3 different names for executing a rpc call are too close together. I don't have good suggestions though, so good as is. thanks! iustin
Re: [PATCH stable-2.16 1/2] Remove storage unit selection from rpcCallData
On 2016-07-04 16:57:23, Ganeti Development List wrote: > This means the one case where rpcCallData needed to use a Node can be > removed, simplifying the rpcCallData interface and making the payload > computation independent of the node its sent to. > > Signed-off-by: Brian Foley LGTM, very nice cleanup, thanks! (Signed-off-by: ius...@google.com).
Re: [PATCH master 2/6] Add bandwidth tags and bandwidth map fields into Node
On 28 June 2016 at 16:16, Даниил Лещёв wrote: > > I have to main concerns here. >> >> 1. I still believe it's wrong to model this on a per-node basis, and that >> it should be rather two things: bandwidth available inside a node group >> (between any two arbitrary nodes), and bandwidth available between any two >> node groups. However, your badwidth tag concept is nice. Hmm… >> > > But with such approach we lose an opportunity to specify network speed > between two specific nodes. > Or it is really rare case and we don't need such flexibility? > That's what I believe, yes. The concept of node groups was introduced do model (approximately) high-bandwidth connectivity, with inter-node-group bandwidth being potentially smaller/more contented, but still symmetrical. I don't know of any deployments that had per-node different bandwidth, but maybe some other people have such cases? CC-ing a couple of other people to see their input as well. thanks, iustin
Re: [PATCH master 2/6] Add bandwidth tags and bandwidth map fields into Node
On 28 June 2016 at 11:28, Даниил Лещёв wrote: > > > The bandwidths map store data about network speed >> > between current node and given Node by it's bandwidth tags. >> >> Filling this up will take some space in a large cluster. Is it really >> necessary to store this by nodes (which makes it O(n²) in terms of >> nodes), or simply in terms of node group wide-bandwidth, plus >> inter-node group bandwidth? >> > > I think, I definitely said wrong. > At the moment bandwidth map store information about network speed between > current node and some bandwidth tag (not another node). > I see. > So at the best case (all nodes in cluster have the same bandwidth tag) you > store O(n) per cluster and in the worst case O(n^2) (all nodes have > different bandwidth tags). > If you only have group-wide bandwidth tags you will store O(n*g) per > cluster, where n is the number of nodes and g is the number of groups (And > I think it is the most common case). > > Should I use some single place for storing network speed information in > order to avoid data duplication? > I have to main concerns here. 1. I still believe it's wrong to model this on a per-node basis, and that it should be rather two things: bandwidth available inside a node group (between any two arbitrary nodes), and bandwidth available between any two node groups. However, your badwidth tag concept is nice. Hmm… 2. Storing the information somewhere else then nodes would indeed be useful. So let's discuss the main point: how do you or anybody else think that bandwidth should be represented? Per node, per node group, etc.? thanks! iustin
Re: [PATCH master 2/6] Add bandwidth tags and bandwidth map fields into Node
On 2016-06-23 17:45:34, mele...@gmail.com wrote: > From: Daniil Leshchev > > The bandwidths map store data about network speed > between current node and given Node by it's bandwidth tags. Filling this up will take some space in a large cluster. Is it really necessary to store this by nodes (which makes it O(n²) in terms of nodes), or simply in terms of node group wide-bandwidth, plus inter-node group bandwidth? thanks, iustin
Re: [PATCH master 0/6] New balancing options implementation
On 24 June 2016 at 14:36, Oleg Ponomarev wrote: > Hi Iustin, > > > I'll look at the patches, but if I read correctly—these are currently > stored as tags. Would it make more sense to have > > them as proper values in the objects, so that (in the future) they can > be used by other parts of the code? Just a thought. > > Do you have any ideas about how network bandwidth might be used in Ganeti > itself? > At my first glance, this information might be useful in HTools only. And > in this case, node tags is the common way to pass the information. It's the > same mechanism as used in HTools to obtain location, migration, desired > location and some other information. > I don't have very strong examples, but the reason I mention this is that I regard network capacity in a nodegroup as a physical characteristic of the hardware, not something that is htools-specific, so modelling it explicitly might have some value. For example, it might make sense to say that the number of concurrent disk replaces on a given node, or the number of concurrent instance migrations, might depend on the network bandwidth, such that Ganeti would automatically adjust the concurrency of such jobs per node, without needing external control. That is however far-fetched, so I'm not proposing any change to the code per se; I was asking to see what others think of this. regards, iustin On Fri, Jun 24, 2016 at 3:17 PM Iustin Pop wrote: > >> On 23 June 2016 at 18:32, Даниил Лещёв wrote: >> >>> >>>> I would slightly prefer if we discuss it over plain email (without >>>> patches), to see what you think about how complex the network model needs >>>> to be, and whether a static "time X" vs. semi-dynamic (based on the >>>> instance disk size) approach is best. >>>> >>>> Maybe there was some more information back at the start of the project? >>>> (I only started watching the mailing list again recently). >>>> >>>> The initial plan was to implement "static" solutions, based on instance >>> disk size and then make it "dynamic" by using information about network >>> speed from data collectors. >>> >> >> Ack. >> >> At the moment, we have "semi-dynamic" solution, I think. The new tags may >>> specify network speed in cluster (and between different parts of cluster). >>> >> >> I'll look at the patches, but if I read correctly—these are currently >> stored as tags. Would it make more sense to have them as proper values in >> the objects, so that (in the future) they can be used by other parts of the >> code? Just a thought. >> >> I am assuming that this speed remains constant since the network usually >>> configured once and locally (for example in server rack). >>> >> >> That makes sense. >> >> >>> I think, with such assumption, the network speed stays almost constant >>> and the time estimations for balancing solutions become predictable. >>> >>> I suggest to use the new options for discarding solutions, that takes >>> long time and slightly changes the state of the cluster. >>> In my mind the time to perform disk replication is directly depends on >>> the network bandwidth. >>> >> >> Hmm, depends. On a gigabyte or 10G network and with mechanical >> harddrives, the time will depend more on disk load. >> >> thanks, >> iustin >> >
Re: [PATCH master 0/6] New balancing options implementation
On 23 June 2016 at 18:32, Даниил Лещёв wrote: > >> I would slightly prefer if we discuss it over plain email (without >> patches), to see what you think about how complex the network model needs >> to be, and whether a static "time X" vs. semi-dynamic (based on the >> instance disk size) approach is best. >> >> Maybe there was some more information back at the start of the project? >> (I only started watching the mailing list again recently). >> >> The initial plan was to implement "static" solutions, based on instance > disk size and then make it "dynamic" by using information about network > speed from data collectors. > Ack. At the moment, we have "semi-dynamic" solution, I think. The new tags may > specify network speed in cluster (and between different parts of cluster). > I'll look at the patches, but if I read correctly—these are currently stored as tags. Would it make more sense to have them as proper values in the objects, so that (in the future) they can be used by other parts of the code? Just a thought. I am assuming that this speed remains constant since the network usually > configured once and locally (for example in server rack). > That makes sense. > I think, with such assumption, the network speed stays almost constant and > the time estimations for balancing solutions become predictable. > > I suggest to use the new options for discarding solutions, that takes long > time and slightly changes the state of the cluster. > In my mind the time to perform disk replication is directly depends on the > network bandwidth. > Hmm, depends. On a gigabyte or 10G network and with mechanical harddrives, the time will depend more on disk load. thanks, iustin
Re: [PATCH master 0/6] New balancing options implementation
On 23 June 2016 at 17:42, Даниил Лещёв wrote: > Hi, Iustin > > >> Oh, no worries, I just wanted to know if Daniil acknowledged the comments >> or not. >> >> Anyway, comments are welcome here and the discussion is still open:) >>> >> >> > The only reason why I didn't reply to your comments is that I wanted to > show patchset in order to discuss ideas in design document in more detailed > way. > Hope, that was not a big mistake. > Oh no, not a mistake at all. > I'm also going to rewrite patch for design document (append information > about bandwidth tags). > I would slightly prefer if we discuss it over plain email (without patches), to see what you think about how complex the network model needs to be, and whether a static "time X" vs. semi-dynamic (based on the instance disk size) approach is best. Maybe there was some more information back at the start of the project? (I only started watching the mailing list again recently). thanks! iustin
Re: [PATCH master 0/6] New balancing options implementation
On 23 June 2016 at 17:08, Oleg Ponomarev wrote: > Hi Iustin, Daniil, > > The reason for Daniil not to reply immediately is his GSoC midterm > evaluation coming soon. As the implementation represents his work during > the first month of GSoC, it's necessary to share it with the community at > this point. We have discussed your comments on design document and Daniil > took them into account. Still, I don't understand why Daniil decided not to > spend 5-10 minutes to reply in the design document discussion thread. > Oh, no worries, I just wanted to know if Daniil acknowledged the comments or not. Anyway, comments are welcome here and the discussion is still open:) > Sounds good. And thanks Daniil for the commits. > Of course, looking forward to see this implemented! thanks! iustin On Thu, Jun 23, 2016 at 5:49 PM 'Iustin Pop' via ganeti-devel < > ganeti-devel@googlegroups.com> wrote: > >> On 23 June 2016 at 16:45, wrote: >> >>> From: Daniil Leshchev >>> >>> The patchset introduces new command line options >>> (--long-solution-threshold" and --avoid-long-solutions"). >>> That gives an ability for HBal to avoid balancing solutions, >>> that take significant amount of time. >>> >> >> Daniil, I've replied to your design doc changes, but I haven't seen a >> reply. That discussion would be useful before implementing this :) >> >> regards, >> iustin >> >
Re: [PATCH master 0/6] New balancing options implementation
On 23 June 2016 at 16:45, wrote: > From: Daniil Leshchev > > The patchset introduces new command line options > (--long-solution-threshold" and --avoid-long-solutions"). > That gives an ability for HBal to avoid balancing solutions, > that take significant amount of time. > Daniil, I've replied to your design doc changes, but I haven't seen a reply. That discussion would be useful before implementing this :) regards, iustin
Re: [PATCH stable-2.15 1/2] KVM: handle gracefully too old psutil versions
On 21 June 2016 at 16:28, Brian Foley wrote:
> On Sat, Jun 18, 2016 at 04:53:25AM +0200, Iustin Pop wrote:
> > On 2016-06-15 10:23:57, Brian Foley wrote:
> > > Additionally, 0.5.0 had a psutil.Process.{get,set}_cpu_affinity() API,
> > > which we use in the kvm code. In 2.0.0 API was renamed to
> cpu_affinity()
> > > and the old API deprecated, and in 3.0 the old {get,set} API was
> removed.
> > >
> > > So we currently need to restrict psutils to 2.x, but maybe it's just
> > > easier to add an __attr__ check for the new affinity API, but perhaps
> > > this should be done in a separate patch.
> >
> > Just to see if I understand: we don't support 3.x+ due to the
> > cpu_affinity API only?
>
> I think so yes. That's the only thing I saw when I looked through the
> psutil
> version history a few months ago.
>
> Thinking a little more about what I wrote above, since we require psutil
> >= 2.0
> for kvm anyway, then it would be perfectly fine to replace the use of the
> 0.5.0
> affinity API in the kvm code with the >= 2.0 API. I don't know if this on
> its
> own would be sufficient to get it to work with psutil >= 3.0, but it would
> be
> a step in the right direction. We can look at this another time though.
>
Yes, that's why I asked—it was kind of surprising to see the use of 0.5.0
API but not actually supporting 0.5.0 :)
thanks,
iustin
Re: [PATCH stable-2.15] KVM: handle gracefully too old/too new psutil versions
On 21 June 2016 at 16:29, Brian Foley wrote: > On Sat, Jun 18, 2016 at 04:54:51AM +0200, Iustin Pop wrote: > > From: Iustin Pop > > > > My previous pylint cleanups were done without psutil installed; as soon > > I installed it, pylint showed that the wheezy's version of psutil is too > > old (0.5.1), not having the cpu_count() function which was introduced in > > 2.0.0. Furthermore, thanks to Brian, it turns out that too new versions > > are also unsupported. > > > > This change adds a simple way to detect this and to degrade gracefully > > by throwing an appropriate exception instead of an unclear one at > > runtime. Tested with wheezy's 0.5.0 and sid's 4.1.1 versions. It also > > updates the INSTALL file to note the supported versions and that > > [2.0.0,2.2.0) had issues with FH leaks. > > > > Signed-off-by: Iustin Pop > > LGTM, and sorry for the delay in reviewing. > No worries, thanks!
Re: [PATCH master] Add proposal of a new data collector and new command-line options
On 2016-06-12 21:22:41, mele...@gmail.com wrote: > From: Daniil Leshchev > > Introduce new command-line options for configuring > balancing process. > > Introduce the data collector for gathering information > about network speed. This information can be used in order > to optimize time of cluster balancing. > --- > doc/design-migration-speed-hbal.rst | 44 > + > 1 file changed, 44 insertions(+) > > diff --git a/doc/design-migration-speed-hbal.rst > b/doc/design-migration-speed-hbal.rst > index a0dcfe0..14b867e 100644 > --- a/doc/design-migration-speed-hbal.rst > +++ b/doc/design-migration-speed-hbal.rst > @@ -26,3 +26,47 @@ a compromise between moves speed and optimal scoring. This > can be implemented > by introducing ``--avoid-disk-moves *FACTOR*`` option which will admit disk > moves only if the gain in the cluster metrics is *FACTOR* times > higher than the gain achievable by non disk moves. > + > +Avoiding insignificant long-time solutions > +== > + > +The next step is to estimate an amount of time required to perform a > balancing > +step and introduce a new term: ``long-time`` solution. > + > +``--long-solution-threshold`` option will specify a duration in seconds. > +A solution exceeding the duration is a ``long-time`` solution by definition. > + > +With time estimations we will be able to filter Hbal's sequences and not > allow > +to perform long-time solutions without enough gain in cluster metric. This > can > +be done by introducing ``--avoid-long-solutions *FACTOR*`` option, which will > +admit only long-time solutions whose K/N metrics are more, than *FACTOR* > where > +K is gain of such solution and N is an estimated time to perform it. > + > +As a result we can achieve almost similar improvement of the cluster metrics > +after balancing with significant decrease of time to balancing. Interesting. The avoid-long-solutions factor makes sense, but the long-solution-threshold as based on a fixed value makes me wonder if it's the appropriate metric. Depending on cluster state, an operation's given speed can vary greatly. Given that the only scenario that takes long and a variable amount of time is replicating disks when using DRBD, my original plan a few years back was to add an instance's disk size into the scoring factors, but I never got around to that. > +Network bandwidth estimation > + > + > +Balancing time can be estimated by taking amount of data to be moved and > +current network bandwidth between each pair of affected nodes. > + > +We propose to add a new data collector, that will gather information about > +network speed by sending some amount of data. By counting time to perform > this, > +we can estimate average network speed between any two nodes in the cluster. > + > +DataCollector implementation details > + > + > +As a first approach we suggest implement dummy data collector whose output > +could be configured by user. > + > +For serious data collector it's useless to send tiny packets less than 100Kb, > +because of time to connection establishing. Since in almost all > implementations > +of TCP/IP stack MTU is limited to approximately 1500 bytes, we propose also > not > +to use *ping* command, but implement own process of package sending or for > +example parse output from *scp* command. > + > +During *dcUpdate* every data collector sends requests to other nodes and > +measures time to get response. So after master node invoke *dcReport* > +on all collectors, it will get full graph of network speed. Is this complex setup needed? What kind of network setups do you have in mind where the need to compute available replication bandwidth in real time is useful? In common Ganeti deployments using DRBD, replication is done over a private network where only replication and instance migration is taking place. As such, the network is fully symmetric and 'dedicated' to Ganeti; a fixed configuration parameter (node-group level) would approximate the available bandwidth well enough, I think. What do you think? regards, iustin
Re: [PATCH stable-2.16] Special case WaitForJobChange to reduce heap use
On 2016-06-15 17:25:18, Ganeti Development List wrote: > As we talked offline, I like the fact that we are not going through the > generic handleCall if we do not need the config regardless of the > optimisation (which is awesome and is the main motivation for this patch). > I'm wondering if we still need to keep in the expensive code path - I don't > see a reason why someone would want to query other fields in WaitForJobChange > than status or logs (I think these are the only ones that change during the > lifetime of the job). Maybe we could drop the expensive code path in later > versions. I discussed this with Brian briefly over chat a while ago. I regard this as a good but temporary stop-gap, as it shouldn't be needed to manually handle routing differently. Better type system separation should allow releasing the config if not needed automatically. It would be good to invest into that, rather than (IMHO) expanding the manual workaround. Just my 2c :) iustin
[PATCH stable-2.15] KVM: handle gracefully too old/too new psutil versions
From: Iustin Pop
My previous pylint cleanups were done without psutil installed; as soon
I installed it, pylint showed that the wheezy's version of psutil is too
old (0.5.1), not having the cpu_count() function which was introduced in
2.0.0. Furthermore, thanks to Brian, it turns out that too new versions
are also unsupported.
This change adds a simple way to detect this and to degrade gracefully
by throwing an appropriate exception instead of an unclear one at
runtime. Tested with wheezy's 0.5.0 and sid's 4.1.1 versions. It also
updates the INSTALL file to note the supported versions and that
[2.0.0,2.2.0) had issues with FH leaks.
Signed-off-by: Iustin Pop
---
INSTALL | 4 +++-
lib/hypervisor/hv_kvm/__init__.py | 17 +++--
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/INSTALL b/INSTALL
index e33ab96..95f3019 100644
--- a/INSTALL
+++ b/INSTALL
@@ -42,7 +42,9 @@ Before installing, please verify that you have the following
programs:
- `Paramiko <http://www.lag.net/paramiko/>`_, if you want to use
``ganeti-listrunner``
- `psutil Python module <https://github.com/giampaolo/psutil>`_,
- optional python package for supporting CPU pinning under KVM
+ optional python package for supporting CPU pinning under KVM, versions
+ 2.x.x only; beware that versions from 2.0.0 to before 2.2.0 had a
+ number of file handle leaks, so running at least 2.2.0 is advised
- `fdsend Python module <https://gitorious.org/python-fdsend>`_,
optional Python package for supporting NIC hotplugging under KVM
- `qemu-img <http://qemu.org/>`_, if you want to use ``ovfconverter``
diff --git a/lib/hypervisor/hv_kvm/__init__.py
b/lib/hypervisor/hv_kvm/__init__.py
index 4df0246..8271e0e 100644
--- a/lib/hypervisor/hv_kvm/__init__.py
+++ b/lib/hypervisor/hv_kvm/__init__.py
@@ -45,7 +45,17 @@ import urllib2
from bitarray import bitarray
try:
import psutil # pylint: disable=F0401
+ if psutil.version_info < (2, 0, 0):
+# The psutil version seems too old, we ignore it
+psutil_err = "too old (2.x.x needed, %s found)" % psutil.__version__
+psutil = None
+ elif psutil.version_info >= (3,):
+psutil_err = "too new (2.x.x needed, %s found)" % psutil.__version__
+psutil = None
+ else:
+psutil_err = ""
except ImportError:
+ psutil_err = "not found"
psutil = None
try:
import fdsend # pylint: disable=F0401
@@ -716,11 +726,14 @@ class KVMHypervisor(hv_base.BaseHypervisor):
"""
if psutil is None:
- raise errors.HypervisorError("psutil Python package not"
- " found; cannot use CPU pinning under KVM")
+ raise errors.HypervisorError("psutil Python package %s"
+ "; cannot use CPU pinning"
+ " under KVM" % psutil_err)
target_process = psutil.Process(process_id)
if cpus == constants.CPU_PINNING_OFF:
+ # we checked this at import time
+ # pylint: disable=E1101
target_process.set_cpu_affinity(range(psutil.cpu_count()))
else:
target_process.set_cpu_affinity(cpus)
--
2.8.1
Re: [PATCH stable-2.15 1/2] KVM: handle gracefully too old psutil versions
On 2016-06-15 10:23:57, Brian Foley wrote:
> On Wed, Jun 15, 2016 at 07:18:38AM +0200, Iustin Pop wrote:
> > From: Iustin Pop
> >
> > My previous pylint cleanups were done without psutil installed; as soon
> > I installed it, pylint showed that the wheezy's version of psutil is too
> > old (0.5.1), not having the cpu_count() function which was introduced in
> > 2.0.0.
> >
> > This change adds a simple way to detect this and to degrade gracefully
> > by throwing an appropriate exception instead of an unclear one at
> > runtime.
>
> This is good, but we could take it a little further. First of all we
> support any version 2.x version of psutil from 2.0.0 to 2.2.1. However
> we probably want to warn if the user is running <2.2.0 because 2.2.0
> fixed a number of file handle leaks.
Uh, that is hard to do properly, I don't know if many people watch
configure-time warnings… I'll add a note to INSTALL.
> Additionally, 0.5.0 had a psutil.Process.{get,set}_cpu_affinity() API,
> which we use in the kvm code. In 2.0.0 API was renamed to cpu_affinity()
> and the old API deprecated, and in 3.0 the old {get,set} API was removed.
>
> So we currently need to restrict psutils to 2.x, but maybe it's just
> easier to add an __attr__ check for the new affinity API, but perhaps
> this should be done in a separate patch.
Just to see if I understand: we don't support 3.x+ due to the
cpu_affinity API only?
Patch coming shortly.
iustin
Re: [PATCH stable-2.17] Prevent watcher from holding lock on verify group
On 17 June 2016 at 10:36, Federico Pareschi wrote: > Yes, this is definitely the case. > We have ideas to implement something in the future to obviate this > problem, although we're still considering exactly how to implement it. > This is a short-term quick fix to solve some blocker issues that show up > as a consequence of it. > Ack, thanks for the info! iustin > On 17 June 2016 at 17:57, Iustin Pop wrote: > >> 2016-06-17 9:46 GMT-07:00 Federico Pareschi : >> >>> When a ganeti-watcher runs on the nodegroup, it submits the verify-group >>> job. If there is another job in the queue that is taking some locks that >>> stop the verify-group job (like an instance creation) then the whole >>> ganeti-watcher is blocked and has to wait for that job to finish. >>> >>> We have a case where we need the ganeti-watcher's periodic check to >>> restart some downed instances, but the ganeti-watcher itself gets stuck on >>> some other jobs and those downed instances don't get brought back up on >>> time (And this causes problems to us). >>> >>> There really is no reason to hold a lock to the watcher state file when >>> submitting the verify disk job anyway. >>> >> >> All this makes sense. My question is rather, if we end up with multiple >> watchers basically running (waiting) concurrently, can we end up with >> multiple (redundant) verify-group jobs? >> >> Sorry if I misunderstand the situation. >> >> iustin >> >> On 17 June 2016 at 17:18, Iustin Pop wrote: >>> >>>> 2016-06-17 8:31 GMT-07:00 'Federico Morg Pareschi' via ganeti-devel < >>>> ganeti-devel@googlegroups.com>: >>>> >>>>> The ganeti-watcher holds the group file lock for too long, until after >>>>> the execution of a group-verify-disk job. This locks for a long time if >>>>> there are other jobs already running and blocking the verify from >>>>> executing. When the lock is held, another ganeti-watcher run cannot be >>>>> scheduled, so this prevents the ganeti-watcher from running for several >>>>> minutes. >>>>> >>>>> With this commit, the lock is released before running the VerifyDisks >>>>> operation, so even if the submitted job gets stuck in the Job Queue, a >>>>> subsequient ganeti-watcher run would still happen. >>>>> >>>> >>>> Quick question: what prevents a runaway case where VerifyDisks is >>>> blocking for hours and we have many watchers all running and submitting >>>> their own VerifyDisks? >>>> >>>> thanks, >>>> iustin >>>> >>> >>> >> >
Re: [PATCH stable-2.17] Prevent watcher from holding lock on verify group
2016-06-17 9:46 GMT-07:00 Federico Pareschi : > When a ganeti-watcher runs on the nodegroup, it submits the verify-group > job. If there is another job in the queue that is taking some locks that > stop the verify-group job (like an instance creation) then the whole > ganeti-watcher is blocked and has to wait for that job to finish. > > We have a case where we need the ganeti-watcher's periodic check to > restart some downed instances, but the ganeti-watcher itself gets stuck on > some other jobs and those downed instances don't get brought back up on > time (And this causes problems to us). > > There really is no reason to hold a lock to the watcher state file when > submitting the verify disk job anyway. > All this makes sense. My question is rather, if we end up with multiple watchers basically running (waiting) concurrently, can we end up with multiple (redundant) verify-group jobs? Sorry if I misunderstand the situation. iustin On 17 June 2016 at 17:18, Iustin Pop wrote: > >> 2016-06-17 8:31 GMT-07:00 'Federico Morg Pareschi' via ganeti-devel < >> ganeti-devel@googlegroups.com>: >> >>> The ganeti-watcher holds the group file lock for too long, until after >>> the execution of a group-verify-disk job. This locks for a long time if >>> there are other jobs already running and blocking the verify from >>> executing. When the lock is held, another ganeti-watcher run cannot be >>> scheduled, so this prevents the ganeti-watcher from running for several >>> minutes. >>> >>> With this commit, the lock is released before running the VerifyDisks >>> operation, so even if the submitted job gets stuck in the Job Queue, a >>> subsequient ganeti-watcher run would still happen. >>> >> >> Quick question: what prevents a runaway case where VerifyDisks is >> blocking for hours and we have many watchers all running and submitting >> their own VerifyDisks? >> >> thanks, >> iustin >> > >
Re: [PATCH stable-2.17] Prevent watcher from holding lock on verify group
2016-06-17 8:31 GMT-07:00 'Federico Morg Pareschi' via ganeti-devel < ganeti-devel@googlegroups.com>: > The ganeti-watcher holds the group file lock for too long, until after > the execution of a group-verify-disk job. This locks for a long time if > there are other jobs already running and blocking the verify from > executing. When the lock is held, another ganeti-watcher run cannot be > scheduled, so this prevents the ganeti-watcher from running for several > minutes. > > With this commit, the lock is released before running the VerifyDisks > operation, so even if the submitted job gets stuck in the Job Queue, a > subsequient ganeti-watcher run would still happen. > Quick question: what prevents a runaway case where VerifyDisks is blocking for hours and we have many watchers all running and submitting their own VerifyDisks? thanks, iustin
[PATCH stable-2.15 2/2] Cleanup more pylint/pep8/apidoc errors
From: Iustin Pop
Hopefully this makes stable-2.15 clean and able to pass a buildbot run.
The changes should all be self-explanatory, except test/mocks.py one:
there were more unused arguments, so I added a silence for that at class
level, and removed the '_' on _ec_id since it was superfluous now.
Signed-off-by: Iustin Pop
---
lib/cli.py | 3 ++-
lib/cmdlib/cluster/verify.py | 6 +++---
lib/rapi/rlib2.py| 1 +
src/Ganeti/Objects/Disk.hs | 2 +-
test/py/mocks.py | 12
5 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/lib/cli.py b/lib/cli.py
index a470ffa..362f2ae 100644
--- a/lib/cli.py
+++ b/lib/cli.py
@@ -747,7 +747,7 @@ def GenericPollJob(job_id, cbs, report_cbs, cancel_fn=None,
timer += constants.CLI_WFJC_FREQUENCY
else:
result = cbs.WaitForJobChangeOnce(job_id, ["status"], prev_job_info,
- prev_logmsg_serial, timeout=update_freq)
+prev_logmsg_serial,
timeout=update_freq)
if not result:
# job not found, go away!
raise errors.JobLost("Job with id %s lost" % job_id)
@@ -906,6 +906,7 @@ class _LuxiJobPollCb(JobPollCbBase):
"""
return self.cl.CancelJob(job_id)
+
class FeedbackFnJobPollReportCb(JobPollReportCbBase):
def __init__(self, feedback_fn):
"""Initializes this class.
diff --git a/lib/cmdlib/cluster/verify.py b/lib/cmdlib/cluster/verify.py
index 2b1d075..5c80c18 100644
--- a/lib/cmdlib/cluster/verify.py
+++ b/lib/cmdlib/cluster/verify.py
@@ -109,8 +109,8 @@ class _VerifyErrors(object):
@type error_descriptor: tuple (string, string, string)
@param error_descriptor: triplet describing the error (object_type,
code, description)
-@type obj_name: string
-@param obj_name: name of object (instance, node ..) the error relates to
+@type object_name: string
+@param object_name: name of object (instance, node ..) the error relates to
@type message_list: list of strings
@param message_list: body of error messages
@type log_type: string
@@ -133,7 +133,7 @@ class _VerifyErrors(object):
log_type, error_code, object_type, object_name, msg))
else:
if not object_name:
-object_name = ""
+object_name = ""
for msg in message_list:
prefixed_list.append(" - %s: %s %s: %s" % (
log_type, object_type, object_name, msg))
diff --git a/lib/rapi/rlib2.py b/lib/rapi/rlib2.py
index 8916004..14c12ac 100644
--- a/lib/rapi/rlib2.py
+++ b/lib/rapi/rlib2.py
@@ -200,6 +200,7 @@ def _UpdateBeparams(inst):
return inst
+
def _CheckIfConnectionDropped(sock):
"""Utility function to monitor the state of an open connection.
diff --git a/src/Ganeti/Objects/Disk.hs b/src/Ganeti/Objects/Disk.hs
index c9f498d..129cd11 100644
--- a/src/Ganeti/Objects/Disk.hs
+++ b/src/Ganeti/Objects/Disk.hs
@@ -81,7 +81,7 @@ instance Show LogicalVolume where
showsPrec _ (LogicalVolume g v) =
showString g . showString "/" . showString v
--- | Check the constraints for a VG/LV names (except the @\/dev\/@ check).
+-- | Check the constraints for VG\/LV names (except the @\/dev\/@ check).
instance Validatable LogicalVolume where
validate (LogicalVolume g v) = do
let vgn = "Volume group name"
diff --git a/test/py/mocks.py b/test/py/mocks.py
index 406caca..b48125b 100644
--- a/test/py/mocks.py
+++ b/test/py/mocks.py
@@ -113,6 +113,7 @@ class FakeGLM(object):
class FakeContext(object):
"""Fake context object"""
+ # pylint: disable=W0613
def __init__(self):
self.cfg = FakeConfig()
@@ -124,9 +125,10 @@ class FakeContext(object):
def GetRpc(self, cfg):
return None
- def GetWConfdContext(self, _ec_id):
+ def GetWConfdContext(self, ec_id):
return (None, None, None)
+
class FakeGetentResolver(object):
"""Fake runtime.GetentResolver"""
@@ -154,6 +156,7 @@ class FakeGetentResolver(object):
def LookupGid(self, gid):
return "group%s" % gid
+
class FakeLU(object):
HPATH = "fake-lu"
HTYPE = None
@@ -161,7 +164,7 @@ class FakeLU(object):
def __init__(self, processor, op, cfg, rpc_runner, prereq_err):
self.proc = processor
self.cfg = cfg
-self.op = op
+self.op = op
self.rpc = rpc_runner
self.prereq_err = prereq_err
@@ -170,7 +173,7 @@ class FakeLU(object):
self.dont_collate_locks = dict.fromkeys(locking.LEVELS, False)
self.add_locks = {}
-self.LogWarning = processor.LogWarning
+self.LogWarning = processor.LogWarning # pylint: disable=C0103
def CheckArguments(self):
pass
@@ -184,7 +187,6 @@ class FakeLU(object):
def CheckPrereq(self):
if self.prereq_err:
raise self.prereq_err
-
[PATCH stable-2.15 1/2] KVM: handle gracefully too old psutil versions
From: Iustin Pop
My previous pylint cleanups were done without psutil installed; as soon
I installed it, pylint showed that the wheezy's version of psutil is too
old (0.5.1), not having the cpu_count() function which was introduced in
2.0.0.
This change adds a simple way to detect this and to degrade gracefully
by throwing an appropriate exception instead of an unclear one at
runtime.
Signed-off-by: Iustin Pop
---
lib/hypervisor/hv_kvm/__init__.py | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/lib/hypervisor/hv_kvm/__init__.py
b/lib/hypervisor/hv_kvm/__init__.py
index 4df0246..0e5ecd0 100644
--- a/lib/hypervisor/hv_kvm/__init__.py
+++ b/lib/hypervisor/hv_kvm/__init__.py
@@ -45,13 +45,20 @@ import urllib2
from bitarray import bitarray
try:
import psutil # pylint: disable=F0401
+ psutil_err = ""
except ImportError:
+ psutil_err = "not found"
psutil = None
try:
import fdsend # pylint: disable=F0401
except ImportError:
fdsend = None
+if psutil is not None and not hasattr(psutil, 'cpu_count'):
+ # The psutil version seems too old, we ignore it
+ psutil_err = "too old (2.0.0 needed, %s found)" % psutil.__version__
+ psutil = None
+
from ganeti import utils
from ganeti import constants
from ganeti import errors
@@ -716,11 +723,14 @@ class KVMHypervisor(hv_base.BaseHypervisor):
"""
if psutil is None:
- raise errors.HypervisorError("psutil Python package not"
- " found; cannot use CPU pinning under KVM")
+ raise errors.HypervisorError("psutil Python package %s"
+ "; cannot use CPU pinning"
+ " under KVM" % psutil_err)
target_process = psutil.Process(process_id)
if cpus == constants.CPU_PINNING_OFF:
+ # we checked this at import time
+ # pylint: disable=E1101
target_process.set_cpu_affinity(range(psutil.cpu_count()))
else:
target_process.set_cpu_affinity(cpus)
--
2.8.1
[PATCH stable-2.15 0/2] A couple of more 2.15 fixes
From: Iustin Pop The first one shows why pylint, as annoying as it is sometimes, is useful: as far as I see, we don't have any other version check for psutil, so this was indeed broken. The second patch is rather trivial. Hopefully these two will for real make 2.15 doclint builder good again. Iustin Pop (2): KVM: handle gracefully too old psutil versions Cleanup more pylint/pep8/apidoc errors lib/cli.py| 3 ++- lib/cmdlib/cluster/verify.py | 6 +++--- lib/hypervisor/hv_kvm/__init__.py | 14 -- lib/rapi/rlib2.py | 1 + src/Ganeti/Objects/Disk.hs| 2 +- test/py/mocks.py | 12 6 files changed, 27 insertions(+), 11 deletions(-) -- 2.8.1
Re: [PATCH stable-2.15 0/2] Make stable-2.15 distcheck-compliant
On 2016-06-14 12:22:21, Brian Foley wrote: > On Tue, Jun 14, 2016 at 06:04:00AM +0200, Iustin Pop wrote: > > From: Iustin Pop > > > > This two patches (well, only the first, the second is cleanup) make > > stable-2.15 > > pass "make distcheck" on wheezy again. > > > > LGTM, thanks, Thanks. Looking at the external buildbot, it seems there are two more things which I forgot that it runs (and which aren't run from distcheck…), so this is not yet green. I'll send sometimes a few more trivial patches like this, sorry for the merge pain. iustin
Re: [PATCH master 2/2] Fix running unittests without python-psutil
On 2016-06-14 13:36:38, Brian Foley wrote: > On Tue, Jun 14, 2016 at 06:10:28AM +0200, Iustin Pop wrote: > > From: Iustin Pop > > > > Python 2.7 has a very nice extension to the unittest module to support > > test skipping, but until we officially stop supporting it we pretend > > these pass. > > Cool. I didn't know about that. However in the meantime would it be > worth getting them to print something rather than silently skipping > (and possibly leading the developer into a false sense of security) > > Maybe > print "skipped 'psutil Python package not found'" Sounds good. It will make the log output noisy, but that's the intention. Can you do this at git am time, or do you want me to resend? thanks, iustin
[PATCH master 0/2] Make master distcheck-compliant
From: Iustin Pop And this is the last couple of patches. Merge 2.16 to 2.17 to master before. Iustin Pop (2): Fix remaining pylint/check-local issues Fix running unittests without python-psutil lib/hypervisor/hv_kvm/__init__.py| 2 +- test/py/cmdlib/test_unittest.py | 3 ++- test/py/ganeti.hypervisor.hv_kvm_unittest.py | 9 + 3 files changed, 12 insertions(+), 2 deletions(-) mode change 100644 => 100755 test/py/cmdlib/test_unittest.py -- 2.8.1
[PATCH master 1/2] Fix remaining pylint/check-local issues
From: Iustin Pop Two trivial things—indentation and line length. Signed-off-by: Iustin Pop --- lib/hypervisor/hv_kvm/__init__.py | 2 +- test/py/cmdlib/test_unittest.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) mode change 100644 => 100755 test/py/cmdlib/test_unittest.py diff --git a/lib/hypervisor/hv_kvm/__init__.py b/lib/hypervisor/hv_kvm/__init__.py index e626bbf..de7c044 100644 --- a/lib/hypervisor/hv_kvm/__init__.py +++ b/lib/hypervisor/hv_kvm/__init__.py @@ -895,7 +895,7 @@ class KVMHypervisor(hv_base.BaseHypervisor): else: target_process.set_cpu_affinity(cpus) for p in target_process.get_children(recursive=True): - p.set_cpu_affinity(cpus) +p.set_cpu_affinity(cpus) @classmethod def _AssignCpuAffinity(cls, cpu_mask, worker_cpu_mask, process_id, diff --git a/test/py/cmdlib/test_unittest.py b/test/py/cmdlib/test_unittest.py old mode 100644 new mode 100755 index 16cc092..9e538ab --- a/test/py/cmdlib/test_unittest.py +++ b/test/py/cmdlib/test_unittest.py @@ -85,7 +85,8 @@ class TestLUTestDelay(CmdlibTestCase): self.ExecOpCode(op) -self.rpc.call_test_delay.assert_called_once_with([self.master_uuid], DELAY_DURATION) +self.rpc.call_test_delay.assert_called_once_with( + [self.master_uuid], DELAY_DURATION) def testFailingRpc(self): op = opcodes.OpTestDelay(duration=DELAY_DURATION, -- 2.8.1
[PATCH master 2/2] Fix running unittests without python-psutil
From: Iustin Pop Python 2.7 has a very nice extension to the unittest module to support test skipping, but until we officially stop supporting it we pretend these pass. Signed-off-by: Iustin Pop --- test/py/ganeti.hypervisor.hv_kvm_unittest.py | 9 + 1 file changed, 9 insertions(+) diff --git a/test/py/ganeti.hypervisor.hv_kvm_unittest.py b/test/py/ganeti.hypervisor.hv_kvm_unittest.py index 55ffb9b..53097fd 100755 --- a/test/py/ganeti.hypervisor.hv_kvm_unittest.py +++ b/test/py/ganeti.hypervisor.hv_kvm_unittest.py @@ -633,6 +633,9 @@ class TestKvmCpuPinning(testutils.GanetiTestCase): self.params = constants.HVC_DEFAULTS[constants.HT_KVM].copy() def testCpuPinningDefault(self): +if hv_kvm.psutil is None: + # FIXME: switch to unittest.skip once python 2.6 is deprecated + return mock_process = mock.MagicMock() cpu_mask = self.params['cpu_mask'] worker_cpu_mask = self.params['worker_cpu_mask'] @@ -646,6 +649,9 @@ class TestKvmCpuPinning(testutils.GanetiTestCase): mock.call(range(0,1237))) def testCpuPinningPerVcpu(self): +if hv_kvm.psutil is None: + # FIXME: switch to unittest.skip once python 2.6 is deprecated + return mock_process = mock.MagicMock() mock_process.set_cpu_affinity = mock.MagicMock() mock_process.set_cpu_affinity().return_value = True @@ -671,6 +677,9 @@ class TestKvmCpuPinning(testutils.GanetiTestCase): mock.call([15, 16, 17])) def testCpuPinningEntireInstance(self): +if hv_kvm.psutil is None: + # FIXME: switch to unittest.skip once python 2.6 is deprecated + return mock_process = mock.MagicMock() mock_process.set_cpu_affinity = mock.MagicMock() mock_process.set_cpu_affinity().return_value = True -- 2.8.1
[PATCH stable-2.16 1/2] Fix line-too long errors
From: Iustin Pop
These are all trivial changes.
Signed-off-by: Iustin Pop
---
lib/http/server.py | 9 ++---
src/Ganeti/Config.hs | 3 ++-
test/py/ganeti.storage.bdev_unittest.py| 6 --
test/py/ganeti.storage.gluster_unittest.py | 3 ++-
4 files changed, 14 insertions(+), 7 deletions(-)
mode change 100644 => 100755 test/py/ganeti.storage.gluster_unittest.py
diff --git a/lib/http/server.py b/lib/http/server.py
index 8b3a4ee..ffdf790 100644
--- a/lib/http/server.py
+++ b/lib/http/server.py
@@ -423,7 +423,8 @@ class HttpServerRequestExecutor(object):
try:
http.Handshake(sock, self.WRITE_TIMEOUT)
except http.HttpSessionHandshakeUnexpectedEOF:
-logging.debug("Unexpected EOF from %s:%s" % (client_addr[0],
client_addr[1]))
+logging.debug("Unexpected EOF from %s:%s" % (client_addr[0],
+ client_addr[1]))
# Ignore rest
return
@@ -609,8 +610,10 @@ class HttpServer(http.HttpBase, asyncore.dispatcher):
t_setup = time.time()
self.request_executor(self, self.handler, connection, client_addr)
t_end = time.time()
-logging.debug("Request from %s:%s executed in: %.4f [setup: %.4f]
[workers: %d]" % (
-client_addr[0], client_addr[1], t_end - t_start, t_setup -
t_start, len(self._children)))
+logging.debug("Request from %s:%s executed in: %.4f"
+ " [setup: %.4f] [workers: %d]" % (
+client_addr[0], client_addr[1], t_end - t_start,
+t_setup - t_start, len(self._children)))
except Exception: # pylint: disable=W0703
logging.exception("Error while handling request from %s:%s",
diff --git a/src/Ganeti/Config.hs b/src/Ganeti/Config.hs
index 5a48316..9e11611 100644
--- a/src/Ganeti/Config.hs
+++ b/src/Ganeti/Config.hs
@@ -185,7 +185,8 @@ getNodeInstances cfg nname =
sec_insts :: [Instance]
sec_insts = [inst |
(inst, disks) <- inst_disks,
- s_uuid <- mapMaybe (\d -> (instPrimaryNode inst) >>=
(computeDiskSecondaryNode d)) disks,
+ s_uuid <- mapMaybe (\d -> (instPrimaryNode inst) >>=
+(computeDiskSecondaryNode d)) disks,
s_uuid == nname]
in (pri_inst, sec_insts)
diff --git a/test/py/ganeti.storage.bdev_unittest.py
b/test/py/ganeti.storage.bdev_unittest.py
index a894e8f..80aa2af 100755
--- a/test/py/ganeti.storage.bdev_unittest.py
+++ b/test/py/ganeti.storage.bdev_unittest.py
@@ -328,9 +328,11 @@ class TestLogicalVolume(testutils.GanetiTestCase):
vg_name="xenvg",
size=350.00, free=500.00,
attributes="wz--n-", lv_list=[])]
-self.pv_info_no_space = [objects.LvmPvInfo(name="/dev/sda5",
vg_name="xenvg",
+self.pv_info_no_space = [objects.LvmPvInfo(name="/dev/sda5",
+ vg_name="xenvg",
size=350.00, free=0.00,
- attributes="wz--n-",
lv_list=[])]
+ attributes="wz--n-",
+ lv_list=[])]
def testParseLvInfoLine(self):
diff --git a/test/py/ganeti.storage.gluster_unittest.py
b/test/py/ganeti.storage.gluster_unittest.py
old mode 100644
new mode 100755
index 8204885..291d825
--- a/test/py/ganeti.storage.gluster_unittest.py
+++ b/test/py/ganeti.storage.gluster_unittest.py
@@ -160,7 +160,8 @@ class TestGlusterStorage(testutils.GanetiTestCase):
@testutils.patch_object(gluster.GlusterVolume, "Mount")
@testutils.patch_object(ssconf.SimpleStore, "GetGlusterStorageDir")
@testutils.patch_object(gluster.GlusterStorage, "Attach")
- def testCreate(self, attach_mock, storage_dir_mock, mount_mock,
create_file_mock):
+ def testCreate(self, attach_mock, storage_dir_mock, mount_mock,
+ create_file_mock):
attach_mock.return_value = True
storage_dir_mock.return_value = "/testmount"
--
2.8.1
[PATCH stable-2.16 0/2] Make stable-2.16 distcheck-compliant
From: Iustin Pop This is now for the 2.16 stable branch. Before committing these, merge with 2.15 after my two previous patches, with the conflicts solved as follows: - lib/cluster/verify.py: keep our version (the one with enable/disable), dropping the C0xx message as not needed - src/Ganeti/THH.hs: keep their version, as the function was removed Iustin Pop (2): Fix line-too long errors Fix lint or silence errors lib/backend.py | 1 + lib/http/client.py | 1 + lib/http/server.py | 9 ++--- lib/storage/base.py| 1 + lib/storage/bdev.py| 3 +-- src/Ganeti/Config.hs | 5 +++-- src/Ganeti/JQScheduler.hs | 5 +++-- src/Ganeti/Rpc.hs | 2 +- test/py/ganeti.storage.bdev_unittest.py| 6 -- test/py/ganeti.storage.gluster_unittest.py | 3 ++- 10 files changed, 23 insertions(+), 13 deletions(-) mode change 100644 => 100755 test/py/ganeti.storage.gluster_unittest.py -- 2.8.1
[PATCH stable-2.16 2/2] Fix lint or silence errors
From: Iustin Pop
Some of the them are fixed (e.g. unused variable), others are simply
silenced especially when there's no context of why the code was
committed as such (and I don't want to change things which are
non-obvious).
Signed-off-by: Iustin Pop
---
lib/backend.py| 1 +
lib/http/client.py| 1 +
lib/http/server.py| 10 +-
lib/storage/base.py | 1 +
lib/storage/bdev.py | 3 +--
src/Ganeti/Config.hs | 2 +-
src/Ganeti/JQScheduler.hs | 5 +++--
src/Ganeti/Rpc.hs | 2 +-
8 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/lib/backend.py b/lib/backend.py
index f0ce380..5df30a5 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -3807,6 +3807,7 @@ def BlockdevGetmirrorstatusMulti(disks):
lvs_cache = None
is_plain_disk = compat.any([_CheckForPlainDisk(d) for d in disks])
if is_plain_disk:
+# pylint: disable=W0212
lvs_cache = bdev.LogicalVolume._GetLvGlobalInfo()
for disk in disks:
try:
diff --git a/lib/http/client.py b/lib/http/client.py
index 4da5aec..7cea000 100644
--- a/lib/http/client.py
+++ b/lib/http/client.py
@@ -227,6 +227,7 @@ class _PendingRequest(object):
try:
# LOCAL_* options added in pycurl 7.21.5
+ # pylint: disable=E1101
from_str = "from %s:%s " % (
curl.getinfo(pycurl.LOCAL_IP),
curl.getinfo(pycurl.LOCAL_PORT)
diff --git a/lib/http/server.py b/lib/http/server.py
index ffdf790..993745c 100644
--- a/lib/http/server.py
+++ b/lib/http/server.py
@@ -423,8 +423,8 @@ class HttpServerRequestExecutor(object):
try:
http.Handshake(sock, self.WRITE_TIMEOUT)
except http.HttpSessionHandshakeUnexpectedEOF:
-logging.debug("Unexpected EOF from %s:%s" % (client_addr[0],
- client_addr[1]))
+logging.debug("Unexpected EOF from %s:%s",
+ client_addr[0], client_addr[1])
# Ignore rest
return
@@ -611,9 +611,9 @@ class HttpServer(http.HttpBase, asyncore.dispatcher):
self.request_executor(self, self.handler, connection, client_addr)
t_end = time.time()
logging.debug("Request from %s:%s executed in: %.4f"
- " [setup: %.4f] [workers: %d]" % (
-client_addr[0], client_addr[1], t_end - t_start,
-t_setup - t_start, len(self._children)))
+ " [setup: %.4f] [workers: %d]",
+ client_addr[0], client_addr[1], t_end - t_start,
+ t_setup - t_start, len(self._children))
except Exception: # pylint: disable=W0703
logging.exception("Error while handling request from %s:%s",
diff --git a/lib/storage/base.py b/lib/storage/base.py
index 461fdad..000f5db 100644
--- a/lib/storage/base.py
+++ b/lib/storage/base.py
@@ -96,6 +96,7 @@ class BlockDev(object):
def __eq__(self, other):
if not isinstance(self, type(other)):
return False
+# pylint: disable=W0212
return (self._children == other._children and
self.dev_path == other.dev_path and
self.unique_id == other.unique_id and
diff --git a/lib/storage/bdev.py b/lib/storage/bdev.py
index 1f95004..e0f16ed 100644
--- a/lib/storage/bdev.py
+++ b/lib/storage/bdev.py
@@ -510,10 +510,9 @@ class LogicalVolume(base.BlockDev):
logging.warning("lvs command returned an empty output, the LV cache will"
"be empty!")
return {}
-info = {}
return dict([LogicalVolume._ParseLvInfoLine(line, sep) for line in out])
- def Attach(self, lv_info=None):
+ def Attach(self, lv_info=None): # pylint: disable=W0221
"""Attach to an existing LV.
This method will try to see if an existing and active LV exists
diff --git a/src/Ganeti/Config.hs b/src/Ganeti/Config.hs
index 9e11611..fa9d183 100644
--- a/src/Ganeti/Config.hs
+++ b/src/Ganeti/Config.hs
@@ -174,7 +174,7 @@ getNodeInstances cfg nname =
let all_insts = M.elems . fromContainer . configInstances $ cfg
all_disks = fromContainer . configDisks $ cfg
-pri_inst = filter ((== Just nname) . instPrimaryNode) $ all_insts
+pri_inst = filter ((== Just nname) . instPrimaryNode) all_insts
find_disk :: String -> Maybe Disk
find_disk d_uuid = M.lookup (UTF8.fromString d_uuid) all_disks
diff --git a/src/Ganeti/JQScheduler.hs b/src/Ganeti/JQScheduler.hs
index df6fefc..73c0f6b 100644
--- a/src/Ganeti/JQScheduler.hs
+++ b/src/Ganeti/JQScheduler.hs
@@ -134,7 +134,7 @@ unreadJob job = JobWithStat {jJob=job, jStat=nullFStat,
jINotify=Nothing}
-- | Reload interval for polling the running jobs for updates in microseconds.
watchInterval :: Int
-watchInterval = C.luxidJobqueuePollInterval * 100
+watchInterval =
[PATCH stable-2.15 2/2] Fix small typo in opcode tests
From: Iustin Pop
Signed-off-by: Iustin Pop
---
test/hs/Test/Ganeti/OpCodes.hs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/test/hs/Test/Ganeti/OpCodes.hs b/test/hs/Test/Ganeti/OpCodes.hs
index 229696f..d677b4c 100644
--- a/test/hs/Test/Ganeti/OpCodes.hs
+++ b/test/hs/Test/Ganeti/OpCodes.hs
@@ -611,7 +611,7 @@ case_AllDefined = do
extra_hs = hs_ops \\ py_ops
HUnit.assertBool ("Missing OpCodes from the Haskell code:\n" ++
unlines extra_py) (null extra_py)
- HUnit.assertBool ("Extra OpCodes in the Haskell code code:\n" ++
+ HUnit.assertBool ("Extra OpCodes in the Haskell code:\n" ++
unlines extra_hs) (null extra_hs)
-- | Custom HUnit test case that forks a Python process and checks
--
2.8.1
[PATCH stable-2.15 1/2] Fix line-too long errors
From: Iustin Pop
This does mostly trivial line-too-long fixes. The only non-trivial is
the rewriting of the checkNonOptDef function, although I think this is
worth anyway as the new version is (IMHO) more readable.
Signed-off-by: Iustin Pop
---
lib/cmdlib/cluster/verify.py | 4 +++-
src/Ganeti/THH.hs| 29 -
2 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/lib/cmdlib/cluster/verify.py b/lib/cmdlib/cluster/verify.py
index 977887a..2b1d075 100644
--- a/lib/cmdlib/cluster/verify.py
+++ b/lib/cmdlib/cluster/verify.py
@@ -139,7 +139,9 @@ class _VerifyErrors(object):
log_type, object_type, object_name, msg))
# Report messages via the feedback_fn
-self._feedback_fn(constants.ELOG_MESSAGE_LIST, prefixed_list) # pylint:
disable=E1101,C0302
+# pylint: disable=E1101
+self._feedback_fn(constants.ELOG_MESSAGE_LIST, prefixed_list)
+# pylint: enable=E1101
# do not mark the operation as failed for WARN cases only
if log_type == self.ETYPE_ERROR:
diff --git a/src/Ganeti/THH.hs b/src/Ganeti/THH.hs
index 443ce31..b27dcfd 100644
--- a/src/Ganeti/THH.hs
+++ b/src/Ganeti/THH.hs
@@ -307,15 +307,16 @@ actualFieldType f | fieldIsOptional f `elem`
[NotOptional, AndRestArguments] = t
-- | Checks that a given field is not optional (for object types or
-- fields which should not allow this case).
checkNonOptDef :: (Monad m) => Field -> m ()
-checkNonOptDef (Field { fieldIsOptional = OptionalOmitNull
- , fieldName = name }) =
- fail $ "Optional field " ++ (T.unpack name) ++ " used in parameter
declaration"
-checkNonOptDef (Field { fieldIsOptional = OptionalSerializeNull
- , fieldName = name }) =
- fail $ "Optional field " ++ (T.unpack name) ++ " used in parameter
declaration"
-checkNonOptDef (Field { fieldDefault = (Just _), fieldName = name }) =
- fail $ "Default field " ++ (T.unpack name) ++ " used in parameter
declaration"
-checkNonOptDef _ = return ()
+checkNonOptDef field
+ | fieldIsOptional field == OptionalOmitNull = failWith kOpt
+ | fieldIsOptional field == OptionalSerializeNull = failWith kOpt
+ | isJust (fieldDefault field) = failWith kDef
+ | otherwise = return ()
+ where failWith kind = fail $ kind ++ " field " ++ name
+++ " used in parameter declaration"
+name = T.unpack (fieldName field)
+kOpt = "Optional"
+kDef = "Default"
-- | Construct a function that parses a field value. If the field has
-- a custom 'fieldRead', it's applied to @o@ and used. Otherwise
@@ -325,8 +326,9 @@ parseFn :: Field -- ^ The field definition
-> Q Exp -- ^ The resulting function that parses a JSON message
parseFn field o =
let fnType = [t| JSON.JSValue -> JSON.Result $(fieldType field) |]
- expr = maybe [| readJSONWithDesc $(stringE . T.unpack $ fieldName field)
|]
- (`appE` o) (fieldRead field)
+ expr = maybe
+ [| readJSONWithDesc $(stringE . T.unpack $ fieldName field) |]
+ (`appE` o) (fieldRead field)
in sigE expr fnType
-- | Produces the expression that will de-serialise a given
@@ -803,7 +805,7 @@ genOpCode name cons = do
genOpConsFields :: OpCodeConstructor -> Clause
genOpConsFields (cname, _, _, fields, _) =
let op_id = deCamelCase cname
- fieldnames f = (T.unpack . fieldName $ f):(map T.unpack $ fieldExtraKeys
f)
+ fieldnames f = map T.unpack $ fieldName f:fieldExtraKeys f
fvals = map (LitE . StringL) . sort . nub $ concatMap fieldnames fields
in Clause [LitP (StringL op_id)] (NormalB $ ListE fvals) []
@@ -1164,7 +1166,8 @@ defaultFromJSArray keys xs = do
-- See 'defaultToJSArray' and 'defaultFromJSArray'.
genArrayObjectInstance :: Name -> [Field] -> Q Dec
genArrayObjectInstance name fields = do
- let fnames = map T.unpack $ concatMap (liftA2 (:) fieldName fieldExtraKeys)
fields
+ let fnames = map T.unpack $
+ concatMap (liftA2 (:) fieldName fieldExtraKeys) fields
instanceD (return []) (appT (conT ''ArrayObject) (conT name))
[ valD (varP 'toJSArray) (normalB [| defaultToJSArray $(lift fnames) |]) []
, valD (varP 'fromJSArray) (normalB [| defaultFromJSArray fnames |]) []
--
2.8.1
[PATCH stable-2.15 0/2] Make stable-2.15 distcheck-compliant
From: Iustin Pop This two patches (well, only the first, the second is cleanup) make stable-2.15 pass "make distcheck" on wheezy again. Iustin Pop (2): Fix line-too long errors Fix small typo in opcode tests lib/cmdlib/cluster/verify.py | 4 +++- src/Ganeti/THH.hs | 29 - test/hs/Test/Ganeti/OpCodes.hs | 2 +- 3 files changed, 20 insertions(+), 15 deletions(-) -- 2.8.1
Re: [MERGE] stable-2.17 to master
2016-06-10 16:25 GMT+02:00 'Brian Foley' via ganeti-devel < ganeti-devel@googlegroups.com>: > commit 7eb49311e18865db76c4e8da5eb4b2e166db2d55 > Merge: a32eb3c 17a1c27 > Author: Brian Foley > Date: Fri Jun 10 15:20:33 2016 +0100 > > Merge branch 'stable-2.17' > > * stable-2.17 > > > * stable-2.16 > Fix optimisation: Correctly extract secondary node > Tune getNodeInstances DRBD secondary computation > Get haskell daemons to only compress files > 4kB > Use zlib compression level 3 in Haskell RPC code > > * stable-2.15 > Fixup compatibility with GHC 7.4/base 4.5 > > Signed-off-by: Brian Foley > LGTM, thanks. iustin
Re: [MERGE] stable-2.16 to stable-2.17
2016-06-10 15:40 GMT+02:00 'Brian Foley' via ganeti-devel < ganeti-devel@googlegroups.com>: > commit b462d8c77bff0789e8a951288dea34226ab8b6d7 > Merge: 20c24a8 90281b4 > Author: Brian Foley > Date: Fri Jun 10 14:35:13 2016 +0100 > > Merge branch 'stable-2.16' into stable-2.17 > > * stable-2.16 > Fix optimisation: Correctly extract secondary node > Tune getNodeInstances DRBD secondary computation > Get haskell daemons to only compress files > 4kB > Use zlib compression level 3 in Haskell RPC code > > * stable-2.15 > Fixup compatibility with GHC 7.4/base 4.5 > > Manually resolve import conflicts. > > Signed-off-by: Brian Foley > LGTM, although I can't check the merge, but the diffs look sane. diff --cc src/Ganeti/Codec.hs > index 9a41499,404c70b..6f54c0d > --- a/src/Ganeti/Codec.hs > +++ b/src/Ganeti/Codec.hs > @@@ -37,21 -37,18 +37,23 @@@ module Ganeti.Code > , decompressZlib > ) where > > +import Prelude () > +import Ganeti.Prelude > Oh, this is interesting, I was wondering if there is such a module, and it seems there is :) iustin
Re: [MERGE] stable-2.15 to stable 2.16
2016-06-10 15:28 GMT+02:00 'Brian Foley' via ganeti-devel < ganeti-devel@googlegroups.com>: > commit 5785f214a9e728465a4bfc1aef7ded306225cfa2 > Merge: 40cd52f 2429235 > Author: Brian Foley > Date: Fri Jun 10 14:23:10 2016 +0100 > > Merge branch 'stable-2.15' into stable-2.16 > > * stable-2.15 > Fixup compatibility with GHC 7.4/base 4.5 > > Signed-off-by: Brian Foley > LGTM, thanks. iustin
[PATCH stable-2.15] Fixup compatibility with GHC 7.4/base 4.5
It looks like commit c429dd26 introduced the use of
atomicModifyIORef', which is only present in base 4.6 (GHC 7.6).
Let's fix that by importing the actual implementation of said function
from base 4.6 in case we're running with earlier versions.
Signed-off-by: Iustin Pop
---
src/Ganeti/Compat.hs | 14 ++
src/Ganeti/JQScheduler.hs | 3 ++-
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/Ganeti/Compat.hs b/src/Ganeti/Compat.hs
index e5276d1..8e002e7 100644
--- a/src/Ganeti/Compat.hs
+++ b/src/Ganeti/Compat.hs
@@ -41,10 +41,12 @@ module Ganeti.Compat
( rwhnf
, Control.Parallel.Strategies.parMap
, finiteBitSize
+ , atomicModifyIORef'
) where
import qualified Control.Parallel.Strategies
import qualified Data.Bits
+import qualified Data.IORef
-- | Wrapper over the function exported from
-- "Control.Parallel.Strategies".
@@ -67,3 +69,15 @@ finiteBitSize :: (Data.Bits.FiniteBits a) => a -> Int
finiteBitSize = Data.Bits.finiteBitSize
#endif
{-# INLINE finiteBitSize #-}
+
+-- FIXME: remove this when dropping support for GHC 7.4.
+atomicModifyIORef' :: Data.IORef.IORef a -> (a -> (a, b)) -> IO b
+#if MIN_VERSION_base(4,6,0)
+atomicModifyIORef' = Data.IORef.atomicModifyIORef'
+#else
+atomicModifyIORef' ref f = do
+b <- Data.IORef.atomicModifyIORef ref $ \a ->
+case f a of
+v@(a',_) -> a' `seq` v
+b `seq` return b
+#endif
diff --git a/src/Ganeti/JQScheduler.hs b/src/Ganeti/JQScheduler.hs
index 4f18f47..3a46934 100644
--- a/src/Ganeti/JQScheduler.hs
+++ b/src/Ganeti/JQScheduler.hs
@@ -56,7 +56,7 @@ import Control.Monad
import Control.Monad.IO.Class
import Data.Function (on)
import Data.Functor ((<$))
-import Data.IORef
+import Data.IORef (IORef, atomicModifyIORef, newIORef, readIORef)
import Data.List
import Data.Maybe
import qualified Data.Map as Map
@@ -66,6 +66,7 @@ import qualified Data.Set as S
import System.INotify
import Ganeti.BasicTypes
+import Ganeti.Compat
import Ganeti.Constants as C
import Ganeti.Errors
import Ganeti.JQScheduler.Filtering (applyingFilter, jobFiltering)
--
2.8.0.rc3.226.g39d4020
Re: [PATCH master] Fixup compatibility with GHC 7.4/base 4.5
On Fri, Jun 10, 2016 at 12:07:45PM +0100, Brian Foley wrote: > On Fri, Jun 10, 2016 at 12:33:23PM +0200, 'Iustin Pop' via ganeti-devel wrote: > > On Fri, Jun 10, 2016 at 12:30:03PM +0200, Iustin Pop wrote: > > > It looks like commit c429dd26 introduced the use of atomicModifyIORef', > > > which > > > is only present in base 4.6 (GHC 7.6). Let's temporarily fix that by > > > adding a > > > small compat layer (which undoes the optimisations of using strict on > > > 7.4, but > > > at least it works), until we decide to officially drop support for 7.4. > > > > Forgot to add that I manually tested this on both ghc 7.4 (wheezy) and > > 7.6.3. > > > > iustin > > Thanks. > > I also noticed this problem later on when reading some the docs, but didn't > get around to fixing it. Given the memory leak was pretty severe, could we > just > put the implementation of atomicModifyIORef' from base-4.6.0.0 into the #ifdef > rather than making it non-strict? > > https://hackage.haskell.org/package/base-4.6.0.0/docs/src/Data-IORef.html#line-132 Good point. I chose the lazier way as I was not sure we care about wheezy performance, but this is trivial enough indeed. > Also, could you make this patch against stable-2.15 please (given that's where > atomicModifyIORef' was introduced). I'll merge it forward to all the newer > branches. Sure thing! I looked at the commit and it was not included in any tag (git describe --contains), so I thought it was done on master. thanks, iustin
Re: [PATCH master] Fixup compatibility with GHC 7.4/base 4.5
On Fri, Jun 10, 2016 at 12:30:03PM +0200, Iustin Pop wrote: > It looks like commit c429dd26 introduced the use of atomicModifyIORef', which > is only present in base 4.6 (GHC 7.6). Let's temporarily fix that by adding a > small compat layer (which undoes the optimisations of using strict on 7.4, but > at least it works), until we decide to officially drop support for 7.4. Forgot to add that I manually tested this on both ghc 7.4 (wheezy) and 7.6.3. iustin
Re: [PATCH master 4/8] Split getInstance into *ByUuid and *ByPartialName
On Fri, Jun 10, 2016 at 11:28:16AM +0100, Brian Foley wrote: > On Thu, Jun 09, 2016 at 11:13:54PM +0200, Iustin Pop wrote: > > From: Iustin Pop > > > > This function (and getNode, next patch) were two pain points when I tried to > > convert UUIDs to another type, since they're called via a single type > > (currently String) that wants to dispatch to either UUID or name. In many > > cases > > we know exactly what we want to search for, so having only this function is > > overly generic. Not useful yet, but will be later for the string type > > conversion. > > > > Additionally change the name of the existing function getInstanceByName to > > match the new functions better. > > > > Signed-off-by: Iustin Pop > > It took me a little while to convince myself that some of the callers should > use getInstanceByExactName, but I think you're correct. Note that I don't change that fact; I just renamed getInstanceByName to getInstanceByExactName to make a clear distinction between PartialName and ExactName. But yes, it's a good point if they need to do that. thanks, iustin
[PATCH master] Fixup compatibility with GHC 7.4/base 4.5
It looks like commit c429dd26 introduced the use of atomicModifyIORef', which
is only present in base 4.6 (GHC 7.6). Let's temporarily fix that by adding a
small compat layer (which undoes the optimisations of using strict on 7.4, but
at least it works), until we decide to officially drop support for 7.4.
Signed-off-by: Iustin Pop
---
src/Ganeti/Compat.hs | 10 ++
src/Ganeti/JQScheduler.hs | 5 +++--
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/Ganeti/Compat.hs b/src/Ganeti/Compat.hs
index e5276d1..3ee3293 100644
--- a/src/Ganeti/Compat.hs
+++ b/src/Ganeti/Compat.hs
@@ -41,10 +41,12 @@ module Ganeti.Compat
( rwhnf
, Control.Parallel.Strategies.parMap
, finiteBitSize
+ , atomicModifyIORef'
) where
import qualified Control.Parallel.Strategies
import qualified Data.Bits
+import qualified Data.IORef
-- | Wrapper over the function exported from
-- "Control.Parallel.Strategies".
@@ -67,3 +69,11 @@ finiteBitSize :: (Data.Bits.FiniteBits a) => a -> Int
finiteBitSize = Data.Bits.finiteBitSize
#endif
{-# INLINE finiteBitSize #-}
+
+-- FIXME: remove this when dropping support for GHC 7.4.
+atomicModifyIORef' :: Data.IORef.IORef a -> (a -> (a, b)) -> IO b
+#if MIN_VERSION_base(4,6,0)
+atomicModifyIORef' = Data.IORef.atomicModifyIORef'
+#else
+atomicModifyIORef' = Data.IORef.atomicModifyIORef
+#endif
diff --git a/src/Ganeti/JQScheduler.hs b/src/Ganeti/JQScheduler.hs
index 2eae077..9d0f72d 100644
--- a/src/Ganeti/JQScheduler.hs
+++ b/src/Ganeti/JQScheduler.hs
@@ -64,7 +64,7 @@ import Control.Monad ( when
, forM_)
import Control.Monad.IO.Class
import Data.Function (on)
-import Data.IORef
+import Data.IORef (IORef, atomicModifyIORef, newIORef, readIORef)
import Data.List ( find
, deleteFirstsBy
, sortBy
@@ -79,6 +79,7 @@ import qualified Data.Set as S
import System.INotify
import Ganeti.BasicTypes
+import Ganeti.Compat
import Ganeti.Constants as C
import Ganeti.Errors
import Ganeti.JQScheduler.Filtering (applyingFilter, jobFiltering)
@@ -449,7 +450,7 @@ scheduleSomeJobs qstate = do
showQueue :: Queue -> String
showQueue (Queue {qEnqueued=waiting, qRunning=running}) =
let showids = show . map (fromJobId . qjId . jJob)
- in "Waiting jobs: " ++ showids waiting
+ in "Waiting jobs: " ++ showids waiting
++ "; running jobs: " ++ showids running
-- | Check if a job died, and clean up if so. Return True, if
--
2.8.0.rc3.226.g39d4020
Re: [PATCH master 3/3] Fix KVM pinning tests to not depend on the local machine
2016-06-10 10:58 GMT+02:00 Viktor Bachraty :
>
> On Thu, Jun 9, 2016 at 11:33 PM, Iustin Pop wrote:
>
>> From: Iustin Pop
>>
>> Commit 8b2ec2f added unittests for KVM pinning, but it introduced a
>> non-obvious
>> local dependency in the tests: the CPU_PINNING_OFF calls work by looking
>> at the
>> (current) machine's core count, and pinning to all those CPUs. In order
>> to make
>> this work independently from the test machine, we must also mock the
>> result of
>> process.cpu_count(). Do this by using a core count that is very much
>> unlikely
>> to ever be present in the real world.
>>
>> Signed-off-by: Iustin Pop
>> ---
>> test/py/ganeti.hypervisor.hv_kvm_unittest.py | 11 +++
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/test/py/ganeti.hypervisor.hv_kvm_unittest.py b/test/py/
>> ganeti.hypervisor.hv_kvm_unittest.py
>> index c7a53b5..55ffb9b 100755
>> --- a/test/py/ganeti.hypervisor.hv_kvm_unittest.py
>> +++ b/test/py/ganeti.hypervisor.hv_kvm_unittest.py
>> @@ -37,6 +37,7 @@ import socket
>> import os
>> import struct
>> import re
>> +from contextlib import nested
>>
>> from ganeti import serializer
>> from ganeti import constants
>> @@ -636,12 +637,13 @@ class TestKvmCpuPinning(testutils.GanetiTestCase):
>> cpu_mask = self.params['cpu_mask']
>> worker_cpu_mask = self.params['worker_cpu_mask']
>> hypervisor = hv_kvm.KVMHypervisor()
>> -with mock.patch('psutil.Process', return_value=mock_process):
>> +with nested(mock.patch('psutil.Process', return_value=mock_process),
>> +mock.patch('psutil.cpu_count', return_value=1237)):
>>
>
> 'nested' is deprecated in Python 2.7 but unfortunately 2.6 does not
> support comma separated contexts as 2.7 does.
>
Yep, saw that. However, nested still works (no warnings) in 2.6 as well, so
it should be good for now.
> All Debian versions newer than Squeeze should have 2.7 already. I believe
> once master goes stable, we can safely drop support for Python <2.7, what
> do you think?
>
It is possible, but if we want to do that, I think we should do it across
the board. E.g. on Haskell side we have a number of workaround for old
compilers/library versions, and we could do some cleanup if we remove that,
but we don't need to hurry.
So declaring that some version (e.g. 2.19, or 2.20 - that is a round
number) is only supported on (Jessie+, or 2015+, or something) would be a
good move.
thanks,
iustin
hypervisor._ExecuteCpuAffinity('test_instance', cpu_mask,
>> worker_cpu_mask)
>>
>> self.assertEqual(mock_process.set_cpu_affinity.call_count, 1)
>> self.assertEqual(mock_process.set_cpu_affinity.call_args_list[0],
>> - mock.call(range(0,12)))
>> + mock.call(range(0,1237)))
>>
>>def testCpuPinningPerVcpu(self):
>> mock_process = mock.MagicMock()
>> @@ -659,11 +661,12 @@ class TestKvmCpuPinning(testutils.GanetiTestCase):
>> def get_mock_process(unused_pid):
>>return mock_process
>>
>> -with mock.patch('psutil.Process', get_mock_process):
>> +with nested(mock.patch('psutil.Process', get_mock_process),
>> +mock.patch('psutil.cpu_count', return_value=1237)):
>>hypervisor._ExecuteCpuAffinity('test_instance', cpu_mask,
>> worker_cpu_mask)
>>self.assertEqual(mock_process.set_cpu_affinity.call_count, 7)
>>self.assertEqual(mock_process.set_cpu_affinity.call_args_list[0],
>> - mock.call(range(0,12)))
>> + mock.call(range(0,1237)))
>>self.assertEqual(mock_process.set_cpu_affinity.call_args_list[6],
>> mock.call([15, 16, 17]))
>>
>> --
>> 2.8.1
>>
>>
>
