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

[WIP] Add dev mode config option #27

Closed
wants to merge 28 commits into from
Closed

[WIP] Add dev mode config option #27

wants to merge 28 commits into from

Conversation

jt-nti
Copy link
Member

@jt-nti jt-nti commented May 17, 2022

Signed-off-by: James Taylor jamest@uk.ibm.com

jt-nti and others added 27 commits March 29, 2022 11:40
Signed-off-by: James Taylor <jamest@uk.ibm.com>
Includes basic build command implementation

Signed-off-by: James Taylor <jamest@uk.ibm.com>
Signed-off-by: James Taylor <jamest@uk.ibm.com>
Build k8s Fabric peer
Signed-off-by: James Taylor <jamest@uk.ibm.com>
Signed-off-by: James Taylor <jamest@uk.ibm.com>
Add script to create k8s chaincode packages and update readme with details

Signed-off-by: James Taylor <jamest@uk.ibm.com>
The initial run implementation is just based on the k8s API in-cluster example to figure out any issues

Signed-off-by: James Taylor <jamest@uk.ibm.com>
Also fix core.yaml and add namespace to pod list

Signed-off-by: James Taylor <jamest@uk.ibm.com>
Not a real chaincode deployment yet!

Signed-off-by: James Taylor <jamest@uk.ibm.com>
The system:serviceaccount:test-network:default service account needs to be able to create a deployment

Signed-off-by: James Taylor <jamest@uk.ibm.com>
Signed-off-by: James Taylor <jamest@uk.ibm.com>
Expand instructions for using the builder with the k8s test network

Signed-off-by: James Taylor <jamest@uk.ibm.com>
- Use env vars for peer ID, and k8s namespace
- Use local kubeconfig if specified via env var

Signed-off-by: James Taylor <jamest@uk.ibm.com>
Signed-off-by: James Taylor <jamest@uk.ibm.com>
There are lots of hacks but it just about works!

Signed-off-by: James Taylor <jamest@uk.ibm.com>
Should default to using the test-network namespace in the k8s test network

The namespace can be set explicitly using the KUBE_NAMESPACE environment variable and will default to “default” if neither is set

Also clean up usage docs

Signed-off-by: James Taylor <jamest@uk.ibm.com>
Signed-off-by: James Taylor <jamest@uk.ibm.com>
Signed-off-by: James Taylor <jamest@uk.ibm.com>
Includes readme update to add advantages of k8s builder

Signed-off-by: James Taylor <jamest@uk.ibm.com>
The peer only wants to start the chaincode once but gets two connections due to “Replicas” being set to 2, which causes an error in the peer log

Signed-off-by: James Taylor <jamest@uk.ibm.com>
An updated script is available in a new GitHub Action repository

Signed-off-by: James Taylor <jamest@uk.ibm.com>
Make use of the prebuilt conga-nft-contract sample chaincode package to simplify chaincode deployment

Signed-off-by: James Taylor <jamest@uk.ibm.com>
The peer is still managing the chaincode lifecycle so a deployment probably doesn’t make sense

Signed-off-by: James Taylor <jamest@uk.ibm.com>
Image tags can change so it’s safer to use the digest instead

Signed-off-by: James Taylor <jamest@uk.ibm.com>
Signed-off-by: James Taylor <jamest@uk.ibm.com>
// Ignore the image digest in dev mode to allow chaincode which has not been pushed to
// a registry to start
// TODO log a suitable health warning!
chaincodeImage = imageData.Name + ":latest"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the :latest attribute in dev mode. This is an incorrect convention and has impacts on how Kube loads the image from the local system.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought Kubernetes would just default to latest anyway, but just wanted to make it explicit.

If you don't specify a tag, Kubernetes assumes you mean the tag latest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Kube image labels and pull policy are subtly intertwined. "no tag" does not have the same behavior as "latest." This is a common assumption but it is incorrect.

See the kube docs for the gory details.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. This seems like a good explanation of how those gory details relate to loading images in kind...

https://kind.sigs.k8s.io/docs/user/quick-start/#loading-an-image-into-your-cluster

// a registry to start
// TODO log a suitable health warning!
chaincodeImage = imageData.Name + ":latest"
imagePullPolicy = apiv1.PullAlways
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the "Always" image pull policy. In dev mode the user must be able to specify IfNotPresent.

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the use case for specifying IfNotPresent in a development mode?

Copy link
Contributor

@jkneubuh jkneubuh left a comment

Choose a reason for hiding this comment

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

James the debug mode may help, but will bring other challenges in current trim (It needs some tweaks on the assumptions of image pull and latest tag to work.) I understand you are advancing the requirement to bind the package chaincode to an immutable image layer. But I really think this is setting the bar “too high” and forcing users into an area that will be uncomfortable on the usability front. This is “too much” of a requirement that everyone run by digest, IMO. For instance, the documentation trail is deceptively simple, making the assumption that users can infer the digest for a given build. In practice, it took me quite a while to track down and understand the logic embedded into the GitHub action responsible for generating the digest, (then a bit of dismay when I found that the digests are only applied by the container registry.)

Please reconsider extending the flexibitily provided by the Kube image declaration and conventions, allowing for both image digest and label+pullpolicy coordinates. Don’t get me wrong. I LOVE this builder feature (pinning by digest). I really want everyone to LOVE it as well, not grapple with Docker and CI pipelines and Kube loading mechanics and … who knows what else will come up in the wild. Kube has solved ALL of the image loading scenarios that have come up in practice… why are we headed off in the weeds on this one?

@jkneubuh
Copy link
Contributor

Use of the following options will allow BOTH "dev mode" and "production / digest" mode.

For "dev mode" we need to be able to specify both the image tag and pull policy in the image descriptor.

{
  image:  <container-registry-url>/<image-path>[ : tag | @ digest ]
  imagePullPolicy : [ Always | IfNotPresent | Never ]
  imagePullSecrets : [ .... array ]
}

@jt-nti
Copy link
Member Author

jt-nti commented May 17, 2022

I don't think I've documented it anywhere yet, however the contents of the chaincode package should only contain details about the chaincode "inside" the package, and not any deployment configuration. In this case the chaincode is not actually inside the package, but in order not to break the Fabric lifecycle, the contents must uniquely identify the chaincode that will run. A digest does that, but a mutable tag does not. Unfortunately that definitely causes problems for chaincode which hasn't been pushed to a repository, but hopefully a "dev mode" could work for that use case.

@jkneubuh
Copy link
Contributor

If the chaincode package only describes the image coordinates, we should definitely take imagePullSecrets out of the image descriptor. This can be set as an attribute of the service account running the peer / chaincode pod. This will help to disassociate the package from the deployment env / target.

Signed-off-by: James Taylor <jamest@uk.ibm.com>
@jkneubuh
Copy link
Contributor

@jt-nti IF we advance the "dev mode" alternative can we change the declaration / interpretation of "DEV_MODE" to enable additional runtimes:

  • Introduce a feature gate in the builder ENV based on REQUIRE_IMAGE_DIGEST (default: true).

  • For REQUIRE_IMAGE_DIGEST=true (or anything other than the literal falseor 0):

{
  "Name" : <container-registry>/<image-path>,
  "digest" : <image-digest> (REQUIRED)
}

builder will create a CC pod with spec:

spec: 
  container:
    image: <container-registry>/<image-path> @ <image-digest>
  • For REQUIRE_IMAGE_DIGEST=false :
{
  "Name" : <container-registry>/<image-path>,
  "digest" : <image-digest> (OPTIONAL),
  "label" : <image-label> (OPTIONAL),
  "imagePullPolicy": [ Always | IfNotPresent | Never ]  (OPTIONAL)
}
  • If digest is specified, it trumps label + imagePullPolicy in all circumstances with @ in the container spec.

  • imagePullPolicy is IGNORED (or better: an error) when REQUIRE_IMAGE_DIGEST is enabled.

  • label is IGNORED (or better: an error) when REQUIRE_IMAGE_DIGEST is enabled.

  • label is applied to the kube spec.container.image ONLY if present in the image.json AND digest is missing.

  • imagePullSecrets will be applied to the Kube service account responsible for running the peer.

The above dials are all required to run "debug mode" across all of the kube / dev platforms. Any hard-coding of conventions will break something in one of the other environments.

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.

None yet

2 participants