Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using prior builds for improving encapsulation #4271

Merged
merged 3 commits into from
May 23, 2023

Conversation

RishabhSaini
Copy link
Contributor

@RishabhSaini RishabhSaini commented Jan 20, 2023

Fixes: #4247
To do:

  • Adding cmd option of taking prior builds
  • Passing packing structure to ostree-rs-ext/pull/456
  • Adding test case for encapsulation and encapsulation_derived

@openshift-ci
Copy link

openshift-ci bot commented Jan 20, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tests/encapsulate.sh Fixed Show fixed Hide fixed
tests/encapsulate_derived.sh Fixed Show fixed Hide fixed
tests/encapsulate_derived.sh Fixed Show fixed Hide fixed
@RishabhSaini RishabhSaini force-pushed the issue/4012 branch 13 times, most recently from be10f1e to 13ff8cc Compare April 5, 2023 22:27
rust/src/compose.rs Outdated Show resolved Hide resolved
@RishabhSaini
Copy link
Contributor Author

RishabhSaini commented Apr 5, 2023

We also need to tackle the case where we need to supply --write-frequency-info-to to compose tree and that same file should be referred in --frequencyinfo in container-encapsulate during the separate invocations of these commands in cosa build

What would be a better way to supply the metadata to container encapsulate?

@cgwalters
Copy link
Member

We also need to tackle the case where we need to supply --write-frequency-info-to to compose tree and that same file should be referred in --frequencyinfo in container-encapsulate during the separate invocations of these commands in cosa build

I think we can stash this file in the temporary directory allocated in compose image right?

let tempdir = Utf8Path::from_path(tempdir.path()).unwrap();

@RishabhSaini RishabhSaini force-pushed the issue/4012 branch 2 times, most recently from 6901fae to 3bb2853 Compare May 19, 2023 16:11
@RishabhSaini RishabhSaini force-pushed the issue/4012 branch 2 times, most recently from 1bd612f to 1f5d758 Compare May 19, 2023 19:24
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nits

rust/src/container.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/libpriv/rpmostree-refts.cxx Outdated Show resolved Hide resolved
src/libpriv/rpmostree-refts.cxx Outdated Show resolved Hide resolved
src/libpriv/rpmostree-refts.cxx Outdated Show resolved Hide resolved
@RishabhSaini RishabhSaini marked this pull request as ready for review May 19, 2023 20:57
@RishabhSaini RishabhSaini requested a review from cgwalters May 19, 2023 21:00
@cgwalters
Copy link
Member

Looks like this needs a rebase now?

@cgwalters
Copy link
Member

This actually needed another rebase after #4422

I also reworked the two commits such that all the changes related to bumping ostree-ext were in the prior commit.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think this is good to land! There's clearly some followup we will want to do but I think we can iterate on that after merging.

@cgwalters
Copy link
Member

Blah, I shouldn't have merged that dependabot clap rebase, it conflicted. Rebased this on that anyways though.

- ostree-ext updates to new ostree, so adjust our usage of the
  ostree crate
- Also adjust for changed ostree-ext APIs
- ostree-ext now re-exports cap-std-ext 2.0, which stopped re-exporting
  rustix, so depend on that directly
Prune rpm changelogs of packages to within the last year and use it as
frequency of update for the rpm.
@cgwalters
Copy link
Member

cgwalters commented May 23, 2023

OK needed a small tweak for the usage test due to the clap change. Not sure what's going wrong though in the container failure, might just be the same timeout as other jobs though.

@RishabhSaini
Copy link
Contributor Author

Not sure why the destructive tests are failing

@cgwalters
Copy link
Member

Not sure yet either...as far as I can tell, we seem to be failing to reboot?

@cgwalters
Copy link
Member

cgwalters commented May 23, 2023

OK so I debugged this using

walters@toolbox /v/s/w/s/g/c/coreos-assembler (enable-ostree-composefs)> git diff
diff --git a/mantle/kola/harness.go b/mantle/kola/harness.go
index ac539638d..e39c028cc 100644
--- a/mantle/kola/harness.go
+++ b/mantle/kola/harness.go
@@ -1085,6 +1085,7 @@ Environment=KOLA_UNIT=%s
 Environment=KOLA_TEST=%s
 Environment=KOLA_TEST_EXE=%s
 Environment=%s=%s
+StandardOutput=journal+console
 ExecStart=%s
 `, unitName, testname, base, kolaExtBinDataEnv, destDataDir, remotepath)
        if targetMeta.InjectContainer {

(Should probably make this something like kola run --debug-ext-to-console or something)

Anyways the problem is the automatic upgrade trigger is failing:

[   26.058914] kola-runext-container-image[1189]: May 23 21:19:43 qemu0 rpm-ostree[1138]: Txn AutomaticUpdateTrigger on /org/projectatomic/rpmostree1/fedora_coreos failed: While checking against deployment timestamp: Upgrade target revision '630f678a16cf080256783b906540906da0b75763905f8d4bc15300fe0bb5af47' with timestamp 'Tue May 23 20:59:57 2023' is chronologically older than current revision '96a6844b758443cb4287a25e74bef511ddd7df00f264d01b4196ff19efb7d55e' with timestamp 'Tue May 23 21:18:46 2023'; use --allow-downgrade to permit

(So we don't reboot, hence hang)

And I think that is fallout from ostreedev/ostree-rs-ext@6c6f9a0
EDIT: No, it must be fallout from ostreedev/ostree-rs-ext@f450812

Which...perhaps arguably is actually a good thing. Hmm.

This no longer works since
ostreedev/ostree-rs-ext@f450812

Which is actually a good thing.

I'll look at readding this test in another place.
@cgwalters
Copy link
Member

OK, pushed a change that should fix this.

@cgwalters cgwalters merged commit ebb10e6 into coreos:main May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using historical build information for encapsulation
3 participants