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

fix(ci): add a "build-id" input var to ci builds #6

Open
wants to merge 107 commits into
base: main
Choose a base branch
from
Open

Conversation

rchincha
Copy link
Owner

@rchincha rchincha commented Mar 7, 2023

Currently, the github workflows dependencies are chained as follows:

build.yaml -> ci.yaml
release.yaml -> ci.yaml
convert.yaml -> ci.yaml

build.yaml and release.yaml are inline builds but operate on different build triggers. convert.yaml is timer-based and works offline.

@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2023

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@rchincha rchincha force-pushed the fix-ci branch 2 times, most recently from 5dac2d6 to 3a15e43 Compare July 12, 2023 04:57
@rchincha rchincha force-pushed the fix-ci branch 2 times, most recently from 5c77f6f to 773f7e6 Compare August 21, 2023 18:28
smoser and others added 9 commits August 22, 2023 09:03
…r#485)

This test is currently broken as a current version of stacker
cannot read an old version's cache.  The change was intended,
to clean up /stacker under c68147c.

Signed-off-by: Scott Moser <[email protected]>
…#482)

Its been a long time that this has been broken.  The run sections
in build.yaml were executing shell scripts without '-e' and thus
they would not exit on failure.

The end result was that if something went amuck you had to parse
through long error logs for what that was.

Also
 * Add '-x' to sh to make it more obvious where things failed.
 * Add the po4a package, which is used as part of the xz build.

Signed-off-by: Scott Moser <[email protected]>
…roject-stacker#487)

* fix: Use arrays for execution rather than scalar string.

This changes the interface for container.Execute to be an
array of strings rather than a string, and then changes
the internal lxc-wrapper pass arguments provided to it
through to the lxc 'start' api call.

It ultimately allows us to safely call programs with spaces,
quotes or other "odd" characters without first serializing
them in go and relying on lxc start api call to unserialize
them properly.  lxc.start would ultimately end up just
split the lxc.execute.cmd on spaces.

Signed-off-by: Scott Moser <[email protected]>

* test: Change bom test to contain spaces

The bom test case previously had Author of 'bom-test',
which was not realiastic and was intentional to avoid
exposing the bug with spaces.

Signed-off-by: Scott Moser <[email protected]>

* refactor: Bind stacker binary into container in SetupBuildContainerConfig

Previously there were 3 individual calls to bind stacker into
tools/static-stacker.  This just changes that to 1 place, so
after calling SetupBuildContainerConfig you can use stacker inside.

Signed-off-by: Scott Moser <[email protected]>

---------

Signed-off-by: Scott Moser <[email protected]>
…roject-stacker#489)

The bom commands are useful outside of stacker, and since
they're exposed to the user, they should *not* be part of the
hidden internal-go interface.

So, change:

 - stacker internal-go bom-discover
 + stacker bom discover

Also, shouldSkipInternalUserns was overly complicated as a result
of iterative development.  It is simpler now and based only
on the subcommand name with a single special case for
testsuite-check-overlay.

Signed-off-by: Scott Moser <[email protected]>
There was already a main for this, so we just have to add an entry
in the subcommand.  Also, sort those subcommand entries for good
measure, and drop a now-unneeded 'nolint'.

Signed-off-by: Scott Moser <[email protected]>
…ject-stacker#493)

A comment in metadata.go says that

  > overlayfs doesn't work with < 2 lowerdirs, so we add some

That doesn't seem right, you definitely do need 1 lowerdir,
but you do not need 2.

Signed-off-by: Scott Moser <[email protected]>
…ect-stacker#492)

Without xino=on you can end up with duplicate inodes numbers in the
same overlayfs mount.

Signed-off-by: Scott Moser <[email protected]>
…stacker#494)

molecule.overlayArgs provides options for a kernel mount-option string.
Kernel mount options for overlay require *more* than one lowerdir if
there is no upperdir.  molecule.Mount always performs read-only
mounts (thus no upperdir). So molecule.overlayArgs must add
a workaround lowerdir if there is only one.

Separately, lxcRootfsString function returns a string to be supplied to
lxc's lxc.rootfs.path.  lxc.rootfs.path is of the form:
  overlayfs:lowerdir[:lowerdir2:lowerdir3]:upperdir.
It requires 1 or more lowerdirs and an upperdir.
So in lxcRootfsString we need to create a workaround dir only if there
are zero lowerdirs (an empty container).

Signed-off-by: Scott Moser <[email protected]>
@rchincha rchincha force-pushed the fix-ci branch 2 times, most recently from ed81c61 to 7c14aa9 Compare September 1, 2023 23:44
rchincha and others added 12 commits September 5, 2023 13:16
* test: add a bom test for derived images

Signed-off-by: Ramkumar Chinchani <[email protected]>

* fix: refactor code and also upload empty config blob

Signed-off-by: Ramkumar Chinchani <[email protected]>

---------

Signed-off-by: Ramkumar Chinchani <[email protected]>
These tests are extremely slow.  We should probably use a different
container to test them in.  A recent run of c-i showed:

    ok 19 all container contents must be accounted for in 2617sec
    20 bom tool should work inside run in 1233sec

So these 2 tests accounted for 3850 seconds (64 minutes).

Signed-off-by: Scott Moser <[email protected]>
…tacker#497)

The build workflow was being 'used' by callers in ci and release.
Here also make 'nightly' a caller.

Additionally,
 * add 'slow-test' input to build.yaml, defaulting to true.
   Set that to false in ci.yaml so we do not run the slow tests
   on all PRs.
   slow-test value gets put into environment variable SLOW_TEST
   which is read by the test harness.
 * make build-id a scalar, not an array.
 * move build-id out of matrix (it is a constant) and reference
   it as an input instead.
 * Provide default values for 'privilege-level' and 'go-version'
   in build.yaml so the callers do not have to specify those.

Note that we still *do* specify the privilege-level and go-version
in a release build.  This is, I believe, because we want a consistent
thing that gets restored in the cache.

It is unfortunate, but as a result, we aren't testing with unpriv
during a release build.

Signed-off-by: Scott Moser <[email protected]>
…r#502)

When we run a stackerfile a second time, previously built tar layers
are assumed to already exist.  But if we are using squashfs layers,
then those layers are missing on a second run.  Make sure to mount
them when we find a built layer.

Closes project-stacker#454

Signed-off-by: Serge Hallyn <[email protected]>
Signed-off-by: Ramkumar Chinchani <[email protected]>
Co-authored-by: Serge Hallyn <[email protected]>
* fix: Create a 'test' target separate from 'check'

This just allows you to run 'make test'.  Previously the only
way to run test from make was to call 'make check' which require
running lint also.

* docs: Add some documentation to Makefile and doc/hacking.md

The docker.io bandwidth limit can easily stop a developer from being
able to build or test docker.  Even without the limit, docker.io
is much slower than a local zot or docker repository.

Add documentation on how to set the makefile variable
STACKER_DOCKER_BASE.

Also add information about 'make test' and individual test files.

---------

Signed-off-by: Scott Moser <[email protected]>
…er#505)

There is a PR against umoci#main to avoid trying to chmod +r when we
don't need to.  This will avoid trying to chmod +r on a mountpoint, which
is the root cause of project-stacker#450.

While we can and should also forward port our umoci dep to point at
current main, we should not do that as part of this PR, as that will
risk confusion if there are regressions from just the fwd port.

Closes project-stacker#450

Signed-off-by: Serge Hallyn <[email protected]>
So that nightly job failures (which are run async) are more explicit.

Signed-off-by: Ramkumar Chinchani <[email protected]>
…acker#511)

This change does several things all towards the end of making
it easier for developers to run tests and lint on go code.

Now a developer can run either 'make go-test' or 'make lint'
without having lots of C dependencies or running in a specific
container or stacker.

Things changed:

 * Add a build tag 'skipembed' that builds without needing
   cmd/stacker/lxc-wrapper/lxc-wrapper . This allows you to
   build (or test) without that file.  In order to ensure that
   you do not get very far trying to run a 'stacker' binary,
   I've added a 'panic' if stacker binary is built with skipembed.

 * Add download-tools target to makefile to ease in downloading of
   regctl and zot.

 * Move the downloading of golangci-lint from the github workflow
   to Makefile.  This makes it easier for developer to get it.
   It is also added to the 'download-tools' target.

 * be specific about the version of golangci-lint that is used.
   1.54.2.  golangci-lint's docs specifically say to do this:

    > IMPORTANT: It's highly recommended installing a specific version
    > of golangci-lint available on the releases page.

   changing it is simply a matter of changing the value in the makefile.

 * Move running of 'go test' out of the lint target to its own
   'go-test' target.  go-test target will now write a hack/coverage.html
   so you can view coverage easily.

 * add 'dlbin' makefile "function" for downloading binaries, and use
   that for regctl and zot from their rules.

 * rename TOOLS_DIR to TOOLS_D (consistency with BUILD_D, and just
   shorter)

Signed-off-by: Scott Moser <[email protected]>
…tacker#512)

Serge's change for avoiding chmod +r if not necessary was merged
upstream under opencontainers/umoci#499.
We do not need to use the project-stacker fork any more.

Dropping that and updating to current umoci main looks like this:

   go mod get github.com/opencontainers/umoci@main
   go mod edit -dropreplace github.com/opencontainers/umoci
   go mod tidy

Signed-off-by: Scott Moser <[email protected]>
@rchincha rchincha force-pushed the fix-ci branch 2 times, most recently from 9d03831 to 722d99e Compare March 23, 2024 16:28
rchincha and others added 20 commits March 23, 2024 09:53
Looks like missed one more place

Signed-off-by: Ramkumar Chinchani <[email protected]>
* feat: test directory removals

Add some tests to make sure when a directory is removed, its
contents are not visible later on.

Signed-off-by: Serge Hallyn <[email protected]>

* test: fix the directory removals test

project-stacker/umoci#6

overlayfs may set 'user.overlay.opaque=y' for deleted directories.
The above PR fixes this handling in our fork of umoci.

Signed-off-by: Ramkumar Chinchani <[email protected]>

---------

Signed-off-by: Serge Hallyn <[email protected]>
Signed-off-by: Ramkumar Chinchani <[email protected]>
Co-authored-by: Serge Hallyn <[email protected]>
CI failures indicate that this could be another error code that needs to
be handled.

project-stacker/umoci#7

Signed-off-by: Ramkumar Chinchani <[email protected]>
…t-stacker#609)

* fix: allow bom build and verification for `build_only` layers

From our experience, package information may be removed in such layers
making it much harder to discover and auto-construct BOMs.

So allow this for `build_only` layers also.

Signed-off-by: Ramkumar Chinchani <[email protected]>

* fix: import a bom only if available from built layers

It is possible that a build_only layer doesn't generate a BOM, so it
cannot be imported in a derived layer.

Signed-off-by: Ramkumar Chinchani <[email protected]>

---------

Signed-off-by: Ramkumar Chinchani <[email protected]>
this was missed earlier

Signed-off-by: Michael McCracken <[email protected]>
This includes a change to add the uncompressed size of each layer of an
image as an annotation.

Signed-off-by: Michael McCracken <[email protected]>
…ct-stacker#618)

The equalfile golang package defaults to max read size of 10**10
bytes unless both files use io.LimitedReader to set an upper bound
on the read.  In stacker import we already stat the two files being
compared and know the actual file size.  This PR constructs two
io.LimitedReader's for each file being compared.

Fixes: project-stacker#617

Signed-off-by: Ryan Harper <[email protected]>
the yaml value is `runtime_user`, not `user`.

Signed-off-by: Michael McCracken <[email protected]>
…t-stacker#610)

stacker builds allow chaining of layer builds.  SBOM chaining should
follow that model, so bom generation directives are concerned with only
that layer.

Signed-off-by: Ramkumar Chinchani <[email protected]>
For import types that will be checked, save the calculated hash of the
downloaded file and print them together in the format of a yaml imports
section for easy copy and pasting to add hashes to a file.

Because we only the parse file contents after the substitution
placeholders are replaced, this isn't a direct copy and paste
replacement, but it is an improvement, and it should be easy enough to
tell which expanded paths come from what input.

Signed-off-by: Michael McCracken <[email protected]>
With latest stacker-bom (v0.0.7) included, all bom enabled layers must
set their annotations.

Signed-off-by: Ramkumar Chinchani <[email protected]>
In the nightly extended bom tests, we use a centos base layer and hence
discovery must be performed.

Signed-off-by: Ramkumar Chinchani <[email protected]>
Stacker does not check if a bind mount source exists and if the source
is a directory then the container crashes with a stack trace that does
not indicate that the missing source dir is the issue.

Check if the source exists and return an error indicating the source
is missing instead of the container stack trace.

Signed-off-by: Ryan Harper <[email protected]>
Just copied Ryan's test from the description of project-stacker#624 with minimal
editing.

Signed-off-by: Michael McCracken <[email protected]>
overlayfs creates whiteout entries whenever a dir is created which is
not ideal. Instead, we inspect all the lower layers and only if a dir
exists, we emit a whiteout entry.

Note that we link against the 'stacker' branch of project-stacker/umoci

Signed-off-by: Ramkumar Chinchani <[email protected]>
we were missing the dict version in the docs

Signed-off-by: Michael McCracken <[email protected]>
codecov report upload is failing due to rate limiting

Signed-off-by: Ramkumar Chinchani <[email protected]>
Ensure that the coverage upload token is passed correctly.

Signed-off-by: Ramkumar Chinchani <[email protected]>
rchincha and others added 4 commits August 30, 2024 01:25
…ker#641)

For newly created dirs, overlayfs sets opaque attr which generates a
whiteout file in the blob tarball.

project-stacker/umoci#11

Signed-off-by: Ramkumar Chinchani <[email protected]>
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.

9 participants