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

[FAB-17374] chaincode service integration test #552

Closed

Conversation

muralisrini
Copy link
Contributor

Type of change

-Test update

Description

Adds integration test to fabric using fabric-chaincode-go change from
hyperledger/fabric-chaincode-go#19.

The test can be followed via integration/e2e/chaincode_service_test.go
which was adapted from the "basic solo.." integration test but adds
support for

  • controlling starting the chaincode
  • generating connection.json

The goal is to provide a basis for creating other chaincode service
tests in future.

Signed-off-by: muralisr srinivasan.muralidharan99@gmail.com

Adds integration test to fabric using fabric-chaincode-go change from
hyperledger/fabric-chaincode-go#19.

The test can be followed via integration/e2e/chaincode_service_test.go
which was adapted from the "basic solo.." integration test but adds
support for
* controlling starting the chaincode
* generating connection.json

The goal is to provide a basis for creating other chaincode service
tests in future.

Signed-off-by: muralisr <srinivasan.muralidharan99@gmail.com>
@muralisrini muralisrini requested a review from a team as a code owner January 25, 2020 23:12
@lindluni
Copy link
Contributor

In general we've been trying to do better at the handling of input artifacts to tests. In the case of the private keys and certificates we've been trying to generate them during the test where possible. When not possible we should at least document in the code how those certificates and keys were generated, even if they were simply generated with openssl, we should document the commands used to create them

@muralisrini
Copy link
Contributor Author

I just used existing TLS artifacts from core/comm/testdata. Didn't generate tls on the fly (with openssl or other dependencies) as this was just well understood standard TLS and not fabric specific crypto (such as MSPs). But if we are already using openssl or something in integration test suite for TLS generation (I didn't want to use cryptogen for just generating TLS) I can try and automate that process (suggest in another PR though for expediency ?)

Copy link
Contributor

@sykesm sykesm left a comment

Choose a reason for hiding this comment

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

The majority of the changes to the network should not have been made. The test implementation you are creating for this function should be fairly self-contained without modifying the network abstraction.

  • Port reservation for your chaincode server instance should be accomplished by reserving a port in your test.
  • The connection.json document should be created by your test or your external builder.
  • The template and port reservation logic need to be removed from the nwo package.
  • The certificate and key material needed for the tests should either be generated in the tests or have explicit information on how to generate them. This is especially true of TLS related assets as they do expire.
  • The chaincode looks to be another copy of SimpleChaincode. Instead of making a full duplicate, it would be preferred to simply modify the main. This would highlight the differences between chaincode as client and chaincode as server and demonstrate that the chaincode does not have to change - simply how it is launched.
  • Launching a server chaincode is the responsibility of the test and not the network. As such, there is no need to create a command for it or model it.

In summary, outside of a couple of simple changes to the nwo package (ex. short-circuit when Server is true), most code should be in a suite that is specific to chaincode-as-server. That suite should not be copying unrelated assertions from e2e but focus on the behavior and function related to external chaincode servers.

Peers []*Peer `yaml:"peers,omitempty"`
Profiles []*Profile `yaml:"profiles,omitempty"`
Templates *Templates `yaml:"templates,omitempty"`
ChaincodeServers []*Chaincode `yaml:"chaincode_servers,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not part of the network configuration but specific to a small subset of tests. There's no need to have this here.

@@ -0,0 +1,18 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

This really shouldn't be a template as it's not part of the configuration, it's part of the requirement for a single chaincode implementation.

Also, once ChaincodeServerAddress is removed from the config (since it shouldn't be there), there will be nothing to template against.

@@ -0,0 +1,263 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need yet another copy of the simple chaincode? Can't we just create a main that reuses the logic?

Just adding a subdirectory where we separate the launch from the logic (eg. integration/chaincode/simple/cmd/{server,client}) should be sufficient.

pb "github.com/hyperledger/fabric-protos-go/peer"
)

var cert string = `-----BEGIN CERTIFICATE-----
Copy link
Contributor

Choose a reason for hiding this comment

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

More statically generated fixtures. We need to stop doing this and I know I've mentioned it in the past as area we are focusing on.

If we have fixtures that are quick to generate, we prefer they are generated in the test; if complicated to generate, we fall back to go:generate commands and/or scripts. If neither of those work, we expecte explicit documentation about how they were generated.

Comment on lines +161 to +168
By("waiting for DeliverFiltered stats to be emitted")
metricsWriteInterval := 5 * time.Second
Eventually(datagramReader, 2*metricsWriteInterval).Should(gbytes.Say("stream_request_duration.protos_Deliver.DeliverFiltered."))

CheckPeerStatsdStreamMetrics(datagramReader.String())
CheckPeerStatsdMetrics(datagramReader.String(), "org1_peer0")
CheckPeerStatsdMetrics(datagramReader.String(), "org2_peer0")
CheckOrdererStatsdMetrics(datagramReader.String(), "ordererorg_orderer")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we copying all of these assertions?

Comment on lines +1555 to +1569
// ChaincodeServerRunner returns an ifrit.Runner for the specified chaincode service.
func (n *Network) ChaincodeServerRunner(c Chaincode, args []string) *ginkgomon.Runner {
srvStartCmd := commands.ChaincodeServerStart{PackageID: c.PackageID, ServerArgs: args}

cmd := NewCommand(c.Path, srvStartCmd)

return ginkgomon.New(ginkgomon.Config{
AnsiColorCode: n.nextColor(),
Name: c.PackageID,
Command: cmd,
StartCheck: `Starting chaincode .* at .*`,
StartCheckTimeout: 15 * time.Second,
})
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be part of the network; this is part of the test... The network does not launch server chaincode; server chaincode is launched and peer connects to it.

Comment on lines +1660 to +1690

// ChaincodeConnectionDir returns the path to the configuration directory for the specified
// chaincode service.
func (n *Network) ChaincodeConnectionDir(chaincode *Chaincode) string {
return filepath.Join(n.RootDir, "chaincode-connections", chaincode.ID())
}

// ChaincodeConnectionPath returns the path to the chaincode connction.json document
// for the specified chaincode service
func (n *Network) ChaincodeConnectionPath(chaincode *Chaincode) string {
return filepath.Join(n.ChaincodeConnectionDir(chaincode), "connection.json")
}

func (n *Network) GenerateChaincodeConnectionConfig(chaincode *Chaincode) {
err := os.MkdirAll(n.ChaincodeConnectionDir(chaincode), 0755)
Expect(err).NotTo(HaveOccurred())

core, err := os.Create(n.ChaincodeConnectionPath(chaincode))
Expect(err).NotTo(HaveOccurred())
defer core.Close()

t, err := template.New("peer").Funcs(template.FuncMap{
"Chaincode": func() *Chaincode { return chaincode },
}).Parse(n.Templates.ChaincodeConnectionTemplate())
Expect(err).NotTo(HaveOccurred())

time.Sleep(15 * time.Second)
pw := gexec.NewPrefixedWriter(fmt.Sprintf("[%s#core.yaml] ", chaincode.ID()), ginkgo.GinkgoWriter)
err = t.Execute(io.MultiWriter(core, pw), n)
Expect(err).NotTo(HaveOccurred())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These do not belong in the network. Your test should explicitly create the connection.json that you need.

Comment on lines +107 to +118
func BasicSoloWithChaincodeServers(ccServs ...*Chaincode) *Config {
for _, cc := range ccServs {
Expect(cc.Server).To(Equal(true))
}

config := BasicSolo()

config.ChaincodeServers = ccServs

return config
}

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for a template; this should be removed.

Core string `yaml:"core,omitempty"`
Crypto string `yaml:"crypto,omitempty"`
Orderer string `yaml:"orderer,omitempty"`
ChaincodeConnection string `yaml:"chaincode_connection,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This template is not needed.

Comment on lines +46 to +51
func (t *Templates) ChaincodeConnectionTemplate() string {
if t.ChaincodeConnection != "" {
return t.ChaincodeConnection
}
return DefaultChaincodeConnectionTemplate
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed and should not exist.

@muralisrini
Copy link
Contributor Author

@sykesm the comments are largely related to isolating chaincode service setup and management to a chaincode-service related package (and moving out of nwo). I'll take a look at how this is can be organized and in the process address the comments above today/tomorrow.

@sykesm
Copy link
Contributor

sykesm commented Jan 27, 2020

@sykesm the comments are largely related to isolating chaincode service setup and management to a chaincode-service related package (and moving out of nwo). I'll take a look at how this is can be organized and in the process address the comments above today/tomorrow.

Thanks. Do you want to pull the bump of fabric-chaincode-go out of this into a separate PR? (Alternatively, I can do that on your behalf.) Given the the PR checks passed, they don't seem to break legacy chaincode.

@muralisrini
Copy link
Contributor Author

If you can, do bump up the fabric-chaincode-go please. Thanks!

@sykesm
Copy link
Contributor

sykesm commented Jan 27, 2020

If you can, do bump up the fabric-chaincode-go please. Thanks!

#563 for master
#564 for release-2.0

Please review and merge. Thanks.

@sykesm
Copy link
Contributor

sykesm commented Feb 12, 2020

@muralisrini, it's been a couple of weeks. Any idea when you might come back to this?

Thanks.

@lindluni
Copy link
Contributor

It would be really nice to get this work done, as we march closer to a new LTS release, having the integration test would be great as people start to migrate from 1.4 LTS to 2.x LTS and start using new features, so we can make sure it functions as expected moving forward.

@lindluni
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@mastersingh24
Copy link
Contributor

I'm closing this as it's seen no activity in months

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants