| === tds1 is now known as tds | ||
| === vrubiolo1 is now known as vrubiolo | ||
| === vrubiolo1 is now known as vrubiolo | ||
| Odd_Bloke | Heads up: GitHub is having availability issues: https://www.githubstatus.com/ | 15:47 |
|---|---|---|
| Odd_Bloke | powersj: blackboxsw: So do we have a release process that I should be following? (I don't _think_ I've cut an upstream release before.) | 17:55 |
| blackboxsw | Odd_Bloke: I added you to the trello board | 17:55 |
| blackboxsw | https://trello.com/b/obFWGwZY/upstream-cloud-init-201 | 17:56 |
| blackboxsw | I copied it yesterday from the template board https://trello.com/b/QQYFXpsA/template-sru-cloud-init-xy and deleted any "SRU" cards | 17:56 |
| Odd_Bloke | Status update on 20.1: we've tested the change that we were worried about and it does not appear to be causing issues | 17:56 |
| Odd_Bloke | blackboxsw: Cool, thanks! | 17:56 |
| Odd_Bloke | rharper: Where are we on the IMDS change you wanted to land for 20.1? | 17:56 |
| rharper | well, it's not a one liner =/ | 17:57 |
| rharper | writing a unittest (augmenting one to ensure we do the redaction) | 17:57 |
| blackboxsw | Odd_Bloke: and the SRU process for that trello board template came originally from https://github.com/CanonicalLtd/uss-tableflip/blob/master/doc/upstream_release_process.md | 17:57 |
| Odd_Bloke | rharper: OK, but in your view it's close enough that it's worth waiting for? | 17:57 |
| rharper | it should be; but since it's not a one liner; I don't suppose we should wait | 17:58 |
| Odd_Bloke | rharper: I'm going to go grab lunch, so you have a bit of time to get something up for review. :) | 18:08 |
| rharper | Odd_Bloke: https://github.com/canonical/cloud-init/pull/219 | 19:15 |
| blackboxsw | minor volley back rharper | 19:21 |
| blackboxsw | I might have to re-review the token interaction again. jussec | 19:33 |
| blackboxsw | yeah X-aws-ec2-metadata-token-ttl-seconds header is just a seconds value, non-secret. so no need to redact | 19:36 |
| rharper | blackboxsw: true but we don't need the value logged do we? | 19:37 |
| rharper | I've replied; having the value won't aide in debugging; and if we lower it, we just end up obtaining a new token more frequently; | 19:40 |
| blackboxsw | I guess we would if at some point we reduce the hardcoded AWS_TOKEN_TTL_SECONDS which afair I thought we would intend at at some point. | 19:41 |
| blackboxsw | fair rharper I concede it's not a big deal, just recognizing that we are redacting things that aren't necessarily sensitive, by that measure where do we draw the line. | 19:42 |
| blackboxsw | I have a couple more nits. we're doing a lot more work in that function than we need with every retry. | 19:57 |
| blackboxsw | will finish my review in ~30. have to afk for a bit | 19:58 |
| Odd_Bloke | rharper: blackboxsw: OK, that lunch break ended up longer than anticipated; I see that PR is Approved so I'll wait for it before cutting the release. | 20:11 |
| blackboxsw | back as well now. a minor set of review comments to followup with | 20:11 |
| blackboxsw | throwing together a paste to see what you guys think | 20:12 |
| rharper | blackboxsw: on my errand I was thinking I'd like to see if I can just copy the redacted header value instead of all headers; | 20:28 |
| blackboxsw | Odd_Bloke: rharper here's what I was thinking https://paste.ubuntu.com/p/NXXXyMZKFH/ . I think we should be hoisting the headers_cb calculation out of the retry for loop as those headers should not change across calls to the same url. | 20:30 |
| rharper | blackboxsw: well, that's a separate fixup no ? | 20:30 |
| rharper | blackboxsw: an alternative approach I thought about was having the headers_cb return the headers and a list of keys to be redacted | 20:31 |
| blackboxsw | as it is currently in the function, we are re-calculating and copying the dict of headers on each run. with rharper's change it gets more costly as we are now extracting each header on each retry and comparing it to the redacted list on every iteration of the liip | 20:31 |
| blackboxsw | loop | 20:31 |
| rharper | and I guess we don't pass back to headers_cb that we're retrying it just gets called a second time | 20:31 |
| blackboxsw | rharper: that'd work, but it's also doing that work on every single retry against a url. | 20:31 |
| rharper | so even if headers_cb wanted to vary based on retry it doesn't know | 20:32 |
| blackboxsw | right, it just gets called again and again | 20:32 |
| blackboxsw | soliciting the same response | 20:32 |
| rharper | well, I like you suggestion but it seems like an optimization | 20:32 |
| blackboxsw | rharper: it does seem like an optimization. | 20:32 |
| rharper | and if I push this copy down to only if the key is in redacted list; we're copying a very small amount of data | 20:32 |
| blackboxsw | rharper: well yes, we are copying a small amount and settting up a pointer to every key in req_args. | 20:33 |
| rharper | blackboxsw: for now, the only time that it's hitting is if we do a retry anyhow; the happy path never does it more than once; but I generally like it; | 20:36 |
| blackboxsw | right, I think the optimization can wait. I'll add the paste there for reference in case we want to go that route in the future. | 20:36 |
| Odd_Bloke | I'm not really following here, but dict operations are generally _very_ optimised in Python (because it's dicts all the way down, internally :p) so I would be surprised if this is a substantial slowdown. Which is to say: I think we can land as-is and then follow with the optimisation. | 20:38 |
| rharper | Odd_Bloke: I'm pushing up two changes in just a second, one blackboxsw wanted the string moved to module variable; and the other is to push the copy down to the matched keys rather than copying all headers even if we don't have anything to redact | 20:39 |
| blackboxsw | yep I've pasted that comment to the PR and set approved from my end | 20:40 |
| Odd_Bloke | rharper: OK, nice. | 20:41 |
| powersj | Odd_Bloke, how's 20.1? | 20:56 |
| Odd_Bloke | powersj: We're landing this IMDS change, then cutting it. | 20:59 |
| Odd_Bloke | blackboxsw: Your Approved is from before rharper's latest push; are you still +1? | 21:00 |
| blackboxsw | I'm still +1 Odd_Bloke | 21:00 |
| blackboxsw | yeah looks good | 21:00 |
| Odd_Bloke | Cool, merged. | 21:01 |
| blackboxsw | heh ohh well. | 21:01 |
| blackboxsw | we didn't really need to deepcopy the entry and then reset the value to REDACTED | 21:02 |
| blackboxsw | Odd_Bloke: rharper not needed for 20.1, but here's a low-hanging-fruit review. sorry I just missed that | 21:05 |
| blackboxsw | https://github.com/canonical/cloud-init/pull/220 | 21:05 |
| blackboxsw | hrm strike that. | 21:06 |
| blackboxsw | shame on me | 21:06 |
| rharper | hehe | 21:06 |
| blackboxsw | yes we did, otherwise we'd overwrite the actual headers | 21:06 |
| rharper | it would have had this done a lot faster if I knew I was modifying my req_args | 21:06 |
| rharper | and then we never pass tests... and the execption callback runs *forever* | 21:06 |
| rharper | it's inifinite | 21:06 |
| rharper | so I sat there... for some tim e | 21:06 |
| blackboxsw | heh | 21:06 |
| rharper | before realizing what was happening | 21:06 |
| blackboxsw | yeah I was wondering the same for me. oopsie | 21:06 |
| blackboxsw | closing that PR | 21:07 |
| rharper | even after I realized that headers were a key *in* all of the args | 21:07 |
| Odd_Bloke | powersj: rharper: blackboxsw: So I just opened up the card for 20.1 (I used shortcuts to assign and move it earlier), and it calls for us to land https://github.com/canonical/cloud-init/pull/54 before cutting. | 21:13 |
| Odd_Bloke | What do we think? | 21:13 |
| powersj | that needs to come after | 21:14 |
| powersj | I don't want to land that right before focal goes out | 21:14 |
| Odd_Bloke | After what? | 21:14 |
| rharper | well | 21:14 |
| rharper | it's non master | 21:14 |
| rharper | it's on the xenial branch only | 21:14 |
| powersj | oh right | 21:14 |
| blackboxsw | rharper: hrm. gonna need a second glance at that..... we are in the middle of the for loop where we are setting filtered_reg_args[k] = value... if we aren't copying over filtered_req_args['headers'] = req_args[k], then we don't have to deepcopy I don't think. Again, doesn't matter for 20.1, but I do think again we can drop that | 21:14 |
| powersj | in which case, not an issue for 20.1, it is for the SRU | 21:14 |
| Odd_Bloke | OK, so that's an SRU thing, cool. | 21:15 |
| rharper | blackboxsw: I was thinking that was correct; it's just a single dict entry; it *could* have a value of a dict; but I think we can reason that header values cannot be dict | 21:15 |
| rharper | blackboxsw: do we know if we pay a heavier cost calling deepcopy() when we don't need it with such a small dict ? | 21:16 |
| * rharper smells a timeit coming on | 21:16 | |
| blackboxsw | rharper: ahh your branch didn't have my local changes. I did the if k != 'headers': filtered_req_args[k] = v else: filtered_req_args[k] = REDACTED | 21:16 |
| blackboxsw | I wasn't setting filtered_req_args[k] = v if we were dealing with a 'headers' key | 21:17 |
| blackboxsw | rharper: +1 on headers values also being a dict and being concerned about not blowing that away | 21:17 |
| blackboxsw | but, I think our logic initially path setting that filtered_req_args[k] = v and then dropping into if k == 'headers' to redact is what causes the need for the copy. | 21:18 |
| blackboxsw | if we didn't set that in the first place, we wouldn't have to copy and overwrite | 21:18 |
| blackboxsw | anyway, not for 20.1 but I'll reopen the appropriate cleanup for us to look at for later | 21:19 |
| blackboxsw | and yes might as well add a timeit to the PR once up :) | 21:22 |
| rharper | blackboxsw: ok, I can see a way without copy | 21:25 |
| rharper | https://paste.ubuntu.com/p/2XQF6CPntK/ | 21:25 |
| rharper | however, I wonder if it's really faster than copying the headers dict | 21:26 |
| blackboxsw | https://github.com/canonical/cloud-init/pull/220 | 21:26 |
| rharper | for small sized dict; | 21:26 |
| blackboxsw | https://github.com/canonical/cloud-init/pull/221 | 21:27 |
| blackboxsw | yeah not much performance improvement I don't think | 21:27 |
| blackboxsw | yeah and now we've spent more time on this than we'll ever likely get back in performance runs ;) | 21:28 |
| blackboxsw | sorry about the rathole there | 21:28 |
| blackboxsw | especially given that our only case of redacting is a simple string value and not a large dict of values | 21:29 |
| johnsonshi | According to https://cloudinit.readthedocs.io/en/latest/topics/modules.html#mounts , "Swap files can be configured by setting the path to the swap file to create with filename". The docs imply that cloud-init creates a swap file at the specified filename if using the swap module. | 22:19 |
| Odd_Bloke | blackboxsw: So I'm looking at the release process, and it assumes that I can push the commit I've created locally to master. We, of course, can't do that with branch protection. We can either revise the docs to document disabling branch protection temporarily (probably not?), or we can update things so that the tagging of the release happens _after_ merge (via GH button) into master. However, I think | 22:20 |
| Odd_Bloke | that will cause CI to fail, because it will look for a 20.1 tag (to determine versions) and fail. | 22:20 |
| johnsonshi | My swap settings contain: | 22:20 |
| johnsonshi | ... | 22:20 |
| johnsonshi | swap: | 22:20 |
| johnsonshi | filename: /mnt/resource/swapfile | 22:20 |
| johnsonshi | ... | 22:20 |
| johnsonshi | ... | 22:20 |
| johnsonshi | When the machine is provisioned, cloud-init fails to create the swapfile. The cloud-init.log file shows that the "swapon -a" command failed. When I ssh into the machine and try running "swapon -a" myself, it says that "swapon: stat failed /mnt/resource/swapfile: No such file or directory". | 22:20 |
| johnsonshi | However, I can confirm that the /mnt/resource path exists and is accessible, and I am able to read/write files within /mnt/resource. I can also see that cloud-init added the swap entry to /etc/fstab. | 22:20 |
| johnsonshi | When I look through the cloud-init code, I can see cloud-init adds the swap entry to /etc/fstab if it exists, then runs "swapon -a" without creating the swapfile. | 22:20 |
| johnsonshi | Is cloud-init supposed to create the swapfile by if it is instructed to do so (the docs seem to imply that)? | 22:20 |
| Odd_Bloke | johnsonshi: I believe you need to specify "size" for the creation to occur. | 22:22 |
| blackboxsw | Odd_Bloke: doc reference? | 22:22 |
| Odd_Bloke | blackboxsw: https://github.com/CanonicalLtd/uss-tableflip/blob/master/doc/upstream_release_process.md | 22:22 |
| blackboxsw | thnaks | 22:22 |
| Odd_Bloke | blackboxsw: Specifically "Note: you need to push the tag or c-i will fail in check version." | 22:23 |
| blackboxsw | Odd_Bloke: we have the ability to git push tags | 22:23 |
| blackboxsw | right? | 22:23 |
| Odd_Bloke | blackboxsw: But we don't have the commit to tag until after merge. | 22:23 |
| Odd_Bloke | It never exists on my system until then. | 22:23 |
| Odd_Bloke | Which means the 20.1 tag I have pushed (until I realised the conflict) points at the commit which will be squashed into master (and then discarded, of course). | 22:24 |
| blackboxsw | ahh right, but we will merge you MP into master once you've bumped version to 20.1 right | 22:24 |
| Odd_Bloke | Right. | 22:24 |
| johnsonshi | swap: | 22:25 |
| johnsonshi | filename: /mnt/resource/swapfile | 22:25 |
| johnsonshi | size: 2147483648 | 22:25 |
| johnsonshi | I tried it again and the swapfile still isn't being created. | 22:25 |
| Odd_Bloke | johnsonshi: Could you file a bug with `cloud-init collect-logs` attached to it and drop the link to it back in here? | 22:25 |
| johnsonshi | cc_mounts.py does not seem to have code that creates a swapfile? | 22:25 |
| blackboxsw | Odd_Bloke: ok, hrm, but your branch did pass ci https://github.com/canonical/cloud-init/pull/222 | 22:25 |
| Odd_Bloke | blackboxsw: Right, I pushed the tag. | 22:25 |
| * blackboxsw checks it out again | 22:25 | |
| Odd_Bloke | As instructed. | 22:25 |
| blackboxsw | with branch restrictions reduced? | 22:26 |
| Odd_Bloke | But that tag points at a commit which will never be in master. | 22:26 |
| blackboxsw | or tag push worked | 22:26 |
| Odd_Bloke | Tag push is fine. | 22:26 |
| blackboxsw | ahh ok. gotcha | 22:26 |
| johnsonshi | Odd_Bloke: Sure thing. So missing swapfile creation is a bug due to the code not being in cloud-init? | 22:26 |
| blackboxsw | not sure if that's a broken process that should be fixes. stale tags pointing to nowhere for previous releases | 22:26 |
| Odd_Bloke | johnsonshi: What version of cloud-init are you using? | 22:26 |
| johnsonshi | Odd_Bloke: The distro I am using it with (RHEL 7.7) comes with cloud-init 18.5. | 22:27 |
| Odd_Bloke | Yeah, that should definitely have swap support, I don't know why you wouldn't be seeing that in cc_mounts. | 22:28 |
| Odd_Bloke | There have been changes since 18.4, however, so you might be hitting an existing bug. | 22:28 |
| Odd_Bloke | *18.5 | 22:28 |
| Odd_Bloke | johnsonshi: Are you able to try again with 19.4 somehow? | 22:28 |
| Odd_Bloke | blackboxsw: It looks like I fixed up the 19.4 tag on the day it happened. | 22:30 |
| Odd_Bloke | blackboxsw: (And 19.3 predated the GH move, I think?) | 22:30 |
| blackboxsw | johnsonshi: any of the copr dev builds work for you? https://copr.fedorainfracloud.org/coprs/g/cloud-init/cloud-init-dev/ | 22:30 |
| blackboxsw | Odd_Bloke: you are right . ok so, branch restrictions causing some issues then with upstream releases. | 22:31 |
| johnsonshi | Odd_Bloke: wait a sec | 22:31 |
| johnsonshi | Yeah I'll try out 19.4 | 22:31 |
| Odd_Bloke | Thanks! | 22:31 |
| Odd_Bloke | johnsonshi: Actually, if you're able, there have been further changes since 19.4, so a test of a daily build would be best. (That cloud-init-dev link has those, so you're already doing that if that's how you're going to do it. :) | 22:32 |
| johnsonshi | Thanks! | 22:33 |
| Odd_Bloke | blackboxsw: I'm thinking that retagging after is probably easier in the short term. | 22:33 |
| Odd_Bloke | But I think I'm going to remove the tag and re-run CI, to see if we actually need it before at all. | 22:34 |
| Odd_Bloke | Just in case that the move to Travis has happened to obviate that need. | 22:34 |
| blackboxsw | +1 Odd_Bloke | 22:35 |
| rharper | johnsonshi: from I can tell, the additional paths to your file are the issue; a work around would be to either use file in an existing directory; or use bootcmd to run an mkdir -p | 22:36 |
| rharper | and the fix is for cloud-init to use util.ensure_file() on the swapfile path | 22:36 |
| Odd_Bloke | powersj: FYI, we've run into some release tooling issues with the migration to GitHub. I'm currently pursuing getting it right this time, so we don't have to revisit it, because I'm not aware of any pressing need for 20.1 to be out the door. Am I missing some info, or are you happy for me to proceed along this path? | 22:37 |
| powersj | Odd_Bloke, can you estimate how long this delay would take? | 22:38 |
| Odd_Bloke | powersj: Tomorrow instead of tonight. | 22:39 |
| powersj | that's fine | 22:39 |
| johnsonshi | Odd_Bloke: Hmmm this is weird. The logs show that the swapfile creation command succeeds (for RHEL 7.7 cloud-init 18.4). | 22:59 |
| johnsonshi | 2020-02-19 22:23:35,338 - cc_mounts.py[DEBUG]: swap file /mnt/resource/swapfile exists, but not in /proc/swaps | 22:59 |
| johnsonshi | 2020-02-19 22:23:35,338 - util.py[DEBUG]: Running command ['sh', '-c', 'rm -f "$1" && umask 0066 && { fallocate -l "${2}M" "$1" || dd if=/dev/zero "of=$1" bs=1M "count=$2"; } && mkswap "$1" || { r=$?; rm -f "$1"; exit $r; }', 'setup_swap', '/mnt/resource/swapfile', '2048'] with allowed return codes [0] (shell=False, capture=True) | 22:59 |
| johnsonshi | 2020-02-19 22:23:35,400 - util.py[DEBUG]: creating swap file '/mnt/resource/swapfile' of 2048MB took 0.062 seconds | 22:59 |
| johnsonshi | After rebooting (cloud-init clean -lr) as well as a fresh provision of another machine, the file still was not being created (despite cloud-init.log saying that it is created). When I ssh'ed into the machine and ran the command above, the command succeeded and the swapfile was created. | 23:00 |
| johnsonshi | I just asked some of my teammates (I am in the Azure Linux provisioning team), and they said that we'd have to use this specific version of cloud-init due to a host of reasons. | 23:00 |
| Odd_Bloke | johnsonshi: What FS is /mnt/resource? | 23:02 |
| Odd_Bloke | johnsonshi: And is /mnt/resource ephemeral? I think cloud-init is going to assume that if it creates a swapfile, it stays there. | 23:03 |
| johnsonshi | Odd_Bloke: ext4. Yeah it is an ephemeral disk. | 23:04 |
| johnsonshi | Odd_Bloke: Oddly enough, when I included a runcmd directive, the swapfile was created. | 23:04 |
| Odd_Bloke | johnsonshi: So that shell snippet has been replaced by Python code in cloud-init master. | 23:05 |
| Odd_Bloke | So a one-off test with a recent daily build would give us some more information about what's going on here. | 23:06 |
| Odd_Bloke | Because if the completely rewritten code _also_ exhibits failure then my guess is that this isn't actually directly related to the swap creation, and instead it's something more complex. | 23:07 |
| Odd_Bloke | johnsonshi: I'm finishing up my day now, I'm afraid. If you can reproduce this with cloud-init dailies, then please file a bug. If it only exhibits in the version you're pinned on, then I think you'll need to talk to your team about how you've handled such issues in the past. | 23:08 |
| Odd_Bloke | (And have a good evening once you're done with your day too, of course. :) | 23:08 |
| johnsonshi | Odd_Bloke: OK I will try that out. Thanks for your help and have a good evening too! :) | 23:09 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!