Bump image-rs rev to include image-layer-ordering fix #8670

mkulke · 2023-12-14T09:15:44Z

The underlying issue in the dependency has been addressed, hence the revision bump of image-rs to use the fix in the agent. I think a fix to the CCv0 branch is warranted, since it makes all images with more than a single layer work unreliably.

cc @stevenhorsman @bpradipt

stevenhorsman · 2023-12-14T09:20:15Z

@mkulke - I think the agreement that Pradipta made with Fabiano on the policy update was to add a statement in the commit message to confirm that this is not tested by CI, so the update is at the end users own risk or similar. @bpradipt - am I remembering that correctly?

mkulke · 2023-12-14T09:27:45Z

@mkulke - I think the agreement that Pradipta made with Fabiano on the policy update was to add a statement in the commit message to confirm that this is not tested by CI, so the update is at the end users own risk or similar. @bpradipt - am I remembering that correctly?

👍 seeing the disclaimer in the policy cherry-pick and added it here

bpradipt

/lgtm
Thanks for adding the disclaimer in the commit message.

stevenhorsman

LGTM. Thanks

mkulke · 2023-12-15T10:59:51Z

anything left to do here on my side? I don't think I can turn the remaining checks green myself (but maybe that's not required?)

stevenhorsman · 2023-12-15T11:02:50Z

/test

stevenhorsman · 2023-12-15T11:10:02Z

anything left to do here on my side? I don't think I can turn the remaining checks green myself (but maybe that's not required?)

So even though this is an untested branch now, I guess it is worth us trying to get some of the checks passing.

Commit message check should be simple - just add agent: to the start of the commit header
Apart from s390x tests I don't think we have any kata-containers Jenkins CI enabled, so I'll update the branch protection rules to disable it
The static checks and s390x CI tests are failing with kata_agent.go:796: File is not gofmt-ed with -s (gofmt) - I know you didn't introduce this, and I guess it came in with Pradipta's cherry pick of policy work, but it might be worth adding that to the commit to get through this hurdle?

The alternative is we just say screw it and use admin override to merge?

mkulke · 2023-12-15T11:16:29Z

Commit message check should be simple - just add agent: to the start of the commit header

ah, sorry. missed that, fixed.

mkulke · 2023-12-15T11:24:22Z

The static checks and s390x CI tests are failing with kata_agent.go:796: File is not gofmt-ed with -s (gofmt) - I know you didn't introduce this, and I guess it came in with Pradipta's cherry pick of policy work, but it might be worth adding that to the commit to get through this hurdle?

👍 formatted the file

stevenhorsman · 2023-12-15T11:31:03Z

/test

mkulke · 2023-12-15T11:55:25Z

@stevenhorsman sry, I messed up the commit message. would you mind...

👉🌟👈

stevenhorsman · 2023-12-15T12:04:15Z

/test

stevenhorsman · 2023-12-15T12:44:33Z

We're getting:

error[E0425]: cannot find value `VERSION_3_3_0` in crate `protobuf`
  --> /home/runner/.cargo/git/checkouts/guest-components-1e54b222ad8d9630/e18e86d/ocicrypt-rs/src/utils/ttrpc/keyprovider.rs:26:49
   |
26 | const _PROTOBUF_VERSION_CHECK: () = ::protobuf::VERSION_3_3_0;
   |                                                 ^^^^^^^^^^^^^ help: a constant with a similar name exists: `VERSION_3_2_0`
   |
  ::: /home/runner/actions-runner/_work/kata-containers/kata-containers/src/github.com/kata-containers/kata-containers/src/agent/target/x86_64-unknown-linux-musl/release/build/protobuf-0624020e552c1844/out/version.rs:7:1
   |
7  | pub const VERSION_3_2_0: () = ();
   | --------------------------- similarly named constant `VERSION_3_2_0` defined here

In the static checks now, but it looks like potentially an issue in the guest-components?

mkulke · 2023-12-15T13:00:02Z

We're getting:

error[E0425]: cannot find value `VERSION_3_3_0` in crate `protobuf`
  --> /home/runner/.cargo/git/checkouts/guest-components-1e54b222ad8d9630/e18e86d/ocicrypt-rs/src/utils/ttrpc/keyprovider.rs:26:49
   |
26 | const _PROTOBUF_VERSION_CHECK: () = ::protobuf::VERSION_3_3_0;
   |                                                 ^^^^^^^^^^^^^ help: a constant with a similar name exists: `VERSION_3_2_0`
   |
  ::: /home/runner/actions-runner/_work/kata-containers/kata-containers/src/github.com/kata-containers/kata-containers/src/agent/target/x86_64-unknown-linux-musl/release/build/protobuf-0624020e552c1844/out/version.rs:7:1
   |
7  | pub const VERSION_3_2_0: () = ();
   | --------------------------- similarly named constant `VERSION_3_2_0` defined here

In the static checks now, but it looks like potentially an issue in the guest-components?

uh, oh. yeah, that's an issue with code-generated files being checked into the repo. those files have to be deleted and rebuilt. There was a similar issue in the guest-components repo.

mkulke · 2023-12-15T13:03:31Z

there's nothing we can do here, we need to repair this comprehensively in guest-components 😑

stevenhorsman · 2023-12-15T13:05:23Z

there's nothing we can do here, we need to repair this comprehensively in guest-components 😑

I guess we need to wait for that fix then and bump the image-rs version afterwards, otherwise we'll hit this issue when trying to build the kata-agent for the peer pod VM image?

mkulke · 2023-12-15T13:11:23Z

there's nothing we can do here, we need to repair this comprehensively in guest-components 😑

I guess we need to wait for that fix then and bump the image-rs version afterwards, otherwise we'll hit this issue when trying to build the kata-agent for the peer pod VM image?

yes. I think that's the case. kata-agent will build w/ the "older" generated ttrpc code, but the guest-component's CI does not. I don't think we can commit the generated files

mkulke · 2023-12-15T13:17:44Z

there's nothing we can do here, we need to repair this comprehensively in guest-components 😑

I guess we need to wait for that fix then and bump the image-rs version afterwards, otherwise we'll hit this issue when trying to build the kata-agent for the peer pod VM image?

yes. I think that's the case. kata-agent will build w/ the "older" generated ttrpc code, but the guest-component's CI does not. I don't think we can commit the generated files

Maybe if we regenerate a full Cargo.lock file (atm only ocicrypt-rs and image-rs are bumped in the lock file) it might still work. (we'll get all sort of dependencies bumped within their semver boundaries, but maybe that's tolerable) 🤞

stevenhorsman · 2023-12-15T13:35:03Z

/test

stevenhorsman · 2023-12-15T13:58:59Z

We've got a cargo fmt failure now:

cargo fmt -- --check
Diff in /home/runner/actions-runner/_work/kata-containers/kata-containers/src/github.com/kata-containers/kata-containers/src/agent/src/rpc.rs at line 147:
     }
 }
 
-
 #[cfg(feature = "agent-policy")]
 async fn policy_allows(req: &(impl MessageDyn + serde::Serialize)) -> ttrpc::Result<()> {
     let request = serde_json::to_string(req).unwrap();

but that's better than the code-gen problem. @mkulke - can you remove that extra line as well, so we can re-try again? Thanks!

stevenhorsman · 2023-12-15T14:06:16Z

/test

stevenhorsman · 2023-12-15T14:32:58Z

One of the unit tests failed with a race condition - re-running now to see if that helps

mkulke · 2023-12-15T15:47:30Z

error[E0425]: cannot find value `VERSION_3_2_0` in crate `protobuf`
  --> /home/runner/actions-runner/_work/kata-containers/kata-containers/src/github.com/kata-containers/kata-containers/src/libs/protocols/src/agent.rs:28:49
   |
28 | const _PROTOBUF_VERSION_CHECK: () = ::protobuf::VERSION_3_2_0;
   |                                                 ^^^^^^^^^^^^^ help: a constant with a similar name exists: `VERSION_3_3_0`
   |
  ::: /home/runner/actions-runner/_work/kata-containers/kata-containers/src/github.com/kata-containers/kata-containers/src/agent/target/x86_64-unknown-linux-musl/debug/build/protobuf-0e973f31b1d98cd4/out/version.rs:7:1
   |
7  | pub const VERSION_3_3_0: () = ();
   | --------------------------- similarly named constant `VERSION_3_3_0` defined here

well, it was worth a try.

stevenhorsman · 2023-12-19T13:02:05Z

/test

stevenhorsman · 2023-12-19T13:31:52Z

The s390x tests are failing with:

13:13:34 error: failed to run custom build command for `ring v0.16.20`
13:13:34 
13:13:34 Caused by:
13:13:34   process didn't exit successfully: `/kata-containers/src/agent/target/release/build/ring-884cb6961366a545/build-script-build` (exit status: 101)
13:13:34   --- stderr
13:13:34   thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /root/.cargo/registry/src/index.crates.io-d11c229612889eed/ring-0.16.20/build.rs:358:10

I know ring didn't use to support s390x. That got added in October in briansmith/ring#1297, but 0.16.20 is ~3 years old, so it won't be in there. I know that Ding did some work to ensure that ring wasn't needed to unblock s390x builds of image-rs, has that been undone recently?

stevenhorsman · 2023-12-19T14:04:44Z

Same error in static-checks with protobuf version:

rror[E0425]: cannot find value `VERSION_3_2_0` in crate `protobuf`
  --> /home/runner/actions-runner/_work/kata-containers/kata-containers/src/github.com/kata-containers/kata-containers/src/libs/protocols/src/agent.rs:28:49
   |
28 | const _PROTOBUF_VERSION_CHECK: () = ::protobuf::VERSION_3_2_0;
   |                                                 ^^^^^^^^^^^^^ help: a constant with a similar name exists: `VERSION_3_3_0`

mkulke · 2023-12-19T14:17:23Z

I know ring didn't use to support s390x. That got added in October in briansmith/ring#1297, but 0.16.20 is ~3 years old, so it won't be in there. I know that Ding did some work to ensure that ring wasn't needed to unblock s390x builds of image-rs, has that been undone recently?

sigh, possibly. that's w/ the latest guest-components commit (4ddac):

$ cat deny.toml
[bans]
multiple-versions = "allow"
deny = [
    { name = "ring" },
]
$ cargo deny check bans
error[banned]: crate 'ring = 0.16.20' is explicitly banned
  ┌─ /home/magnuskulke/dev/kata-containers/src/agent/deny.toml:4:14
  │
4 │     { name = "ring" },
  │              ^^^^^^ banned here
  │
  = ring v0.16.20
    └── tough v0.14.0
        └── sigstore v0.8.0
            └── image-rs v0.1.0
                └── kata-agent v0.1.0

error[banned]: crate 'ring = 0.17.7' is explicitly banned
  ┌─ /home/magnuskulke/dev/kata-containers/src/agent/deny.toml:4:14
  │
4 │     { name = "ring" },
  │              ^^^^^^ banned here
  │
  = ring v0.17.7
    └── rustls-webpki v0.102.0
        └── sigstore v0.8.0
            └── image-rs v0.1.0
                └── kata-agent v0.1.0

bans FAILED

image-rs w/ rev a151bca works (contains only the ttrpc files):

cargo deny check bans
bans ok

stevenhorsman · 2023-12-19T14:28:12Z

/test

fixes kata-containers#8669 drive-by fix: go-fmt on runtime/kata_agent.go There are no tests for this feature in CCv0 branch and you should use it at your own risk. Signed-off-by: Magnus Kulke <magnuskulke@microsoft.com>

mkulke · 2023-12-19T16:51:47Z

@stevenhorsman

I also bumped protobuf to 3.3.0 on src/libs/protocol since we are also seeing version conflicts on generated source files here

stevenhorsman · 2023-12-19T17:09:45Z

/test

stevenhorsman · 2023-12-19T20:32:07Z

Finally we have the tests passing, so I'll merge this as an urgent fix to CCv0, even though this branch is largely read-only and untested.

katacontainersbot added the size/tiny Smallest and simplest task label Dec 14, 2023

mkulke force-pushed the mkulke/fix-layer-ordering branch from 0e43589 to df6d719 Compare December 14, 2023 09:25

bpradipt approved these changes Dec 14, 2023

View reviewed changes

stevenhorsman approved these changes Dec 14, 2023

View reviewed changes

mkulke force-pushed the mkulke/fix-layer-ordering branch from df6d719 to 910dee9 Compare December 15, 2023 11:15

mkulke force-pushed the mkulke/fix-layer-ordering branch from 910dee9 to 80d31d6 Compare December 15, 2023 11:23

mkulke force-pushed the mkulke/fix-layer-ordering branch from 80d31d6 to cb333f8 Compare December 15, 2023 11:37

mkulke force-pushed the mkulke/fix-layer-ordering branch from cb333f8 to b375307 Compare December 15, 2023 13:17

katacontainersbot added size/large Task of significant size and removed size/tiny Smallest and simplest task labels Dec 15, 2023

mkulke force-pushed the mkulke/fix-layer-ordering branch from b375307 to 9ed9fc5 Compare December 15, 2023 14:02

mkulke force-pushed the mkulke/fix-layer-ordering branch from 9ed9fc5 to c5d8910 Compare December 19, 2023 12:57

katacontainersbot added size/huge Largest and most complex task (probably needs breaking into small pieces) and removed size/large Task of significant size labels Dec 19, 2023

mkulke force-pushed the mkulke/fix-layer-ordering branch from c5d8910 to 997b3b5 Compare December 19, 2023 14:22

agent: bump image-rs to include layer-ordering fix

4ebac8d

fixes kata-containers#8669 drive-by fix: go-fmt on runtime/kata_agent.go There are no tests for this feature in CCv0 branch and you should use it at your own risk. Signed-off-by: Magnus Kulke <magnuskulke@microsoft.com>

mkulke force-pushed the mkulke/fix-layer-ordering branch from 997b3b5 to 4ebac8d Compare December 19, 2023 16:48

stevenhorsman added the no-backport-needed label Dec 19, 2023

stevenhorsman merged commit d0df919 into kata-containers:CCv0 Dec 19, 2023
15 of 18 checks passed

mkulke deleted the mkulke/fix-layer-ordering branch December 19, 2023 20:38

stevenhorsman mentioned this pull request Dec 22, 2023

image-rs re-introduce ring version that is incompatible with s390x? confidential-containers/guest-components#422

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump image-rs rev to include image-layer-ordering fix #8670

Bump image-rs rev to include image-layer-ordering fix #8670

mkulke commented Dec 14, 2023

stevenhorsman commented Dec 14, 2023

mkulke commented Dec 14, 2023

bpradipt left a comment

stevenhorsman left a comment

mkulke commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

mkulke commented Dec 15, 2023

mkulke commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

mkulke commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

mkulke commented Dec 15, 2023

mkulke commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

mkulke commented Dec 15, 2023

mkulke commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

mkulke commented Dec 15, 2023

stevenhorsman commented Dec 19, 2023

stevenhorsman commented Dec 19, 2023

stevenhorsman commented Dec 19, 2023

mkulke commented Dec 19, 2023

stevenhorsman commented Dec 19, 2023

mkulke commented Dec 19, 2023

stevenhorsman commented Dec 19, 2023

stevenhorsman commented Dec 19, 2023

Bump image-rs rev to include image-layer-ordering fix #8670

Bump image-rs rev to include image-layer-ordering fix #8670

Conversation

mkulke commented Dec 14, 2023

stevenhorsman commented Dec 14, 2023

mkulke commented Dec 14, 2023

bpradipt left a comment

Choose a reason for hiding this comment

stevenhorsman left a comment

Choose a reason for hiding this comment

mkulke commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

mkulke commented Dec 15, 2023

mkulke commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

mkulke commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

mkulke commented Dec 15, 2023

mkulke commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

mkulke commented Dec 15, 2023

mkulke commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

stevenhorsman commented Dec 15, 2023

mkulke commented Dec 15, 2023

stevenhorsman commented Dec 19, 2023

stevenhorsman commented Dec 19, 2023

stevenhorsman commented Dec 19, 2023

mkulke commented Dec 19, 2023

stevenhorsman commented Dec 19, 2023

mkulke commented Dec 19, 2023

stevenhorsman commented Dec 19, 2023

stevenhorsman commented Dec 19, 2023