| * mwhudson has lunch instead of worrying about this | 00:00 | |
| mwhudson | "Error while reading: error:140E0197:SSL routines:SSL_shutdown:shutdown while in init - trying cache." | 01:10 |
|---|---|---|
| jamesh | amurray: who usually reviews changes to the review-tools package? I filed https://code.launchpad.net/~jamesh/review-tools/+git/review-tools/+merge/403932 a few days ago, and it would be great to get some feedback. | 01:50 |
| amurray | jamesh: myself and emi would normally review these - it's already on my todo-list, I hope to get to it today | 01:52 |
| jamesh | amurray: the tests were a bit of a pain to update: many just assert on the number of warnings/errors, rather than what those warnings or errors are. That makes it difficult to tell exactly what they were trying to test. | 01:54 |
| amurray | yeah I am still trying to get my head around all of that - but I am super glad jamie did such a great job of setting those all up and adding so many test cases already as it makes me more confident when making changes to review-tools that I won't suddenly change it's behaviour unexpectedly | 01:57 |
| amurray | also I think the output from the tests gets diffed against known-good values so that should check that the output itself matches what we expect in terms of warnings/errors | 01:57 |
| jamesh | amurray: I suspect a lot of the logic could be simplified by running a schema validator (e.g. using the jsonschema package Snapcraft does) over the yaml before doing most of the checks | 01:58 |
| jamesh | amurray: if you already know that certain properties exist or have particular types, the later checks can be a lot more concise | 01:58 |
| amurray | hmm that is a great idea - thanks :) | 01:59 |
| jamesh | amurray: I think the right option would be to do some initial normalisation of the yaml (mainly fixing up plugs/slots), then run the schema validation on the result, then run all the other checks if that passes. | 02:11 |
| jamesh | a lot of the checks could probably be removed in favour of declarations in the schema too | 02:11 |
| jamesh | (there's obviously still going to be ones that need to be done in code though) | 02:12 |
| amurray | jamesh: do we have an existing snap.yaml schema? I guess I could use the snapcraft one as a head start... | 02:20 |
| jamesh | amurray: I don't think so. The snapcraft one could be used as a start, but you'd probably want something looking a bit different | 02:22 |
| jamesh | amurray: e.g. Snapcraft doesn't really care about interface properties, but review-tools does | 02:22 |
| amurray | yeah plus there is all the build recipe etc info in snapcraft.yaml that doesn't end up in snap.yaml that we can ignore | 02:23 |
| mborzecki | morning | 05:22 |
| mardy | mborzecki: hi! | 05:28 |
| mborzecki | mardy: heya | 06:02 |
| mardy | mborzecki: I'm trying to implement this suggestion: https://github.com/snapcore/snapd/pull/10266/files#r649324515 Do I understand it correctly, that the best practice is to immediately follow a Lock (or Unlock) invocation with a "defer Unlock (or Lock)"? | 06:13 |
| mborzecki | let me see | 06:13 |
| mborzecki | mardy: yes | 06:14 |
| mardy | even when there are several of them in the same method, right? | 06:14 |
| mborzecki | same usually goes for files, f, err := os.Open() followed by defer f.Close() in the err == nil path | 06:14 |
| mardy | I think it's a good practice to follow when there's just one operation to ve reverted, but in our case, when we have several Lock() and Unlock() invocations *on the same mutex*, this results in unnecessary locks being performed when we return from the function | 06:15 |
| mardy | I guess it's not a problem, but it appears suboptimal | 06:16 |
| mborzecki | mardy: hm that depends, though i agree it is more clear when there's just one operation, otherwise things get a little messy | 06:17 |
| mborzecki | mardy: there's some test code that locks and unlock the state lock a couple of times, if a panic appears in the unlocked section, and before that code there was lock && defer unlock, the tests will get stuck (or panic in teardown) | 06:18 |
| mborzecki | mardy: same for actual code, take a look at this fix that landed: https://github.com/snapcore/snapd/pull/10357 | 06:19 |
| mborzecki | there's a section where the state lock is selectively unlocked and then locked again, but at the beginning there's still the lock && defer unlock pattern | 06:20 |
| mborzecki | mardy: hm pedronis even mentioned that pattern here: https://github.com/snapcore/snapd/pull/10266/files#r649325941 | 06:21 |
| mardy | mborzecki: when does a go function return? Is it only when you call "return", or can it also exit its scope because of a runtime error ("exception", in C++ terms)? | 06:22 |
| mborzecki | mardy: in go that's a panic | 06:22 |
| mborzecki | mardy: this has some nice coverage: https://blog.golang.org/defer-panic-and-recover | 06:24 |
| zyga-mbp | o/ | 06:25 |
| mardy | thanks, this clarifies quite a bit. The last remaining question is whether in the tests we should clear up the lock states when the test fails in an Assert or Check. I guess we shouldn't. | 06:36 |
| mborzecki | mardy: what do you mean by clearing up the lock? | 06:40 |
| mardy | I guess the question is becoming philosophical, but let's consider https://github.com/snapcore/snapd/blob/master/overlord/servicestate/service_control_test.go#L457-L472 | 06:40 |
| mborzecki | glad i got the coffe ;) right in time for philisophical questions | 06:41 |
| mardy | I see three options: 1) a defer Unlock() just after the first Lock() - 2) a defer Unlock after the *second* Lock() - 3) a direct call to Unlock() at the end | 06:41 |
| mborzecki | mardy: hmm that bit looks weird, i'd spect defet st.Unlock() in line 460 | 06:41 |
| mborzecki | s/spect/expect/ | 06:42 |
| mardy | that's in line with what Samuele and Ian suggested :-) It's not super clear to me why, though. I would probably add if after the second Lock, if it was just me | 06:43 |
| mardy | or maybe even do the Unlock explicitly, since this is a test and it must succeed. Anyway, I'll follow your advice and add it at the beginning :-) | 06:45 |
| zyga-mbp | mardy without looking at code, in tests I preferred to write code that panics in the error path rather than hang | 06:45 |
| zyga-mbp | morning mvo | 06:46 |
| mborzecki | mardy: hm this is what i think the test should look like: https://play.golang.org/p/ifD-ulGXVJh | 06:46 |
| mborzecki | mvo: hey | 06:46 |
| mardy | zyga-mbp: yes, that's a given :-) | 06:47 |
| zyga-mbp | mborzecki yeah, looks right | 06:47 |
| mardy | mborzecki: ok | 06:47 |
| mborzecki | mardy: you can try and fix it this way and see what happens, it shouldn't hang ;) | 06:48 |
| mborzecki | mardy: and that Assert() between unlock and lock would cause a panic if settle fails, making the test kind of useless | 06:48 |
| mardy | mborzecki: they never hang, even now. Maybe because the state is getting destroyed, I dunno | 06:48 |
| mborzecki | mardy: ah right, there's no tear down, so the test just finishes and next one start with a clean slate again | 06:49 |
| mardy | mborzecki: adding an EnsureUnlocked() method in the state, which calls Unlock only if the state is locked, could it make sense? Then people would always call it deferred, and could have as many Lock() and Unlock() calls in their method as they like | 06:52 |
| mardy | (for the matter in question your solution works, I'm just thinking in the general case now) | 06:53 |
| mborzecki | mardy: not sure there's a way to interrogate the mutex to check whether it's locked | 06:56 |
| mvo | good morning zyga, mborzecki and mardy | 06:56 |
| mborzecki | mardy: unless you add auxiliary variable, or maybe it's already in the state.Lock()/Unlock()? | 06:57 |
| mardy | mvo: hi! | 07:08 |
| mardy | mborzecki: right, it looks like there isn't :-) Or maybe we could add a method like RunInLockedState(func()), that does { Lock(); defer Unlock(); func() } and then we can call this as many times as we want from inside the same method. But maybe I'm overdoing it... | 07:11 |
| mborzecki | mardy: i'd try to keep it simple ;) | 07:24 |
| mborzecki | mvo: can you take a look at https://github.com/snapcore/snapd/pull/10377 ? | 07:24 |
| mvo | mborzecki: sure | 07:26 |
| mborzecki | thanks! | 07:26 |
| zyga-mbp | hey pedronis | 07:28 |
| mardy | mborzecki: then this should be a quick one: https://github.com/snapcore/snapd/pull/10383 | 07:35 |
| pedronis | mborzecki: hi, I just noticed we have something called bootStateUpdate20.resealForModel, you might have to check whether it still makes sense as is | 07:43 |
| pedronis | with the changes you need to make | 07:43 |
| mborzecki | pedronis: yeah, looked at it yesterday and we'll likely be able to simplify/drop it | 07:46 |
| mborzecki | pedronis: also as you mentione secboot takes SnapModel interface, now we have all the fields, so maybe we can provide some internal wrapper that uses modeenv as an input | 07:46 |
| pedronis | mborzecki: yes, our secboot will need some tweaks | 07:48 |
| pedronis | mborzecki: I re-reviewed https://github.com/snapcore/snapd/pull/10377, one comment there | 07:49 |
| mborzecki | pedronis: thanks | 07:49 |
| mborzecki | hmm i'm sill puzzled why people insist on using foobar2000 via wine on linux | 08:26 |
| zyga-mbp | mborzecki really beats the llamas ass? | 08:29 |
| mborzecki | hahah | 08:42 |
| mborzecki | didn't they resurrect winamp bw? | 08:42 |
| zyga-mbp | yeah I think they did | 08:45 |
| pedronis | mborzecki: I reviewed https://github.com/snapcore/snapd/pull/10361 , mostly comments about the tests | 09:11 |
| mborzecki | pedronis: thank you! | 09:12 |
| pedronis | https://github.com/snapcore/snapd/pull/10372 needs 2nd review, it's test only but is a bit repetitive | 10:00 |
| mborzecki | mvo: can you land https://github.com/snapcore/snapd/pull/10377 ? | 10:57 |
| mvo | mborzecki: sure, done | 11:03 |
| mborzecki | mvo: thanks! | 11:03 |
| mardy | pedronis: can you please alaborate a bit more your on comment at https://github.com/snapcore/snapd/pull/10375? What is the "snap-id"? | 11:03 |
| pedronis | mardy: each snap as a unique indentifier associated in the store with it, assertion for example whose that as primery key when they need to refer to snap | 11:05 |
| pedronis | mardy: you can see it in snap info for example | 11:05 |
| mort | continuation of discussion in #ubuntu | 11:13 |
| ogra | mort, so snap uploads are binary ... a snap is a gpg signed squashfs file ... (the gpg signing happens on the server, what you upload is just a squashfs, called .snap and using teh correct metadata structure) | 11:14 |
| mardy | pedronis: OK. But how can snapd know that the snap is authorized to declare this user? Do we need to hardcode a list of snap ids (well, I guess it will be just one for now) into snapd? | 11:15 |
| mort | if I made a classic snap of rakudobrew, and inserted some code in a binary which checks the date, and if it's two months after the date I uploaded it, it does something nefarious (stealing bitcoin wallets, installing keyloggers, installing botnet client software, whatever) | 11:16 |
| mort | how would that be caught in manual review | 11:16 |
| ogra | first of al you could not just upload it | 11:18 |
| ogra | you'd have to submit it for review in the forum and proof that it falls into the set of apps that get allowed classic at all | 11:18 |
| ogra | then yu would need to be vetted as a publisher | 11:18 |
| ogra | classic has quite a lot of hurdles to overcome before you can evn upload | 11:19 |
| zyga-mbp | hey ogra :) | 11:19 |
| ogra | mort, https://forum.snapcraft.io/t/process-for-reviewing-classic-confinement-snaps/1460 | 11:19 |
| mort | I see, so I would need to be a relatively trusted snap "crafter" with a history of trustworthy snaps | 11:19 |
| ogra | hey zyga-mbp | 11:19 |
| zyga-mbp | mort or a well know upstream, perhaps | 11:19 |
| zyga-mbp | it's not a perfect system | 11:20 |
| zyga-mbp | it's mostly based on trust | 11:20 |
| ogra | mort, and your app must fall into the specific set of categories ... not every snap can become classic ... | 11:20 |
| ogra | only if you can prove that strict can not work for your use case you get it granted | 11:20 |
| mort | ogra: yeah, the hypothetical target here is rakudobrew which is a command-line app to install the raku language to your system | 11:20 |
| ogra | loo at "supported" and "unsupported" on the above forum post | 11:20 |
| ogra | "3rd party installer snaps " | 11:21 |
| zyga-mbp | seems like rustup | 11:21 |
| pedronis | mardy: we have to untilt that list can be managed by the store some other way | 11:21 |
| ogra | explicitly unsupported | 11:21 |
| zyga-mbp | well, except rustup is a classic snap | 11:22 |
| mort | ogra: huh, why is rustup allowed then? | 11:22 |
| zyga-mbp | I recall the snapcraft summit when rustup was created | 11:22 |
| zyga-mbp | perhaps that's why? | 11:22 |
| zyga-mbp | but it seems to be maintained | 11:22 |
| ogra | because it comes from a well known upstream i guess | 11:22 |
| pedronis | it says it is experimental, I don't know what process it went through | 11:23 |
| zyga-mbp | pedronis it was made in montreal during the sprint with upstream being on site | 11:23 |
| mort | it also says it's unofficial, which is the most interesting part because I would understand it if it was an official snap from the rust project | 11:24 |
| zyga-mbp | IIRC jdstrand was there but perhaps my memory is rusty | 11:24 |
| pedronis | but is not owned by upstream, it seems to be owned by a single person | 11:24 |
| zyga-mbp | owner seems to lead the rustup working group | 11:25 |
| * ogra notes that this single person wrote half of launchpad 😉 | 11:25 | |
| zyga-mbp | so semi official perhaps | 11:25 |
| mort | it's probably fine, but it just got me wondering when I tried to `apt install rustup`, apt recommended that I `snap install rustup`, and rustup is a classic confinement unofficial experimental snap | 11:25 |
| zyga-mbp | but I only mentioned that because it seems relevant to this case | 11:25 |
| mort | from a random person I don't know | 11:25 |
| ogra | yeah, rustup might be special given it comes from an ex canonical amployee we all trust ... | 11:25 |
| ogra | ... it does not really fall under the classic specification though | 11:25 |
| pedronis | I'll have to dig because I think we should push a bit to have it owned by an actual upstream at least | 11:26 |
| ogra | 👍 | 11:26 |
| pedronis | pstolowski: I made a remark in https://github.com/snapcore/snapd/pull/10385 I don't have time myself right now, but maybe you can start thinking about it | 11:31 |
| pstolowski | pedronis: thanks, i'm not sure i understand the concern, maybe we need to chat whenever you have a moment | 11:35 |
| pedronis | pstolowski: let's say a snap holds another for 48h, at the end of that 48h hours this code might reset hold before we call the hook, now that snap can simply hold the snap again for 48h, because we lost firstHeld info, no? | 11:37 |
| pedronis | basically I'm not sure we should do this in this form, but need to think more | 11:38 |
| pstolowski | pedronis: ah, because it also prunes based on time, not just refresh candidates, got it | 11:38 |
| mardy | pedronis: do you think that it's something that the store can easily do, or should we hardcode the microk8s snap-id in snapd for the time being, to unblock them? | 11:50 |
| pedronis | mardy: it needs a full design to do that in the store and time from them | 11:51 |
| pedronis | pstolowski: https://github.com/snapcore/snapd/pull/10369 looks good but I'm confused by the tests | 11:51 |
| zyga | o/ | 11:55 |
| zyga | I've patched spread to have an export feature | 11:55 |
| zyga | spread -export "format" | 11:55 |
| zyga | spread -list is equivalent to -export=list | 11:55 |
| zyga | I'll add two export formats as well: shellcheck-friendly bash script tree | 11:56 |
| zyga | and lava :) | 11:56 |
| zyga | this should help making sure shellcheck is clean without having to re-implement any spread logic in python | 11:56 |
| zyga | I'll happily share that when it's ready | 11:56 |
| zyga | suggestions and ideas welcome | 11:56 |
| zyga | CC: mborzecki ^ (since you maintained the checker) | 11:56 |
| mardy | pedronis: then would you complain if we temporarily do the check in snapd? | 11:58 |
| pedronis | mardy: I don't understand your question, isn't that what I proposed? | 11:59 |
| pstolowski | pedronis: yes it's slightly confusing, i'm making it more explicit. basically i'm not mocking time now so if undo wasn't implemented, last refresh would be set to real current time and fail | 12:01 |
| pstolowski | pedronis: updated | 12:08 |
| pedronis | pstolowski: reviewed also https://github.com/snapcore/snapd/pull/10362 | 12:10 |
| mardy | pedronis: yes, but then here in the chat when I mentioned hardcoding I got the feeling you were insisting for this check to be done in the store only. Looks like I misunderstood, then. I'll implement the check in snapd :-) | 12:11 |
| pstolowski | pedronis: thanks! | 12:13 |
| pstolowski | is check-rerefresh a good place to do something after refreshing all snaps during auto-refresh? (thinking about pruneGating there) | 12:17 |
| pedronis | pstolowski: we need to chat a bit, is not super clear to me why we need pruneGating as is | 12:18 |
| pedronis | pstolowski: I re-reviewed https://github.com/snapcore/snapd/pull/10369 , I think I was confused also because I didn't read the test name and expected it to be a different test, which is actually missing | 12:19 |
| pedronis | pstolowski: won't https://github.com/snapcore/snapd/pull/10384 + a pruneGating that deals only with affecting and nothing time based, work ? | 12:21 |
| pstolowski | pedronis: you may be right, a snap is either eventually refreshed or disappears from candidates | 12:25 |
| pstolowski | mborzecki: maybe you could take a look at https://github.com/snapcore/snapd/pull/10358 ? | 12:35 |
| mborzecki | looking | 12:45 |
| mardy | ijohnson[m]: I see an Unlock() here, but I cannot find the corresponding Lock(): https://github.com/snapcore/snapd/blob/master/overlord/servicestate/quota_handlers.go#L183 | 13:48 |
| mardy | it looks like this method assumes to be invoked with the state locked | 13:49 |
| ijohnson[m] | mardy yes that function assumes it is called with state locked | 13:54 |
| ijohnson[m] | It should probably mention that in the doc-comment | 13:54 |
| ijohnson[m] | cachio_: hey it seems we are running out of space on some of the workers, can you take a look? https://github.com/snapcore/snapd/runs/2802991450 | 14:28 |
| cachio_ | ijohnson[m], I'll clean up | 14:29 |
| cachio_ | thank | 14:29 |
| ijohnson[m] | thanks | 14:29 |
| cachio_ | I'll need to add more disk soon | 14:29 |
| pedronis | jamesh: hi, I opened this https://github.com/snapcore/snapd/pull/10388 (something I mentioned before) | 14:32 |
| cachio_ | ijohnson[m], I have a job to cleanup montly but I'll run it every 2 weeks | 14:32 |
| ijohnson[m] | cachio_: nice sounds good | 14:33 |
| cachio_ | ijohnson[m], I see the problem, the cleanup is not reducing too much the free space | 14:35 |
| cachio_ | I need to dig a bit more | 14:35 |
| === pedronis_ is now known as pedronis | ||
| pedronis | https://github.com/snapcore/snapd/pull/9292 needs 2nd reviews | 15:16 |
| mvo | pedronis: looking | 15:40 |
| pedronis | mvo: sorry I had to re-open https://github.com/snapcore/snapd/pull/10390 , it was actually broken because assertstate_test.go was confused the tool | 15:41 |
| mvo | pedronis: oh, ok. sorry, I must have missed this | 15:41 |
| pedronis | mvo: the test would not have passed, anyway now there are some manual changes in it | 15:42 |
| pedronis | assertstate_test.go was importing go-check in two different ways ?? | 15:42 |
| mvo | pedronis: woah, amazing that this even works | 15:43 |
| mvo | pedronis: your changes still look fine fwiw | 15:43 |
| pedronis | thx | 15:43 |
| mvo | thank *you* | 15:43 |
| * cachio_ lunch | 15:48 | |
| ijohnson[m] | mvo: I forgot to do it yesterday, but I just filed the refresh certificate bug that I talked about yesterday in the SU https://bugs.launchpad.net/snapd/+bug/1931724 | 15:48 |
| ijohnson[m] | mvo: pedronis: can one of you land https://github.com/snapcore/snapd/pull/10347? the only failure is debian interfaces-many | 15:49 |
| pedronis | ijohnson[m]: done | 15:51 |
| ijohnson[m] | \o/ yay thank you | 15:51 |
| pedronis | https://github.com/snapcore/snapd/pull/10390 is simple | 15:52 |
| ijohnson[m] | pedronis: +1 | 16:00 |
| ijohnson[m] | pedronis: oops forgot to squash merge that, oh well we don't need to cherry-pick it anyways was just hoping to simplify the git history | 16:02 |
| pedronis | our squash-merge label is hit and miss | 16:03 |
| ijohnson[m] | yeah, some teams use bots to do the actual merge to prevent this sort of situation, but then you need to give bots permissions | 16:03 |
| pedronis | partly is also that we need to force merge, that's why I prefer when possible if it's the submitter to merge, they should have the most context in theory | 16:05 |
| pedronis | but given our flakiness that's also hard | 16:06 |
| ijohnson[m] | yeah submitters being able to merge is most ideal | 16:06 |
| === E_Eickmeyer is now known as Eickmeyer | ||
| * cachio_ afk | 18:47 | |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!