Skip to content

Cirrus: Use packaged-based dependencies - #1936

Merged
openshift-merge-robot merged 1 commit into
podman-container-tools:masterfrom
cevich:cirrus_packaged_deps
Jul 8, 2019
Merged

Cirrus: Use packaged-based dependencies#1936
openshift-merge-robot merged 1 commit into
podman-container-tools:masterfrom
cevich:cirrus_packaged_deps

Conversation

@cevich

@cevich cevich commented Dec 4, 2018

Copy link
Copy Markdown
Contributor

Building/installing dependencies from fixed source-version ensures
testing is reliable, but introduces a maintenance burden and
risks testing far outside of a real-world environment. The
sensible alternative is to install dependencies from distro-packaging
systems.

Install all development and testing dependencies at cache-image build
time, to help ensure testing remains stable. The existing cache-image
build workflow can be utilized at any future time to build/test
with updated packages.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 4, 2018
@cevich cevich changed the title Cirrus: Use packaged dependencies WIP: [skip ci] Cirrus: Use packaged dependencies Dec 4, 2018
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 4, 2018
@cevich cevich changed the title WIP: [skip ci] Cirrus: Use packaged dependencies WIP: Cirrus: Use packaged dependencies Dec 4, 2018
@cevich
cevich force-pushed the cirrus_packaged_deps branch 5 times, most recently from 452bc54 to c65ff39 Compare December 4, 2018 19:54
@rh-atomic-bot

Copy link
Copy Markdown

☔ The latest upstream changes (presumably #1932) made this pull request unmergeable. Please resolve the merge conflicts.

@cevich
cevich force-pushed the cirrus_packaged_deps branch 15 times, most recently from caef081 to e778929 Compare December 5, 2018 21:29
@cevich

cevich commented Dec 5, 2018

Copy link
Copy Markdown
Contributor Author

N/B: Images built from this PR here: https://cirrus-ci.com/task/6236613032017920

@cevich
cevich force-pushed the cirrus_packaged_deps branch from e778929 to a29ff52 Compare December 5, 2018 22:32
@cevich

cevich commented Dec 6, 2018

Copy link
Copy Markdown
Contributor Author

Status (Both Fedora 29 and Ubuntu 18.04)

  • The package-based mechanics are working (though may be missing some pieces).
  • Direct podman testing dependencies are installed/upgraded at cache-image build-time.
  • At PR test-time, they are installed/upgraded again (to handle differences from, or benefit from cache).
  • All files associated with the 'podman' package are removed (to prepare building/installing from source).

@baude @mheon need some discussion/guidance here, given the frequent "issues" we've hit in PR & Master testing WRT runc (or any other direct dependency really). I'm afraid this approach is going to shackle modern libpod development work, to potentially much older dependency (e.g. runc) API's, behaviors, and bugs. At the same time, for any expected incompatible feature/API gaps, specific tests will need to be automatically skipped based on the dependency versions. That often leads to incomplete code coverage at the "edges" of tests.

In other words, if a runc problem is found by CI in a libpod PR, the author may potentially wait a long time (days) for an upstream fix, test, review, merge, test, package, and more testing.

Or (maybe worse), a test which touches a new (not yet available in runc package) feature and other code may be skipped. Meaning the "other" code does not get tested at all.

Are these things actual problems, and something we want to stand in front of as a libpod PR merge requirement? Tell me your thoughts?

BTW: I'm open to having two sets of tests, one being package-based, the other running with test-time compiled, "latest-master" version of the direct-dependencies (no sha-binding like before). If this is a better idea, which (or both) should be a gate for PR merges vs just advisory testing?

@cevich

cevich commented Feb 27, 2019

Copy link
Copy Markdown
Contributor Author

(rebased & force-pushed)

@cevich

cevich commented Mar 1, 2019

Copy link
Copy Markdown
Contributor Author

(rebased & force-pushed)

@rh-atomic-bot

Copy link
Copy Markdown

☔ The latest upstream changes (presumably #2528) made this pull request unmergeable. Please resolve the merge conflicts.

@rh-atomic-bot

Copy link
Copy Markdown

☔ The latest upstream changes (presumably #2214) made this pull request unmergeable. Please resolve the merge conflicts.

@cevich

cevich commented Mar 7, 2019

Copy link
Copy Markdown
Contributor Author

Properly FUBAR now...Realized it's a terrible idea to update + install at testing-time: This will cause non-sequiturial and difficult to diagnose problems. We need to cache all necessary package and source dependencies at image-build time. This way, during testing time, the environment is always configured consistently from run to run.

@rh-atomic-bot

Copy link
Copy Markdown

☔ The latest upstream changes (presumably #2531) made this pull request unmergeable. Please resolve the merge conflicts.

@rhatdan

rhatdan commented Mar 14, 2019

Copy link
Copy Markdown
Contributor

@cevich Still working on this one?

@rh-atomic-bot

Copy link
Copy Markdown

☔ The latest upstream changes (presumably #2660) made this pull request unmergeable. Please resolve the merge conflicts.

@rh-atomic-bot

Copy link
Copy Markdown

☔ The latest upstream changes (presumably #2650) made this pull request unmergeable. Please resolve the merge conflicts.

@lsm5

lsm5 commented Mar 18, 2019

Copy link
Copy Markdown
Contributor

sent runc updates to bodhi: https://bodhi.fedoraproject.org/updates/?packages=runc

@cevich

cevich commented Mar 21, 2019

Copy link
Copy Markdown
Contributor Author

Thanks @lsm5 . This PR is also dependent on the separate conmon package. One step closer though I s'pose 😄

@cevich

cevich commented May 2, 2019

Copy link
Copy Markdown
Contributor Author

Status: Waiting for #2561 before resuming work on this. Many changes here were applied in that PR, so this one needs a substantial refactoring.

@cevich

cevich commented Jun 3, 2019

Copy link
Copy Markdown
Contributor Author

Note to me: Rebased from 51923dea30e26089253b467b672d179d213c60a6 got most but not all conflicts. Still needs a bit of work on lib, some fixups of req_env_var calls, and a simplification: Nuke all the source-cache and install stuff, only do packages. If source is ever needed, it can be done as a one-off conditionally in setup_environment.sh while removing the cached package file. This also allows removing $DEPENDENCIES

@rhatdan

rhatdan commented Jun 8, 2019

Copy link
Copy Markdown
Contributor

@cevich Any update on this PR?

@cevich

cevich commented Jun 11, 2019

Copy link
Copy Markdown
Contributor Author

Yes, though this PR is dependent on the work separating/packaging conmon. It's also bit of a rebase-conflict nightmare. Many little bits and pieces were extracted and merged by other PRs, leaving a big mess here. I've done about half the cleanup work, but more is needed. I'll pick this up once I finish with my automated binary-release PR.

@rh-atomic-bot

Copy link
Copy Markdown

☔ The latest upstream changes (presumably #3261) made this pull request unmergeable. Please resolve the merge conflicts.

@rh-atomic-bot

Copy link
Copy Markdown

☔ The latest upstream changes (presumably #3321) made this pull request unmergeable. Please resolve the merge conflicts.

@cevich

cevich commented Jun 25, 2019

Copy link
Copy Markdown
Contributor Author

note to me: Ubuntu fails b/c searching for conmon in deprecated locations. Need something like #3429 to fix.

This was referenced Jun 25, 2019
Building/installing dependencies from fixed source-version ensures
testing is reliable, but introduces a maintenance burden and
risks testing far outside of a real-world environment.  The
sensible alternative is to install dependencies from distro-packaging
systems.

Install all development and testing dependencies at VM cache-image build
time, to help ensure testing remains stable.  The existing cache-image
build workflow can be utilized at any future time to build/test
with updated packages.

***N/B***: This does not update any dockerfiles used by testing, that is
left up to future efforts.

Signed-off-by: Chris Evich <cevich@redhat.com>
@cevich

cevich commented Jul 1, 2019

Copy link
Copy Markdown
Contributor Author

@rhatdan @mheon @baude Finally after months and months of toil, I think this is ready. PTAL.

@mheon

mheon commented Jul 1, 2019

Copy link
Copy Markdown
Contributor

This is smaller than I thought it would be. A lot easier to review as such.

Initial pass, everything seems reasonable. I'd just like @baude to have a look at the Varlink stuff - I know that's a packaging minefield.

Comment thread contrib/cirrus/lib.sh
@cevich

cevich commented Jul 1, 2019

Copy link
Copy Markdown
Contributor Author

This is smaller than I thought it would be.

Much of what made this PR so large originally, was split off into XXXL #2561 (already merged) 😄 Having that was absolutely critical for finishing development on this PR. Also critical was all of @haircommander and @lsm5 efforts on splitting/packaging conmon and maintaining the other dependent packages (thanks!).

@cevich

cevich commented Jul 8, 2019

Copy link
Copy Markdown
Contributor Author

@mheon @baude anything else holding this one back?

@mheon

mheon commented Jul 8, 2019

Copy link
Copy Markdown
Contributor

LGTM on my end
@baude @rhatdan @giuseppe PTAL

@rhatdan

rhatdan commented Jul 8, 2019

Copy link
Copy Markdown
Contributor

/lgtm

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants