OS-7601 want channel support for imgadm Reviewed by: John Levon <john.levon@joyent.com>

[timfoster]

This PR was migrated from https://github.com/joyent/smartos-live/pull/870

OS-7601 want channel support for imgadm Reviewed by: John Levon john.levon@joyent.com

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/6796/. The raw archive of this CR is here. See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@timfoster commented at 2019-08-15T15:57:18

Patch Set 1:

New commits:
commit 06f5104688b0cf5334292db2ec154d39fc08d50a
OS-7601 want channel support for imgadm

@timfoster commented at 2019-08-15T17:41:02

Patch Set 1:

I need to fix 'make check' - some of the new tests are tripping it up.

@timfoster commented at 2019-08-16T09:07:12

Patch Set 2:

New commits:
commit a8f1e255c4e847c7a0a6bf4fb3ef7d1c081a0741
fix 'make check' errors in imgadm tests

@timfoster commented at 2019-08-27T08:31:00

Patch Set 3: Patch Set 2 was rebased

@johnlevon commented at 2019-08-27T16:29:07

Patch Set 3:

(4 comments)

Do we want to keep a copy of the sdc-clients fork in the upstream repo as well and release it with a specific version number? I'm somewhat weirded out by local modifications here.

• imgadm list and other channel changes don't have tests

I'm also not really sure how much review you actually want for the specific changes, as presumably it's a straight backport of already-integrated code. So feel free to ignore my comments as required.

@timfoster commented at 2019-08-28T10:46:16

Patch Set 3:

(4 comments)

Do we want to keep a copy of the sdc-clients fork in the upstream repo as well and release it with a specific version number?

Good question: I don't know. I'm not sure whether there's any precedent for out-of-band updates of a node module. We certainly could create a 'smartos-live' branch of sdc-clients, and in there, add a "8.1.6" version (the next semver available in the 8.1.x stream) then tag a release that delivers this backport.

That would involve smartos-live pulling in another node-module, 'backoff', introduced in IMGAPI-484. That's only an extra 77k afaict, so might be feasible. The weird thing will be though, imgadm in smartos-live would still only contain a subset of that sdc-clients module, due to our deletion of irrelevant sdc-clients content!

I don't mind doing that if it'll help future maintenance.

I'm somewhat weirded out by local modifications here.

• imgadm list and other channel changes don't have tests

That's fair. Initially I thought it was only imgadm sources management that warranted new tests, but I'll add sanity tests elsewhere, thanks.

I'm also not really sure how much review you actually want for the specific changes, as presumably it's a straight backport of already-integrated code. So feel free to ignore my comments as required.

Yes, it was a straight backport. I'm not sure what's the best way to visualize that. Here's a patch showing imgapi.js in smartos-live today (without the changes from this review) with the last modification to imgapi.js from node-sdc-clients, which is:

commit 93af39d56fba23f40db6b0dc99950286ed47d1f5 (tag: v12.1.0) Author: Tim Foster tim.foster@joyent.com Date: Tue Nov 20 12:10:20 2018 +0000

TRITON-774 imgapi should allow admin-only imports from URLs
Reviewed by: Trent Mick <trentm@gmail.com>
Approved by: Trent Mick <trentm@gmail.com>

https://gist.github.com/timfoster/e394c0fb9ebb16b2c80b9710d55f547e

changes I have not taken in are related to the following modules

• sshpk
• smartdc-auth
• backoff

notably, we're not taking the cross-datacenter import work, img cloning, img import from URLs, pushing and importing docker images, none of which seem relevant to SmartOS.

I'll address your other comments later today - thanks for the review!

@timfoster commented at 2019-08-28T14:52:46

Patch Set 3:

(4 comments)

Patch Set 3 code comments

src/img/lib/sources/source.js#63 @johnlevon

no channel here?

src/img/lib/sources/source.js#63 @timfoster

Good idea, thanks.

src/img/node_modules/sdc-clients/lib/imgapi.js#274 @johnlevon

this comment seems at odds with :286 now

src/img/node_modules/sdc-clients/lib/imgapi.js#274 @timfoster

Yep, that seems like it was always wrong, even before this change. I'll delete that comment.

Here's a snippet:

url = 'https://ook.book.com/foobar?channel=experimental' 'https://ook.book.com/foobar?channel=experimental' var mod_url = require('url') undefined mod_url.parse(url) Url { protocol: 'https:', slashes: true, auth: null, host: 'ook.book.com', port: null, hostname: 'ook.book.com', hash: null, search: '?channel=experimental', query: 'channel=experimental', pathname: '/foobar', path: '/foobar?channel=experimental', href: 'https://ook.book.com/foobar?channel=experimental' } var parsed = mod_url.parse(url) undefined delete parsed.search true this.url = mod_url.format(parsed) 'https://ook.book.com/foobar' parsed.path '/foobar?channel=experimental'

src/img/test/import.test.js#107 @johnlevon

don't we want to fail the test if we hit an error here?

src/img/test/import.test.js#107 @timfoster

Yes, thanks. Testing that with a dummy TEST_EXPERIMENTAL_ORIGIN, the fix does correctly fails now.

src/img/test/import.test.js#386 @johnlevon

style nit: the other exec()s mark their args as err, o, e: can we in these new tests too please?

And don't we want to explicitly check for err?

src/img/test/import.test.js#386 @timfoster

Yep, we can certainly check for err.

In cases where we have nested execs() we can't reuse 'o' and 'e' because that fails make check with eg.

/mnt/tank/projects/smartos-live.git/src/img/test/import.test.js(469): warning: identifer e hides an identifier in a parent scope /mnt/tank/projects/smartos-live.git/src/img/test/import.test.js(469): warning: identifer o hides an identifier in a parent scope

@timfoster commented at 2019-08-29T12:18:05

Patch Set 4:

New commits:
commit f02bd00d87aa78ed202841be13a40894f71d2e29
update node-sdc-clients to new 8.2.0 version to get channel backport update backoff, required by the above improve tests a little

@timfoster commented at 2019-08-29T12:19:55

Patch Set 4:

This change brings in the 8.2.0 sdc-clients node module, and adds a test for 'imgadm list' showing a source with an associated channel.

@timfoster commented at 2019-08-30T13:07:14

Patch Set 5:

New commits:
commit d806c1b0ddd104768ad3e4347c034fb827e179b9
fix incorrect removal of comment about this.url double-check sdc-clients/lib/imgapi.js is identical to 8.2.0 copy

@johnlevon commented at 2019-08-30T13:40:40

Patch Set 5: Code-Review+1

@timfoster commented at 2019-09-03T12:21:48

Patch Set 6: Patch Set 5 was rebased

@timfoster commented at 2019-09-03T12:23:22

Uploaded patch set 7: Commit message was updated.

@timfoster commented at 2019-09-03T12:23:28

Patch Set 7:

New commits:
commit 01d3f4ca2c5e28eeec61853ab4fc25feadbb256e
fix incorrect removal of comment about this.url double-check sdc-clients/lib/imgapi.js is identical to 8.2.0 copy

commit ab60cd1294efe8862941e2056acc79276bc74b82  
update node-sdc-clients to new 8.2.0 version to get channel backport
update backoff, required by the above
improve tests a little

commit 9a8e62a33924299f9302ca64599a20276cb352fb  
fix 'make check' errors in imgadm tests

commit 72c936fefc3a021d5757b7da927c19f137dd5c00  
OS-7601 want channel support for imgadm

@timfoster commented at 2019-09-12T10:03:24

Patch Set 8: Patch Set 7 was rebased

comments copied from PR 870

[sjorge]

Should https://github.com/joyent/smartos-live/pull/870 be closed then?

[timfoster]

Should #870 be closed then?

Yes it should - unclear why the migration script didn't close it :-/ Thanks for the reminder! https://gist.github.com/timfoster/062d30ca636584bd7f2a1d1a53d6da01

[timfoster]

(This change needs to wait for joyent/node-sdc-clients#20 before it can integrate)

[timfoster]

I'd like to do another full test of this change before seeking review - last-minute testing on a full build of a SmartOS iso hit the bug that I think is fixed in the most recent commit, but I want to make sure.

[timfoster]

I've re-run the entire imgadm test suite, and did some manual sanity tests on the correct iso image I built, and am happy with this change now. The import test results were:

import.test.js
✔ setup: clean WRKDIR (/var/tmp/img-test-import)
✔ setup: ensure images.joyent.com source
✔ setup: get test image in local SDC IMGAPI (if available)
✔ setup: get origin for experimental image
✔ setup: CACHEDIR (/var/tmp/img-test-cache)
✔ setup: cache test image manifest
✔ setup: cache test image file
✔ setup1: remove image 0764d78e-3472-11e5-8949-4f31abea4e05
✔ imgadm import 0764d78e-3472-11e5-8949-4f31abea4e05
✔ imgadm ancestry 0764d78e-3472-11e5-8949-4f31abea4e05
✔ imgadm ancestry -j 0764d78e-3472-11e5-8949-4f31abea4e05
✔ setup2: remove image 0764d78e-3472-11e5-8949-4f31abea4e05
✔ imgadm install ... 0764d78e-3472-11e5-8949-4f31abea4e05
✔ setup3: remove image 0764d78e-3472-11e5-8949-4f31abea4e05
✔ concurrent: imgadm install ... 0764d78e-3472-11e5-8949-4f31abea4e05
✔ setup4: remove image 0764d78e-3472-11e5-8949-4f31abea4e05
✔ concurrent: imgadm import 0764d78e-3472-11e5-8949-4f31abea4e05
✔ setup5: remove image 0764d78e-3472-11e5-8949-4f31abea4e05
✔ pre-downloaded file; imgadm import 0764d78e-3472-11e5-8949-4f31abea4e05
✔ setup6: remove image 0764d78e-3472-11e5-8949-4f31abea4e05
✔ pre-downloaded file (bad size); imgadm import 0764d78e-3472-11e5-8949-4f31abea4e05
✔ setup7: remove image 0764d78e-3472-11e5-8949-4f31abea4e05
✔ pre-downloaded file (bad checksum); imgadm import 0764d78e-3472-11e5-8949-4f31abea4e05
✔ setup8: rm experimental image b323e23f-e762-4677-a2c8-b56f3bd5ef48
✔ experimental image import fails
✔ setup9: add updates.joyent.com source
✔ experimental image import with -C arg
✔ setup10: delete experimental image
✔ experimental image import with -S channel url
✔ setup11: delete experimental image
✔ experimental image import configured channel
✔ experimental channel sources show up in list output

update.test.js
✔ imgadm update

OK: 191 assertions (1840103ms)

[jlevon]

Thanks!