balena-io-modules / balena-compose

Complete toolkit for building docker-compose.yml files and optionally deploy them on balenaCloud
Apache License 2.0
8 stars 1 forks source link

Fix release models compile, and test failure from dependency regression #11

Closed kb2ma closed 2 years ago

kb2ma commented 2 years ago

See issue below for details on compilation failure. Also includes a cosmetic fix from prettify.

This fix minimally narrows the body input parameters by keeping them generics. It would be simpler to directly describe the body parameters as AnyObject without the generics, but I don't have enough context to know if that solution is better.

The test failure is due to a regression in dockerode v3.3.4, as described in this issue. We have proposed a fix PR. While waiting for a response, this PR pins dockerode to v3.3.3.

Fixes #10 , #15

kb2ma commented 2 years ago

Thanks for the review, @dfunckt . As you can see below the CI tests are failing, and I can recreate locally. I see a lot of timeout failures. Below is the first example. I'm new to this code. Does anything jump out at you as the cause?

  Directory build
    1) should build a directory image

  0 passing (1m)
  1 failing

  1) Directory build
       should build a directory image:
     Error: Timeout of 60000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/kbee/dev/balena-compose/repo/test/build/all.spec.ts)
      at listOnTimeout (internal/timers.js:557:17)
      at processTimers (internal/timers.js:500:7)
kb2ma commented 2 years ago

@dfunckt , with a pointer from @pipex, I've found the compilation problem is due to a stricter check in TypeScript 4.8 -- Unconstrained Generics No Longer Assignable to {}. In this case the body param is a Params object in which all members are optional. So, the new constraint ensures the passed in object is not null.

Still need to fix the test failures.

kb2ma commented 2 years ago

Tests fail with dependency dockerode v3.3.4; succeed with v3.3.3. Now need to understand why.

kb2ma commented 2 years ago

@pipex, @dfunckt : I understand more about the test failure, but not enough to fix it. The problem is with the code below, from dockerode's docker.js, new in v3.3.4. I have added a couple of console.log() statements, and show their output below when run from a failing balena-compose test.

 if (callback === undefined) {
    const prepareCtxPromise = new self.modem.Promise(function(resolve, _) {
      util.prepareBuildContext(file, resolve);
      console.log(`file: ${JSON.stringify(file)}`);
    });

    return prepareCtxPromise.then((ctx)=> {
      console.log(`ctx: ${JSON.stringify(ctx)}`);
      return new self.modem.Promise(function(resolve, reject) {
        optsf.file = ctx;
        self.modem.dial(optsf, function(err, data) {
          if (err) {
            return reject(err);
          }
          resolve(data);
        });
      });
    })
  } else {
    util.prepareBuildContext(file, (ctx) => {
      optsf.file = ctx;
      self.modem.dial(optsf, function(err, data) {
        callback(err, data);
      });
    })
  }

Log output below. Notice in the ctx: line that the object has false for the readable and writeable properties. So, something happens after resolving prepareCtxPromise.

file: {"_events":{"end":[null,null,null],"close":[null,null],"error":[null,null]},"_eventsCount":5,"writable":true,"readable":true,"paused":false,"autoDestroy":true}

ctx: {"_events":{"end":[null,null,null],"close":[null,null],"error":[null,null]},"_eventsCount":5,"writable":false,"readable":false,"paused":false,"autoDestroy":true}

Strictly speaking, the creation of prepareCtxPromise is not essential. Here is a modified version without the separate prepareCtxPromise, and this works fine from the balena-compose tests.

However, I think the problem is that balena-compose is using the file/ctx object in a way that dockerode's buildImage() function does not expect. balena-compose passes a Duplex type via event-stream, as you can see here to mimic (?) the expected ReadableStream. We don't push data into the stream until later.

So, at the moment the problem is with the way that balena-compose uses dockerode. Is dockerode being too strict in what they expect from a ReadableStream?

dfunckt commented 2 years ago

@kb2ma DuplexStream is a ReadableStream and it should be accepted. Without digging deep into the details, it appears to be a regression in Dockerode introduced by that PR.

pipex commented 2 years ago

The latest bump to dockerode also bumped docker-modem, with this fix https://github.com/apocas/docker-modem/commit/b3f33cd565a1b6014db44dcd55108fb1bf53352a for the following issue https://github.com/apocas/docker-modem/issues/142. I wonder if balena-compose was relying on the callback being triggered multiple times.

One test you might want to do @kb2ma is to downgrade just docker-modem under node_modules/dockerode and test again.

dfunckt commented 2 years ago

I wonder if balena-compose was relying on the callback being triggered multiple times.

I can't think of any reason that would make invoking the callback more than once desirable, so I don't think this is the case.

kb2ma commented 2 years ago

Thanks for the suggestion to downgrade docker-modem @pipex. I agree that the changes in v3.0.6 look like they might be relevant. However, I tested with docker-modem v3.0.3 through v3.0.5, and none of them worked either.

My approach at this point is that there is an issue with the extra layer of promise in the prepareCtxPromise variable shown above that does not support the balena-compose use of buildImage() with a Duplex as a ReadableStream. I will try to create a simple test to recreate the failure for the dockerode repo.

kb2ma commented 2 years ago

Creating simple example in balena-io-playground/dockerode-build-image-hang repo.

kb2ma commented 2 years ago

Created an issue / fix for Dockerode to resolve test failures.

kb2ma commented 2 years ago

I'd prefer to use the slightly more relaxed extends

Will do. Thanks for the context check.

kb2ma commented 2 years ago

Squashed changes