_lifecycle ignore previous build failure during install

[wlahti]

Type of change

• Bug fix

Description

When a chaincode build previously failed while installing the chaincode, _lifecycle should ignore the cached error and attempt to rebuild the chaincode. This is because we should assume the end user knows something about why the build may succeed on retry if they're reattempting an install.

Also, this PR updates integration tests to not care about the exit status of chaincode installs since reinstalls of previously successful installs now return an error.

Related issues

FAB-17907

[wlahti]

/ci-run

[jyellick]

This is racey. What if the chaincode is currently being built, and fails? Until the done channel closes, we don't know whether the result of buildStatus.Err() is error free or simply uninitialized.

We need to wait until after the build has completed, (line 682/685)

Then, if we were not the ones who initiated the build (ie ok) and the build failed (ie Err() != nil). Then we should reset and rebuild.

[jyellick]

This still seems racey to me. If the buildStatus is non-nil, it could be that the build is still ongoing, and may yet fail, so it is not safe to read from Err() yet. We must wait until the done channel closes.

[wlahti]

Doesn't the build lock by package id prevent that? We make sure that only one chaincode per package id can be built at a given time. The first caller may have a build running but they hold the buildLock. Once the buildLock is unlocked by the first caller, the second caller grabs the lock, gets the build status, and continues from there.

[jyellick]

I see what you're saying. Since we are in the new lifecycle, there should not be other competing threads trying to race to build the chaincode, other than ourselves.

Still, I think this isn't quite right. buildStatus is never nil. So checking for non-nil is redundant. I guess I would do:

if !ok {
    // Another invocation of lifecycle has concurrently installed this same package-id
   <-buildStatus.Done() // this should always already be closed
   if buildStatus.Err() == nil {
       return nil, errors.Errorf("chaincode already successfully installed")
  }
  buildStatus, _ = ef.BuildRegistry.ResetBuildStatus(packageID)
}

[jyellick]

I would have thought to leave this case the same, but split it into two scenarios, one where it does not attempt to build because the previous build was successful, and another where the previous build failed and it does.

[jyellick]

I would probably note that it's the caller's responsibility to ensure that ResetBuildStatus is only called when an existing build status is done. This means that the caller must use external locking to ensure that the build status is not reset between the check that the status is done, and when the build status is reset.

[wlahti]

Done. I updated the comment.

[wlahti]

I added locking per package ID, as requested, to protect against concurrent install of the same chaincode. I'm not sure I love where everything lives/is initialized (particularly the BuildLocks map) but this is at least closer.

[jyellick]

This still seems racey to me. If the buildStatus is non-nil, it could be that the build is still ongoing, and may yet fail, so it is not safe to read from Err() yet. We must wait until the done channel closes.

[jyellick]

Why return a constant false here?

[wlahti]

To incorporate resetting the status into the existing flow.

We get the build status, if ok (aka build status already exists), we then check for an error and reset the build status. We then continue to the existing logic of: if not ok, build the chaincode (which is also still applicable for the initial install/build of a chaincode).

[jyellick]

If this is not satisfying an interface, which I don't think it is, I can't think of a good reason to return a parameter which is a fixed constant, and has no obvious way to ever change. Essentially, we are modifying the signature of a function in another package in a non-obvious way in order to make the code look simpler, which doesn't sit well with me.

If you really want to integrate it into the existing flow, you can always do something like:

buildStatus, ok = br.ResetBuildStatus(), false

Though I'd think it would be more readable over two lines. [edit: or, looking at that logic, I think it's simpler to ignore the 'ok' variable, as its only possible value is false when it's next checked anyway]

[wlahti]

Fixed.

[jyellick]

I see what you're saying. Since we are in the new lifecycle, there should not be other competing threads trying to race to build the chaincode, other than ourselves.

Still, I think this isn't quite right. buildStatus is never nil. So checking for non-nil is redundant. I guess I would do:

if !ok {
    // Another invocation of lifecycle has concurrently installed this same package-id
   <-buildStatus.Done() // this should always already be closed
   if buildStatus.Err() == nil {
       return nil, errors.Errorf("chaincode already successfully installed")
  }
  buildStatus, _ = ef.BuildRegistry.ResetBuildStatus(packageID)
}

[wlahti]

Let me know how it looks now. I took your most recent comment and incorporated it based on what I think you meant to type for some parts of the code.

[wlahti]

This current implementation breaks some of the existing integration tests, which rely on the DeployChaincode helper function and the fact that attempting to reinstall a chaincode that succeeded previously didn't return an error. If we think this is the correct behavior (which does match the LSCC functionality), I can update the integration tests to not install while updating the chaincode definition. We could also not return an error when a previous chaincode install succeeded.

[wlahti]

Actually, the easier "fix" (which I believe is still valid) for my previous comment about the integration tests was to simply not check the exit status on install. If the chaincode has not been successfully installed when expected to, the EnsureInstalled call which catch that condition and fail accordingly.

[jyellick]

If this is not satisfying an interface, which I don't think it is, I can't think of a good reason to return a parameter which is a fixed constant, and has no obvious way to ever change. Essentially, we are modifying the signature of a function in another package in a non-obvious way in order to make the code look simpler, which doesn't sit well with me.

If you really want to integrate it into the existing flow, you can always do something like:

buildStatus, ok = br.ResetBuildStatus(), false

Though I'd think it would be more readable over two lines. [edit: or, looking at that logic, I think it's simpler to ignore the 'ok' variable, as its only possible value is false when it's next checked anyway]

[jyellick]

Why check ok here? Either, the previous ok was false, or, we just set ok to be false, since we didn't return, and ResetBuildStatus always returns false.

[wlahti]

Excellent point. I've been looking at some of this too much and it was hard to take a step back. Fixed.

[jyellick]

Up to you, but you could do lazy map initialization here:

if ef.BuildLocks == nil {
    ef.BuildLocks = map[string]sync.Mutex{}
}

then you could skip the initialization elsewhere, but, not a requirement.

[wlahti]

I considered this but I wasn't entirely sure if it was a good practice or not. I definitely prefer this over the random empty map initialization all the way out in peer/node/start. Done.

[jyellick]

Thanks for making those changes!