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

feat: add the ability to manage sidecar processes #465

Merged
merged 11 commits into from
Jul 29, 2023
Merged

Conversation

jtieri
Copy link
Member

@jtieri jtieri commented Mar 29, 2023

There are cases where it is necessary to run some sidecar process on a per validator basis and/or on a chain basis. One example of this is the orchestrator needed to run a Sommelier validator. This PR introduces the concept of a SidecarProcess which can be scoped either at the chain level or at the validator level.

SidecarProcesses can optionally be started before the chain or validator node starts or explicitly started after the chain or validator is started.

Copy link
Member

@agouin agouin left a comment

Choose a reason for hiding this comment

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

This a great abstraction and will be extremely useful for chains that have additional processes.

Left a comment about potentially automatically storing new sidecars into the Chain/ChainNode slices.

It would also be great to wire up sidecar config in the ChainConfig so that they can be configured there.

chain/cosmos/chain_node.go Outdated Show resolved Hide resolved
chain/cosmos/cosmos_chain.go Outdated Show resolved Hide resolved
chain/cosmos/sidecar.go Outdated Show resolved Hide resolved
@jtieri jtieri marked this pull request as ready for review April 4, 2023 19:00
@jtieri jtieri requested a review from a team as a code owner April 4, 2023 19:00
@jtieri jtieri requested a review from vimystic April 4, 2023 19:00
@@ -42,6 +42,8 @@ type ChainConfig struct {
EncodingConfig *testutil.TestEncodingConfig
// Required when the chain uses the new sub commands for genesis (https://github.com/cosmos/cosmos-sdk/pull/14149)
UsingNewGenesisCommand bool `yaml:"using-new-genesis-command"`
// Configuration describing additional sidecar processes.
Copy link
Member

Choose a reason for hiding this comment

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

This covers chain-level sidecars, but can you include an additional slice for per-node sidecars?

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 was handling all sidcars from this single slice by using the bool field ValidatorProcess. the way I imagined this would work is people would instantiate the sidcars and add them to the slice, for any sidecar that should be scoped at a validator level ValidatorProcess will be true

Chain level sidecars are handled here

func (c *CosmosChain) initializeSidecars(
ctx context.Context,
testName string,
cli *client.Client,
networkID string,
) error {
eg, egCtx := errgroup.WithContext(ctx)
for i, cfg := range c.cfg.SidecarConfigs {
i := i
cfg := cfg
if cfg.ValidatorProcess {
continue
}
eg.Go(func() error {
err := c.NewSidecarProcess(egCtx, cfg.PreStart, cfg.ProcessName, testName, cli, networkID, cfg.Image, i, cfg.Ports, cfg.StartCmd)
if err != nil {
return err
}
return nil
})
}
if err := eg.Wait(); err != nil {
return err
}
return nil
}

and then the validator sidecars are handled here

for _, cfg := range c.cfg.SidecarConfigs {
if !cfg.ValidatorProcess {
continue
}
err = tn.NewSidecarProcess(ctx, cfg.PreStart, cfg.ProcessName, testName, cli, networkID, cfg.Image, cfg.Ports, cfg.StartCmd)
if err != nil {
return nil, err
}
}

im indifferent to the path forward here and can definitely see the benefits of making this more explicit so if you think thats a better design i can make those changes

@aljo242
Copy link

aljo242 commented Apr 10, 2023

@jtieri I think you will need to add the following logic to ChainConfig.Clone() in ibc/types.go

func (c ChainConfig) Clone() ChainConfig {
	x := c

	images := make([]DockerImage, len(c.Images))
	copy(images, c.Images)
	x.Images = images

	sidecars := make([]SidecarConfig, len(c.SidecarConfigs))
	copy(sidecars, c.SidecarConfigs)
	x.SidecarConfigs = sidecars

	return x
}

And the following to MergeChainSpecConfig()

...
	if len(other.SidecarConfigs) > 0 {
		c.SidecarConfigs = append([]SidecarConfig(nil), other.SidecarConfigs...)
	}

@jtieri
Copy link
Member Author

jtieri commented Apr 12, 2023

grazie! I'm picking this back up so will get this added in there

@aljo242
Copy link

aljo242 commented Jun 5, 2023

@jtieri any updates on this?

@jtieri jtieri requested a review from agouin July 10, 2023 22:38
Copy link
Member

@agouin agouin left a comment

Choose a reason for hiding this comment

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

🚀

@jtieri jtieri merged commit 2d8518c into main Jul 29, 2023
6 checks passed
@jtieri jtieri deleted the justin/feat-sidecars branch July 29, 2023 02:38
@boojamya
Copy link
Contributor

@Mergifyio backport v6

@mergify
Copy link
Contributor

mergify bot commented Aug 17, 2023

backport v6

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 17, 2023
* feat: add the ability to manage sidecar processes on a per chain or per validator level

* feat: add the ability to configure sidecars through chain config

* feat: add the ability to check if a container is running

* handle changes in update to docker dep

* add logic for merge and clone of sidecars

* Sidecar home dir and host ports (#665)

* Make it work with horcrux

* Add GetHostPorts

---------

Co-authored-by: Andrew Gouin <[email protected]>
(cherry picked from commit 2d8518c)

# Conflicts:
#	ibc/types.go
boojamya added a commit that referenced this pull request Aug 17, 2023
* feat: add the ability to manage sidecar processes (#465)

* feat: add the ability to manage sidecar processes on a per chain or per validator level

* feat: add the ability to configure sidecars through chain config

* feat: add the ability to check if a container is running

* handle changes in update to docker dep

* add logic for merge and clone of sidecars

* Sidecar home dir and host ports (#665)

* Make it work with horcrux

* Add GetHostPorts

---------

Co-authored-by: Andrew Gouin <[email protected]>
(cherry picked from commit 2d8518c)

# Conflicts:
#	ibc/types.go

* conflicts

---------

Co-authored-by: Justin Tieri <[email protected]>
Co-authored-by: Dan Kanefsky <[email protected]>
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.

4 participants