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

implement grid-compose #1130

Open
wants to merge 22 commits into
base: development
Choose a base branch
from
Open

Conversation

eyad-hussein
Copy link

@eyad-hussein eyad-hussein commented Jul 24, 2024

Description

Building the grid-compose cli tool by implementing several commands adapted by docker-compose(up, down, version, etc)

Changes

  • ps
  • up
  • down
  • version

Related Issues

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstring

@eyad-hussein eyad-hussein changed the title implementation of a docker-compose-like cli interface for managing deployment of services simultaneously implementation of a docker-compose-like cli for managing deployment of services simultaneously Jul 24, 2024
rootCmd.AddCommand(downCmd)
}

type App struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not the right place, should be in internal/app.go or similar

Specs types.Specs
}

func NewApp(net, mne, filePath string) (App, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

factory function should be in internal/app.go and use better names eg mnemonic instead of mne

}, nil
}

func LoadSpecsFromFile(filePath string) (types.Specs, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

create a loadSpecsFromReader with io.Reader and don't bother about paths, opening/closing the files.

Copy link
Contributor

Choose a reason for hiding this comment

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

and of course, not the right place, internal/app.go

return specs, nil
}

func Execute() {
Copy link
Contributor

Choose a reason for hiding this comment

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

move this to the beginning of the file


var (
app App
configFile string
Copy link
Contributor

Choose a reason for hiding this comment

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

configPath

Short: "Grid-Compose is a tool for running multi-vm applications on TFGrid defined using a Yaml formatted file.",
PersistentPreRun: func(cmd *cobra.Command, args []string) {
var err error
app, err = NewApp(network, mnemonic, configFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

if app, err := ... ; err != nil {
log.Fatal ...
}

},
}

func up(ctx context.Context) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

move that into internal/app.go and make it a method

Copy link
Contributor

Choose a reason for hiding this comment

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

and same for the rest..

}

for key, val := range results {
fmt.Printf("%s vm addresses:\n", key)
Copy link
Contributor

Choose a reason for hiding this comment

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

why mixing logging with printing?

"github.com/threefoldtech/zos/pkg/gridtypes/zos"
)

func GetProjectName(key string, twinId uint32) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

method on the app

return fmt.Sprintf("compose/%v/%v", twinId, key)
}

func GetVmAddresses(vm workloads.VM) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

move into vmaddresses.go in internal

return res
}

func GetRandomMyceliumIPSeed() ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

move into internal/mycelium_seed.go

type Service struct {
Flist string `yaml:"flist"`
Entrypoint string `yaml:"entrypoint,omitempty"`
Environment KVMap `yaml:"environment"`
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for the new type here map[string]string, specially if you don't have any new methods reacting on that type and doesn't provide any type safety


type Service struct {
Flist string `yaml:"flist"`
Entrypoint string `yaml:"entrypoint,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 required

Test []string `yaml:"test"`
Interval string `yaml:"interval"`
Timeout string `yaml:"timeout"`
Retries int `yaml:"retries"`
Copy link
Contributor

Choose a reason for hiding this comment

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

think of the type used, is that retries going to be a negative at anypoint? use uint or uint8 instead

}

type HealthCheck struct {
Test []string `yaml:"test"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that a test command or what exactly?add a comment next to it


type KVMap map[string]string

func (m *KVMap) UnmarshalYAML(value *yaml.Node) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

projectName := internal.GetProjectName(key, app.Client.TwinID)
log.Info().Str("projectName", projectName).Msg("canceling deployments")
if err := app.Client.CancelByProjectName(projectName); err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

wrap the error here was e.g fmt.Errorf("failed to cancel project %s: %w", projectName, err)

Copy link
Contributor

Choose a reason for hiding this comment

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

move to to a method on App struct in app.go

},
}

func down() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

move to a method on App struct in internal/app.go

},
}

func ps(ctx context.Context) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

move to a method on the app

// try to get contracts with the old project name format "<name>"
contracts, err := t.ContractsGetter.ListContractsOfProjectName(projectName, true)
if err != nil {
return workloads.Deployment{}, err
Copy link
Contributor

Choose a reason for hiding this comment

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

consider returning a wrapper error with more context

"github.com/spf13/cobra"
)

var version = "v0.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -0,0 +1,70 @@
package types

Copy link
Contributor

Choose a reason for hiding this comment

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

All of these fields require validation e.g valid flist url, positive disk space, valid node id, depends_on a service and so on.

}

func GetVmAddresses(vm workloads.VM) string {
var res string
Copy link
Contributor

Choose a reason for hiding this comment

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

use a string Builder for this https://pkg.go.dev/strings#Builder

@xmonader
Copy link
Contributor

Please review the project structure, cobra commands should ONLY invoke methods defined on the app, don't leak any code logic into your commands. make sure the input is validated with decent error messages as well.

@xmonader
Copy link
Contributor

xmonader commented Aug 4, 2024

still needs to handle the dependency resolution "and guard against cycles"

grid-compose/internal/utils.go Fixed Show fixed Hide fixed
grid-compose/internal/utils.go Fixed Show fixed Hide fixed
grid-compose/internal/utils.go Fixed Show fixed Hide fixed
grid-compose/internal/utils.go Fixed Show fixed Hide fixed
Copy link
Contributor

@Omarabdul3ziz Omarabdul3ziz left a comment

Choose a reason for hiding this comment

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

good job ya eyad

i have some comments on the project structure

},
}

func init() {
Copy link
Contributor

@Omarabdul3ziz Omarabdul3ziz Aug 15, 2024

Choose a reason for hiding this comment

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

move registering commands to the init() in root.go


func init() {
psCmd.PersistentFlags().BoolP("verbose", "v", false, "all information about deployed services")
psCmd.PersistentFlags().StringP("output", "o", "", "output result to a file")
Copy link
Contributor

Choose a reason for hiding this comment

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

we can drop this and simply use bash redirecting. grid-compose ps > dep.json

Run: func(cmd *cobra.Command, args []string) {
flags := cmd.Flags()

if err := app.Ps(cmd.Context(), flags); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

invoke the flags here, and pass to the method only the values it needs


var (
app *internal.App
configPath string
Copy link
Contributor

Choose a reason for hiding this comment

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

to avoid the global variable, we could add the app to the context. it will shared between the commands

}

func init() {
network = os.Getenv("NETWORK")
Copy link
Contributor

Choose a reason for hiding this comment

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

move getting the env to the Run in rootCmd

@@ -0,0 +1,372 @@
package internal
Copy link
Contributor

Choose a reason for hiding this comment

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

a better structure for this file could be a separate package internal/app have the app.go file for the app struct & factory method. and a file for each command with its utils ps.go, down.go, ...


// App is the main application struct that holds the client and the config data
type App struct {
Client *deployer.TFPluginClient
Copy link
Contributor

Choose a reason for hiding this comment

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

is it needed to be public?

// App is the main application struct that holds the client and the config data
type App struct {
Client *deployer.TFPluginClient
Config *config.Config
Copy link
Contributor

Choose a reason for hiding this comment

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

i think it is not used anywhere? if so remove it


if !verbose {
if !dlAdded {
output.WriteString(fmt.Sprintf("%-15s | %-15s | %-15s | %-15s | %-10s | %s\n", contractDlData.Name, vm.NetworkName, vm.Name, vm.Mounts[0].DiskName, wl.Result.State, utils.GetVmAddresses(vm)))
Copy link
Contributor

Choose a reason for hiding this comment

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

could this formatting be abstracted? get the result as a map/array then pass it to some method maybe in pkg/logging/print_resutl.go it will be easier to make changes or even use a different pkg for formatting

}

// freeCRU is not in NodeFilter?
var freeMRU, freeSRU uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

yes freeCRU is not a filter. due to cores overprovisioning mulitple vms can use the same cpu cores, use totalCRU instead just making sure the node has the cores you need

eyad-hussein and others added 3 commits August 21, 2024 05:20
tests: test each example manually

docs: populate readme file, add future_work, add config.md, and extend cases
…dependency_resolution

implementation of a docker-compose-like cli for managing deployment of services simultaneously
@eyad-hussein eyad-hussein changed the title implementation of a docker-compose-like cli for managing deployment of services simultaneously implementation grid-compose Aug 22, 2024
@eyad-hussein eyad-hussein changed the title implementation grid-compose implement grid-compose Aug 22, 2024
@xmonader
Copy link
Contributor

Let's unblock this

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.

3 participants