Merge lp://staging/~paelzer/curtin/bug-1574113-derived-repositories into lp://staging/~curtin-dev/curtin/trunk

Proposed by Christian Ehrhardt 
Status: Merged
Merged at revision: 410
Proposed branch: lp://staging/~paelzer/curtin/bug-1574113-derived-repositories
Merge into: lp://staging/~curtin-dev/curtin/trunk
Diff against target: 4008 lines (+3551/-146)
25 files modified
curtin/__init__.py (+2/-0)
curtin/commands/apt_config.py (+691/-0)
curtin/commands/curthooks.py (+11/-135)
curtin/commands/main.py (+1/-1)
curtin/gpg.py (+74/-0)
curtin/util.py (+144/-2)
doc/devel/README-vmtest.txt (+3/-3)
doc/topics/apt_source.rst (+152/-0)
examples/apt-source.yaml (+239/-0)
examples/tests/apt_config_command.yaml (+85/-0)
examples/tests/apt_source_custom.yaml (+97/-0)
examples/tests/apt_source_modify.yaml (+92/-0)
examples/tests/apt_source_modify_arches.yaml (+102/-0)
examples/tests/apt_source_modify_disable_suite.yaml (+92/-0)
examples/tests/apt_source_preserve.yaml (+98/-0)
examples/tests/apt_source_search.yaml (+97/-0)
examples/tests/apt_source_search_dns.yaml (+21/-0)
examples/tests/test_old_apt_features.yaml (+10/-0)
examples/tests/test_old_apt_features_ports.yaml (+10/-0)
tests/unittests/test_apt_custom_sources_list.py (+172/-0)
tests/unittests/test_apt_source.py (+927/-0)
tests/vmtests/__init__.py (+19/-5)
tests/vmtests/test_apt_config_cmd.py (+55/-0)
tests/vmtests/test_apt_source.py (+277/-0)
tests/vmtests/test_old_apt_features.py (+80/-0)
To merge this branch: bzr merge lp://staging/~paelzer/curtin/bug-1574113-derived-repositories
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve
Ryan Harper (community) Approve
Review via email: mp+296118@code.staging.launchpad.net

Commit message

Adding apt features to curtin.

This allows to define derived repositories by being able to control various apt configurations, proxies, mirrors, ppa's, sources.list content and pgp keys trusted by apt.
See examples/apt-source.yaml for examples and explanation of the individual capabilities.

Along the actual features a huge list of related unit- and vm-tests is added.

Description of the change

Adding apt_source features for the install stage of curtin.
This is inherited from cloud-init, but simplified (as we don't have to care about some features and old compatibility).
And then added plenty of unit- and vm-tests for the functionality.

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
442. By Christian Ehrhardt 

test_mir_apt_list_rename fix assertion of call

depending on the unittest environment the rename calls can differ in order
and amount. Convert the check to any_call

443. By Christian Ehrhardt 

test_mir_apt_list_rename mockup source filename

Depending on the unittest environment the source file could be wrong.
E.g. if a developer runs with a non default mirror.
Mock up the check for this file and by that, fixup the test in various
environments.

444. By Christian Ehrhardt 

fix premission error in unit test

depending on the environment it could have had the side effect of really trying
to rename files. That was bad for either the permission error or the actual
rename.
So we mock the rename to keep it free of side effects.

445. By Christian Ehrhardt 

drop reinstalling the mocked subp as it is not needed anymore

In the simplified test subp is not mocked to begin with, so there is no reason
to restore it.

446. By Christian Ehrhardt 

convert all apt_custom_sources_list tests to fakerelease

That makes them independent to the execution environment

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

The jenkins bot had identified a few issues with the unittests in Trusty environments which are now fixed.

Revision history for this message
Ryan Harper (raharper) wrote :

This is really nice.

A couple of nits in the code.

Super happy to see unittests AND vmtests +1

One last request, might be a doc/topics/apt_source.rst

I think most of it can be copied out of your example files which included lots of
documentation, but in general, I'd like to see some documentation that we'd hand
to the MAAS team about how to use the new dictionary config to pass through the
landscape derived repo.

review: Needs Fixing
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Hi,
thanks again for the review.

I added some comments to your feedback where appropriate.
ALmost all is accepted and integrated, just sometimes in slightly modified ways and the comments state the reasons.
Any non commented feedback is acked and integrated as suggested.

My own unit and vmtests are fine still, I'll run a full suite of vmtests and if successful push to the MP for the next round of review or acceptance.

447. By Christian Ehrhardt 

clarify docstring for mirror2lists_fileprefix

448. By Christian Ehrhardt 

make lists_d a global constant in the form of APT_LISTS

449. By Christian Ehrhardt 

fix whitespace issue

450. By Christian Ehrhardt 

fix docstrings to always use triple quotes

451. By Christian Ehrhardt 

degrade test_apt_srcl_custom to static method

452. By Christian Ehrhardt 

handle potential exception on os.rename

453. By Christian Ehrhardt 

replace get_release usage with already existing lsb_release

454. By Christian Ehrhardt 

add sanity checks and early exit to render_string_to_file

455. By Christian Ehrhardt 

move BASIC_MATCHER up to global variables

456. By Christian Ehrhardt 

improve error message if add_key_raw fails

457. By Christian Ehrhardt 

improve docstring of method add_key

458. By Christian Ehrhardt 

make default mirror and keyserver global variables

459. By Christian Ehrhardt 

add unittest for specifying a custom keyserver

460. By Christian Ehrhardt 

simplify find_apt_mirror_info by using DEFAULT_MIRRORS

This now shares a single default definition with rename_apt_lists

461. By Christian Ehrhardt 

dropping unrelated changes

FYI - rharper will carry those in his docu update branch

462. By Christian Ehrhardt 

optimize apt_source unittest imports

463. By Christian Ehrhardt 

apt_source: add doc in rst format and improve example yaml

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
464. By Christian Ehrhardt 

improve rst formatting

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Ok, I found an rst format checker and was able to improve that as well.
Should be available for another round of review towards acceptance now.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) :
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Hi,
thanks again for all the feedback.
Every section not explicitly mentiones is acked and integrated.

For everything else see the comment why/how I have modified the suggestions.

Running a full round of tests again before next upload.

465. By Christian Ehrhardt 

convert gpg key handling to pure python

466. By Christian Ehrhardt 

rename mirror2lists_fileprefix to mirrorurl_to_apt_fileprefix

This much more reflects what it does, so it helps code readability

467. By Christian Ehrhardt 

include exception info on the OSError exception for file renaming

468. By Christian Ehrhardt 

move generic gpg functions to util

469. By Christian Ehrhardt 

move generic part of template rendering to util

470. By Christian Ehrhardt 

add apt name portion to name add_key[_raw]

471. By Christian Ehrhardt 

backup sources.list before writing to it

472. By Christian Ehrhardt 

rename add_sources to the more appropriate add_apt_sources

473. By Christian Ehrhardt 

simply control-flow in apt_source

474. By Christian Ehrhardt 

simplify error handling in handle_apt_source

475. By Christian Ehrhardt 

remove unused import

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
476. By Christian Ehrhardt 

unify key tests for real keys in apt_src_keyid_real

477. By Christian Ehrhardt 

fix gpg key based unitests to work in networkless environments as well

478. By Christian Ehrhardt 

add test for alternative keyserver

479. By Christian Ehrhardt 

capture output of gpg calls to avoid messing up stdout/stderr

480. By Christian Ehrhardt 

fix fallback key import

481. By Christian Ehrhardt 

drop gpg activity from apt-source key tests

While the unittest gets a bit less real by that change, it will work in
protected environment (e.g. sbuild) and leave the developers .gpg keyring

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

This now also includes some extensions/fixes created and discussed on the cloud-init side of things.
There is no reason to wait and find them again in the curtin code.

I could build fine in sbuild, but was unable to test in a ppa for now (gets rejected for bin/source mismatch for some reason).

I started to mess around with the changes file trying to fix it, but I guess smoser has a much nicer and faster way to throw that at a ppa - I'd be happy about a discussion what was missing.

Ready for another round of review for getting merged.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :

This is really nice. Just a couple of minor issues with naming and logging.

review: Needs Fixing
482. By Christian Ehrhardt 

improve debug message

483. By Christian Ehrhardt 

report stacktrace on high level exception handler

484. By Christian Ehrhardt 

typo fix fule->file

485. By Christian Ehrhardt 

lower debug level of renaming apt list message

486. By Christian Ehrhardt 

fix exception handler that had a unreferenced %s

487. By Christian Ehrhardt 

prefix getkeybyid with gpg_ as the other related functions

488. By Christian Ehrhardt 

rename basic_render to basic_template_render for readability

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

All integrated, except one that I discussed on IRC

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Ready for another review or acceptance

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
489. By Christian Ehrhardt 

improve error handling of gpg functions

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Added another fix that came up when working on the cloud-init portion.
Ready for review towards merging again.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :

Looks great. Should get this merged today or tomorrow.

review: Approve
Revision history for this message
Scott Moser (smoser) wrote :

mostly nitpicks
some to make it like what is in cloud-init trunk.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Hi,
thanks for the review - all accepted and integrated except one.
I commented below and catch you on IRC for that one.

490. By Christian Ehrhardt 

move docsting after copyright

491. By Christian Ehrhardt 

move gpg functions to their own file

492. By Christian Ehrhardt 

drop unused self.orig_gpg_recv_key

493. By Christian Ehrhardt 

add curtin/gpg.py

494. By Christian Ehrhardt 

drop rstrip from gpg function

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

That is the integration of the last feedback, ready for review/merge again

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

further reading and thinking.
I think that the implementation doens't actually solve the thing we need solved.

apt_sources configuration to curtin declares what the target's apt should be configured with (much the same as the storage_config or 'kernel' configuration is about the installed environment not the host).

So, the apt_sources needs to apply to the target.
For maas, to get all of this working together, it has to
a.) declare to cloud-init that certain apt sources are necessary in the ephemeral environment
b.) declare to curtin that the target environment must include apt configured with a set of apt sources.

'a' is done in cloud-init user-data
'b' is done in curtin config.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Hi Scott,
I've implemented it the "new" way now and adapted all around that was needed.
Please read the comments I added below as answer to your feedback - to see if my assumptions when implementing made sense.

Everything else in the next round of code review (just doing a full round of tests and build before to be sure).

495. By Christian Ehrhardt 

Merge with upstream to avoid conflicts

pending merges:
  Ryan Harper 2016-06-14 [merge] reporting: set webhook handler level to DEBUG, no filtering
    Ryan Harper 2016-06-14 [merge] merge from trunk
    Ryan Harper 2016-06-14 reporting: default webhook handler level to DEBUG, no filtering
    Scott Moser 2016-06-14 [merge] improve net-meta network configuration
    Scott Moser 2016-06-13 spelling
    Scott Moser 2016-06-13 log cleanup
    Scott Moser 2016-06-13 improve net-meta network configuration
    Ryan Harper 2016-06-03 [merge] curtin/net: fix inet value for subnets, don't add interface attributes to alias
    Ryan Harper 2016-06-03 [merge] curtin/net: calculate the inet value for each subnet independently
    Mike Pontillo 2016-06-03 Address review comment.
    Mike Pontillo 2016-06-02 Don't append the '6' to IPv4 aliases if an IPv6 alias preceded it.
    Mike Pontillo 2016-06-02 Add failing test case for bug #1588547
    Ryan Harper 2016-06-03 [merge] curtin/net: Don't add interface attributes to alias interfaces
    Mike Pontillo 2016-06-03 Don't add additional parameters to alias interfaces. (Rebase from trunk and address review comments.)

496. By Christian Ehrhardt 

add docstring to apt_source.py's POPULATE_SUBCMD

497. By Christian Ehrhardt 

avoid python checker warnings by making POPULATE_SUBCMD lower case

498. By Christian Ehrhardt 

add --target parameter to apt-source command

This allows to modify the target environment as needed.
But given it is an argment can work in arbitrary chroots as long as the tools
like gpg, apt-key as well as the files it modifies are there.

499. By Christian Ehrhardt 

adapt apt-source tests to check the target environment

500. By Christian Ehrhardt 

report checked files in output_files_exist

This can help to quickly identify which file is missing in case a long list
of files is checked.

501. By Christian Ehrhardt 

lsb_release to support target

502. By Christian Ehrhardt 

propagate and use target throughout apt-source handling

503. By Christian Ehrhardt 

fix issue in ChrootableTarget if resolv.conf didn't yet exist in target

504. By Christian Ehrhardt 

ensure errorlist contains only strings

505. By Christian Ehrhardt 

improve error handling to not silently ignore program execution issues

506. By Christian Ehrhardt 

order config builtin in execution order

507. By Christian Ehrhardt 

extend builtin key of CONFIG_BUILTIN

It is a single namespace per stage - just "builtin" is too ambiguous.

508. By Christian Ehrhardt 

insert apt-source after extract

509. By Christian Ehrhardt 

remove useless warn if no source is specified

This can be a quite common case since we allo keys without surce now.
No need to flood the log with warnings that didn't even tell you much.

510. By Christian Ehrhardt 

revert rev 503: fix issue in ChrootableTarget if resolv.conf

511. By Christian Ehrhardt 

update apt data after changing configuration and/or source.lists

512. By Christian Ehrhardt 

mock out apt_update in apt-source unit tests

513. By Christian Ehrhardt 

allow a testcase to provide custom cloud-init data to the booted system

514. By Christian Ehrhardt 

apt-source vmtest now uses boot_cloudconf

This allows the vmtest to avoid having cloud-init to rewrite its just laid
down sources list. Just as a final user is expected to set this conf when
using curtin apt-source feature the test is using itnow as well.

515. By Christian Ehrhardt 

doc: update for apt-source now having a target specification

516. By Christian Ehrhardt 

don't skip cloud-init if we run in preserved mode

517. By Christian Ehrhardt 

take back individual builtin naming

518. By Christian Ehrhardt 

hook apt-source handling at curthooks

519. By Christian Ehrhardt 

doc: update new placement of apt-source in curthooks

520. By Christian Ehrhardt 

harden apt-source unittests for more test environments

In some environments the lower levels run a few extra udevadm settle.
This makes the unittests resilent against those slightly different, but
acceptable changes in behavior.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Ran full testsuite, tested build in sbuild and on ppa.
Ready for review and inclusion again.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
521. By Christian Ehrhardt 

remove gpg_ prefix not that related functions are in a gpg.py

522. By Christian Ehrhardt 

drop builtin sources.list

In case there is no custom source list specified we will fall back to modify
the one we find on the given target. That is done with as few dependencies to
the source.list content as possible, to avoid issues in case that changes.
If expected content isn't found curtin will warn about it but not fail.
That means we have time to adapt/fix in case of changes and in the worst case
one can always fix things with a custom template.

523. By Christian Ehrhardt 

drop unused method load_tfile_or_url in tests

524. By Christian Ehrhardt 

adapt documentation and examples

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Integrated the changes due to our discussion about having no "builtin" source.list.

If no custom template is provided we now fall back to modify whatever we find in the target.
All tests that should be affected passed already as well as an sbuild.

I'm running a ppa build and a full test-suite just to be sure and give this thread another ping then.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

All tests and ppa build went well, ready for re-review and inclusion.

525. By Christian Ehrhardt 

merge with latest trunk to avoid later merge conflicts

526. By Christian Ehrhardt 

restore populate_subcmd to old coding style, ignoring pylint complaint

This should keep the MP free of unrelated changes

527. By Christian Ehrhardt 

remove unrelated change in curtin/commands/install.py

That was needed/intended when we still wired it up as extra builtin command.
But no more now that we move to curthooks integration.

528. By Christian Ehrhardt 

remove stale merge artifacts

529. By Christian Ehrhardt 

integrate apt in curthooks

This makes the apt feature run by curthooks (but a noop of no config provided).
And additionally keeps the standalone command for other configs/targets.

530. By Christian Ehrhardt 

fix apt handling if no config is provided

531. By Christian Ehrhardt 

fix debconf if only provided for unconfigurable packages

532. By Christian Ehrhardt 

log apt_proxy configuration

533. By Christian Ehrhardt 

fix apt conf to match upper/lower case of apt-config

534. By Christian Ehrhardt 

add Testcases for old curtin apt features

535. By Christian Ehrhardt 

match aptproxy config path as specified

536. By Christian Ehrhardt 

translate old curtin apt features to new format

This includes (re-)moving of old curtin code.
All apt related configuration is handled in apt.py now.
In a config that specified old&new content it fails with a proper error
message as they are mutually exclusive (curtin can't decide which one to keep).

537. By Christian Ehrhardt 

modify existing functionality and tests to follow the new apt config format

538. By Christian Ehrhardt 

improve mirror and source_list handling

Replace the full string found in config, set the full specified string.
This gets rid of // or no / in testcases.
Set the mirror to what was specified, that is what has to be done.
Also fixup some references to the old custom_source_list to source_list

539. By Christian Ehrhardt 

add search and search_dns features

This inherits the cloud-init capabilities for these two features.
But it follows the new apt config format with split options for
primary/secondary for that as well.
Code could be simplified and unified a lot so that despite the extra split
primary/secondary it is mre readable now.
As discussed some fallbacks of cloud-init that rely on cloud-datasource are
dropped in the curtin handling of that configuration.

540. By Christian Ehrhardt 

add unittest test_mirror_search

541. By Christian Ehrhardt 

add unittest test_mirror_search_dns

542. By Christian Ehrhardt 

add unittests test_mirror_search_many2 and test_mirror_search_many3

543. By Christian Ehrhardt 

drop second subcheck that is already covered by test_mirror_search_many3

Would require some extra mocking as now search is hit for dns and non_dns cases.
Since it is covered already drop that part.

544. By Scott Moser

list feature APT_CONFIG_V1

545. By Christian Ehrhardt 

fixed urlparse call path

546. By Christian Ehrhardt 

fix py2/py3 compatibility import of urlparse

547. By Christian Ehrhardt 

add unittests for is_resolvable_url

548. By Christian Ehrhardt 

add vmtest for mirror_search features

549. By Christian Ehrhardt 

make dns search resilent against fqdn not being avavilable

550. By Christian Ehrhardt 

add vmtest for dns_search feature

551. By Christian Ehrhardt 

adapt example to cover search and search_dns features

552. By Christian Ehrhardt 

move primary/security mirror specifications into a dict - code

553. By Christian Ehrhardt 

move primary/security mirror specifications into a dict - examples

554. By Christian Ehrhardt 

move primary/security mirror specifications into a dict - testcases

555. By Christian Ehrhardt 

fix TestAptSrcModify reference to test yaml

556. By Christian Ehrhardt 

make mirror specs per acrhitecture - code

557. By Christian Ehrhardt 

make mirror specs per acrhitecture - examples

558. By Christian Ehrhardt 

make mirror specs per acrhitecture - testcases

559. By Christian Ehrhardt 

arches and repositioned mirrors adapted in doc

560. By Christian Ehrhardt 

add unittest test_mirror

561. By Christian Ehrhardt 

add unittest test_mirror_default

562. By Christian Ehrhardt 

add unittest test_mirror_arches

563. By Christian Ehrhardt 

add unittest test_mirror_arches_default

564. By Christian Ehrhardt 

add unittest test_mirror_arches_sysdefault

565. By Christian Ehrhardt 

add vmtest TestAptSrcModifyArches

566. By Christian Ehrhardt 

make replaced archive arch dependent

567. By Christian Ehrhardt 

make tetsing old apt features work cross platforms

568. By Christian Ehrhardt 

make use of new arch-dependent mirrors in unittests

This should also make the tests more likely to work on non amd64 architectures.

569. By Christian Ehrhardt 

make find apt mirror info tests work on ubuntu ports

570. By Christian Ehrhardt 

disable_pockets feature - code

571. By Christian Ehrhardt 

disable_pockets feature - example

572. By Christian Ehrhardt 

disable_pockets feature - unittest

573. By Christian Ehrhardt 

disable_pockets feature - vmtest

574. By Christian Ehrhardt 

update example in rst doc with disable_pockets feature

575. By Christian Ehrhardt 

update docu with some more use cases

576. By Christian Ehrhardt 

adapt old apt feature entry path to new mirrr config

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Ready for review - curtin implementation of the new definition as in the gdoc.
I need to update the commit message thou ..

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

That seems to be stuff that only triggers in an environment even more
restricted than I have tested on horse.
I'll try to fix and push over the day, ...

Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Wed, Jul 20, 2016 at 8:08 PM, Server Team CI bot <
<email address hidden>> wrote:

> Review: Needs Fixing continuous-integration
>
> FAILED: Continuous integration, rev:576
> https://server-team-jenkins.canonical.com/job/curtin-ci/318/
> Executed test runs:
> None:
> https://server-team-jenkins.canonical.com/job/generic-update-mp/332/console
>
> Click here to trigger a rebuild:
> https://server-team-jenkins.canonical.com/job/curtin-ci/318/rebuild
>
> --
>
> https://code.launchpad.net/~paelzer/curtin/bug-1574113-derived-repositories/+merge/296118
> You are the owner of lp:~paelzer/curtin/bug-1574113-derived-repositories.
>

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Hmm,
seems to be about mocking out an object that it hard fetched not from mock
context or so.
What might be interesting for CI in general is that I can reproduce with:
"nosetests tests/unittests" (as called by jenkins), while "make check" runs
just fine.

Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Thu, Jul 21, 2016 at 11:16 AM, Christian Ehrhardt <
<email address hidden>> wrote:

> That seems to be stuff that only triggers in an environment even more
> restricted than I have tested on horse.
> I'll try to fix and push over the day, ...
>
> Christian Ehrhardt
> Software Engineer, Ubuntu Server
> Canonical Ltd
>
> On Wed, Jul 20, 2016 at 8:08 PM, Server Team CI bot <
> <email address hidden>> wrote:
>
>> Review: Needs Fixing continuous-integration
>>
>> FAILED: Continuous integration, rev:576
>> https://server-team-jenkins.canonical.com/job/curtin-ci/318/
>> Executed test runs:
>> None:
>> https://server-team-jenkins.canonical.com/job/generic-update-mp/332/console
>>
>> Click here to trigger a rebuild:
>> https://server-team-jenkins.canonical.com/job/curtin-ci/318/rebuild
>>
>> --
>>
>> https://code.launchpad.net/~paelzer/curtin/bug-1574113-derived-repositories/+merge/296118
>> You are the owner of lp:~paelzer/curtin/bug-1574113-derived-repositories.
>>
>
>

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

The magic switch seems to be "--nologcapture" which is kind of unexpected
...
With it enabled things work, without it seems that the mocked call to
get_mirror like:
pmirror = get_mirror(cfg, "primary", arch)
Gets "mocked wrong"

Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Thu, Jul 21, 2016 at 11:24 AM, Christian Ehrhardt <
<email address hidden>> wrote:

> Hmm,
> seems to be about mocking out an object that it hard fetched not from mock
> context or so.
> What might be interesting for CI in general is that I can reproduce with:
> "nosetests tests/unittests" (as called by jenkins), while "make check" runs
> just fine.
>
> Christian Ehrhardt
> Software Engineer, Ubuntu Server
> Canonical Ltd
>
> On Thu, Jul 21, 2016 at 11:16 AM, Christian Ehrhardt <
> <email address hidden>> wrote:
>
>> That seems to be stuff that only triggers in an environment even more
>> restricted than I have tested on horse.
>> I'll try to fix and push over the day, ...
>>
>> Christian Ehrhardt
>> Software Engineer, Ubuntu Server
>> Canonical Ltd
>>
>> On Wed, Jul 20, 2016 at 8:08 PM, Server Team CI bot <
>> <email address hidden>> wrote:
>>
>>> Review: Needs Fixing continuous-integration
>>>
>>> FAILED: Continuous integration, rev:576
>>> https://server-team-jenkins.canonical.com/job/curtin-ci/318/
>>> Executed test runs:
>>> None:
>>> https://server-team-jenkins.canonical.com/job/generic-update-mp/332/console
>>>
>>> Click here to trigger a rebuild:
>>> https://server-team-jenkins.canonical.com/job/curtin-ci/318/rebuild
>>>
>>> --
>>>
>>> https://code.launchpad.net/~paelzer/curtin/bug-1574113-derived-repositories/+merge/296118
>>> You are the owner of lp:~paelzer/curtin/bug-1574113-derived-repositories.
>>>
>>
>>
>

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Ok, things make sense now.
Mocking without retvale sets pmirror to the mock.
That without disabled log leads to "LOG.info("got primary mirror: %s",
pmirror)" be a constructor call.

Ok, that should be fixable ...

Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Thu, Jul 21, 2016 at 11:54 AM, Christian Ehrhardt <
<email address hidden>> wrote:

> The magic switch seems to be "--nologcapture" which is kind of unexpected
> ...
> With it enabled things work, without it seems that the mocked call to
> get_mirror like:
> pmirror = get_mirror(cfg, "primary", arch)
> Gets "mocked wrong"
>
> Christian Ehrhardt
> Software Engineer, Ubuntu Server
> Canonical Ltd
>
> On Thu, Jul 21, 2016 at 11:24 AM, Christian Ehrhardt <
> <email address hidden>> wrote:
>
>> Hmm,
>> seems to be about mocking out an object that it hard fetched not from
>> mock context or so.
>> What might be interesting for CI in general is that I can reproduce with:
>> "nosetests tests/unittests" (as called by jenkins), while "make check" runs
>> just fine.
>>
>> Christian Ehrhardt
>> Software Engineer, Ubuntu Server
>> Canonical Ltd
>>
>> On Thu, Jul 21, 2016 at 11:16 AM, Christian Ehrhardt <
>> <email address hidden>> wrote:
>>
>>> That seems to be stuff that only triggers in an environment even more
>>> restricted than I have tested on horse.
>>> I'll try to fix and push over the day, ...
>>>
>>> Christian Ehrhardt
>>> Software Engineer, Ubuntu Server
>>> Canonical Ltd
>>>
>>> On Wed, Jul 20, 2016 at 8:08 PM, Server Team CI bot <
>>> <email address hidden>> wrote:
>>>
>>>> Review: Needs Fixing continuous-integration
>>>>
>>>> FAILED: Continuous integration, rev:576
>>>> https://server-team-jenkins.canonical.com/job/curtin-ci/318/
>>>> Executed test runs:
>>>> None:
>>>> https://server-team-jenkins.canonical.com/job/generic-update-mp/332/console
>>>>
>>>> Click here to trigger a rebuild:
>>>> https://server-team-jenkins.canonical.com/job/curtin-ci/318/rebuild
>>>>
>>>> --
>>>>
>>>> https://code.launchpad.net/~paelzer/curtin/bug-1574113-derived-repositories/+merge/296118
>>>> You are the owner of
>>>> lp:~paelzer/curtin/bug-1574113-derived-repositories.
>>>>
>>>
>>>
>>
>

577. By Christian Ehrhardt 

make metohd of apt commandline handling less ambiguous

578. By Christian Ehrhardt 

add docstring to clean_cloud_init

579. By Christian Ehrhardt 

follow the "logging-not-lazy" warning of checkers and change string extension

580. By Christian Ehrhardt 

fix unittests for cases without --nologcapture

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Hmm, that is tox now with a special case on urllib which changed py2->py3.
General question - should we make a tox run part of make check ?

581. By Christian Ehrhardt 

avoid py2 pylint issue due to renamed urllib import

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

Started a vmtest run at
https://server-team-jenkins.canonical.com/job/curtin-vmtest-venonat-devel/33/

I dont think we have 'preserve_sources_list' implemented as intended.

Our doc says:
|  preserve_sources_list: <true|false|keep>
|    # For cloud-init this defaults to false. By default, cloud-init renders
|    # /etc/apt/sources.list from its provided templates on first boot.
|    # For Curtin this defaults to false. By default, curtin will always attempt to
|    # replace mirrors inside the sources.list it finds in the image.
|    # Curtin will configure cloud-init in the target via
|    # /etc/cloud/cloud.cfg.d/curtin-preserve-sources.list with True,
|    # which causes cloud-init to not touch /etc/apt/sources.list by default.
|    # Unless explicitly  requested by cloud-init userdata (overwriting to False).
|    # Effectively this changes the default from False to True

I thought that in discussion you and I had agreed that this was in fact 'boolean', and that curtin would *always* replace /etc/apt/sources.list . I'm not terribly opposed to a tri-state, where it can be told to leave it untouched though.

That said, i do not see where curtin is configuring cloud-init in the target to set. curtin should always set cloud-init to preserve sources.list unless there is another use case that i'm missing.

I'm not done with the review yet, but wanted to hit 'submit' before losing context and losing what i had done.

Revision history for this message
Scott Moser (smoser) wrote :

for disable_pockets, it looks to me like the string provided always has the release prepended.
for example:
 disable_pockets = [updates]
will disable 'trusty-updates'.

that makes sense.
but for:
  disable_pockets = ["mypocket", "unstable"]
this doesnt make so much sense.
I dont know how easily to do this other than special casing the words:
 updates, release, security, backports

also, might want to just check that this content from my /etc/apt/sources.list.d/google-chrome.list works as expected:
 deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main

Revision history for this message
Scott Moser (smoser) wrote :

I thought that 'uri' would override 'search', does it?

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

> Started a vmtest run at
> https://server-team-jenkins.canonical.com/job/curtin-vmtest-venonat-devel/33/

Thanks, that one worked at least.

> I dont think we have 'preserve_sources_list' implemented as intended.

Yeah the tristate was dropped in our discussion yet the doc needs update (DONE now).
The placing of the config for cloud-init was missing (I missed to add to our todo's after the discussion).
Pushed in the next update to the MP.

Also all vmtests that take care to check.
So far I provided "preserve_sources_list" via the vmtest, now that it is implemented I dropped it there and it is still working the same way.

It seems I need something to make cloud-init to pick up the config thou - will contact you later today as I guess that is talked in 3 or searched in 15 minutes :-)

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

> I thought that 'uri' would override 'search', does it?

Yes it does:
see curtin/commands/apt.py:get_mirror

1. uri
2. search
3. search_dns

As soon as one of 1-3 hits the others aren't checked anymore.

Also see the following unittests that ensure this is true:
test_mirror_search_many3 (uri beats search & search_dns)
test_mirror_search_many2 (search beats search_dns)

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

> for disable_pockets, it looks to me like the string provided always has the
> release prepended.
> for example:
> disable_pockets = [updates]
> will disable 'trusty-updates'.
>
> that makes sense.
> but for:
> disable_pockets = ["mypocket", "unstable"]
> this doesnt make so much sense.
> I dont know how easily to do this other than special casing the words:
> updates, release, security, backports

I didn't think of "custom" pockets yet, but I see no other easy way than "special casing" them.

> also, might want to just check that this content from my
> /etc/apt/sources.list.d/google-chrome.list works as expected:
> deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main

Hrm, yeah man sources.list states this (options) in the section "ONE-LINE-STYLE FORMAT".
I need to adapt to work with them as well.
Although it is highly unlikely to find some in a source.list (disabling doesn't care about other files in sources.list.d) these days we should look ahead and be prepared.
Working on it ...

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

> > I dont know how easily to do this other than special casing the words:
> > updates, release, security, backports
>
> I didn't think of "custom" pockets yet, but I see no other easy way than
> "special casing" them.

I was missing an "... either" here.

But giving it some thought - if we go as far as to allow custom pocket names.
We need to cover custom pocket names that match the known names.
So what if someone has a "updates" pocket and it shall not become xenial-updates.

We already have a way to specify these and I think we should stick to that.
We can allow $RELEASE in there and replace it, just as we do when rendering sources.list in the first place.

That allows one to differentiate between: "$RELEASE-updates" and "updates"

That also covers the sepcial case of the main release pocket being "".
One can just specify "$RELEASE" and gets e.g. "xenial" which is just what we need in that case as well.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Reading more into it we should think about:
- what we called "pockets" is actually defined as "suite" - should we rename our option?
- long term (not for this bug & MP) think about supporting deb822 format as well

582. By Christian Ehrhardt 

leave cloud-init config to prevent clobbering sources.list

583. By Christian Ehrhardt 

add $RELEASE replacement to disable_pockets

584. By Christian Ehrhardt 

improve doc wording

585. By Christian Ehrhardt 

check for old apt_preserve_list config as we select th eold for compatibility

586. By Christian Ehrhardt 

fix typo

587. By Christian Ehrhardt 

support source.list lines with options

588. By Christian Ehrhardt 

cover options in source.list lines in unittests

589. By Christian Ehrhardt 

fix test_old_apt_features vmtest

590. By Christian Ehrhardt 

switch apt_preserve_sources_list to True

Now that all tests have succeeded we switch the apt_preserve_sources_list
that is passed to cloud-init to True to keep curtin rendered sources.list
intact unless overwritten by user data.

591. By Christian Ehrhardt 

rename pocket to suite following man sources.list

592. By Christian Ehrhardt 

rename curtin-preserve-sources.list to curtin-preserve-sources.cfg

The example in the gdoc was uncommon, clout-init usually has .cfg files

593. By Christian Ehrhardt 

rename vmtets yaml following the pockets to suite rename

594. By Christian Ehrhardt 

fix preserve check in vmtest TestAptSrcPreserve

In this path the preserving of the target is not triggered, so the respective
checks need to be adapted.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

All Feedback handled and a bit more (according to my comments to the review feedback before).
Ready again.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
595. By Christian Ehrhardt 

make unittests runnable as non-privileged user in tox

596. By Christian Ehrhardt 

let curtin exit with failure if protecting sources.list didn't work

597. By Christian Ehrhardt 

make trusty pep8 happy which even complains about raw string length

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

de-toxed the unitests for disable_suites

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

> > > I dont know how easily to do this other than special casing the words:
> > > updates, release, security, backports
> >
> > I didn't think of "custom" pockets yet, but I see no other easy way than
> > "special casing" them.
>
> I was missing an "... either" here.
>
> But giving it some thought - if we go as far as to allow custom pocket names.
> We need to cover custom pocket names that match the known names.
> So what if someone has a "updates" pocket and it shall not become xenial-
> updates.
>
> We already have a way to specify these and I think we should stick to that.
> We can allow $RELEASE in there and replace it, just as we do when rendering
> sources.list in the first place.
>
> That allows one to differentiate between: "$RELEASE-updates" and "updates"
>
> That also covers the sepcial case of the main release pocket being "".
> One can just specify "$RELEASE" and gets e.g. "xenial" which is just what we
> need in that case as well.

I'd really prefer to handle let the user specify 'updates', 'release' literally rather than '$RELEASE-updates'.

that does mean that these "well known names" block the user from using pockets literally named 'release'. I usually dont like such holes in design, but i think it might be worth it here for readability. The pocket names 'release', 'updates', 'proposed' and 'backports' are well known in Ubuntu (example https://wiki.ubuntu.com/SecurityTeam/FAQ).

given the same url there, i use that for justification of the name 'pockets'. If you'd like we can also support 'suites', but i think pocket is fine.

Revision history for this message
Scott Moser (smoser) wrote :

I think you must not have seen my in-line comments in my review that starts with
'for disable_pockets, '

598. By Christian Ehrhardt 

auto extend known ubunut suites by $RELEASE-

599. By Christian Ehrhardt 

reduce default verbosity of apt feature by moving some info to debug

600. By Christian Ehrhardt 

make old apt features less verbose

601. By Christian Ehrhardt 

move old apt feature config conversion into own method and out of curthooks

602. By Christian Ehrhardt 

rework apt standalone command entry path

This is dropping a lot of old artifacts from the command entry path.

603. By Christian Ehrhardt 

minimal commandline call test for apt features

604. By Christian Ehrhardt 

add spaced around string concat plus operators

605. By Christian Ehrhardt 

merge rev 400: move uefi boot knowledge from launch and vmtest to xkvm

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

All we discussed yesterday night is in now, ready for review/merge

606. By Christian Ehrhardt 

fix config of ChrootableTarget for apt command

607. By Christian Ehrhardt 

adapt standalong command example to new invocation

608. By Christian Ehrhardt 

change standalong apt command test to be vmtest based

609. By Christian Ehrhardt 

rename apt to apt_config for stanalone command

That implies to rename the module name as well.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
610. By Christian Ehrhardt 

change apt to apt-config in vmtest

611. By Christian Ehrhardt 

change ppa's and keys used to something under our control

612. By Christian Ehrhardt 

change the preserve_sources_list default to True in curtin

613. By Christian Ehrhardt 

rework testing of standalone command

614. By Christian Ehrhardt 

make copy in testcases release independent

615. By Christian Ehrhardt 

add fallback if user did not add .list to source keys

Includes modification to one of the testcastes to cover that.

616. By Christian Ehrhardt 

fixup test to check for content added by apt-add-repository

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Since we co-work on a branch that we can jointly push to now there is a new MP at https://code.launchpad.net/~ubuntu-server/curtin/bug-1574113-derived-repositories/+merge/301362

This old MP here is kept open to have the discussion history available (not sure if it would go away when rejecting the old MP).

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches