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

enhance dapr run to run multiple applications #6

Merged
merged 4 commits into from
Dec 2, 2022

Conversation

mukundansundar
Copy link
Contributor

Signed-off-by: Mukundan Sundararajan 65565396+mukundansundar@users.noreply.github.com

Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
@mukundansundar
Copy link
Contributor Author

mukundansundar commented Nov 18, 2022

Open Question

Should dapr compose be renamed to something else?
Say project?

So the command becomes

dapr project up

We will have a project configuration file defaulting to dapr.yaml name in pwd.


Users developing different apps using dapr will have different resources/configurations that they use per application. Each time the user has to run the application with a particular config and resources directory value, they have to override the flag for `dapr run` command.

Instead the following convention is proposed for loading the resources/config for an application.

Choose a reason for hiding this comment

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

Since this is about usage during development, if an app is part of a repo, too many transient files will be created. It would be good to give the user an option to set the root directory via an ENVIRONMENT VARIABLE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain this part @akhilac1?


### Constraints

`compose` should mimic what `run` does for the most part. The defaults should not be different from `run` command with any new configuration options having sane documented least permissions based defaults.

Choose a reason for hiding this comment

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

Assuming default values will be provided for attributes like DAPR_HTTP_PORT, what is the mechanism for the user to identify this port, so that they could execute a HTTP request etc.? It would be good to document user experience for interacting with daprd launched with compose

Copy link
Member

Choose a reason for hiding this comment

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

We should be mounting these env vars on the user process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both DAPR_HTTP_PORT and DAPR_GRPC_PORT will be passed as env vars. I do not think metrics is port is needed inside the app, that can be part of the dapr compose output?


The logs will be as shown above, it will be chaotic to see what application is in which state and so on.

Instead of writing to STDOUT, each application and associated `daprd` process will write the logs to `{application dir}/.dapr/logs/{app id}/app_{datetime}.log`, `{application dir}/.dapr/logs/{app id}/daprd_{datetime}.log`.

Choose a reason for hiding this comment

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

Is the user expected to look at the latest log to get the key attributes like DAPR HTTP PORT? This does not seem to be a good experience. Can you please check if key attributes required for the user to interact with Dapr and available as output of the dapr compose command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed ... That is what I was thinking as well.

@nyemade-uversky
Copy link
Contributor

@dapr/maintainers-cli Putting this on your radar for 1.10


### Recommendation for 1.10

- Initial implementation will only support Linux OS.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Initial implementation will only support Linux OS.
- Initial implementation will only support Linux OS.

This should support both MacOS and Linux OS


### Constraints

`compose` should mimic what `run` does for the most part. The defaults should not be different from `run` command with any new configuration options having sane documented least permissions based defaults.
Copy link
Member

Choose a reason for hiding this comment

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

We should be mounting these env vars on the user process.


### Why dapr CLI?

From the initial [proposal](https://github.com/dapr/community/issues/207), the solution was proposed as a seprate repo and CLI in itself. But later it was suggested to use dapr CLI itself to have a `compose` command in it instead of a having a separate CLI.
Copy link
Member

@msfussell msfussell Nov 23, 2022

Choose a reason for hiding this comment

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

Using the word compose has so many connotation to Docker Compose, it leads to think this is only about running container. Given this is about mostly running processes I would use a different word, such as dapr start which carries not baggage and is clearer IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the only concern that I have here is, we would have too many start related commands.
dapr init, dapr start, dapr run.

Also dapr start does in no way convey that it is a command to start multiple apps in a single go.

Choose a reason for hiding this comment

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

dapr up?


### Subcommands and flags for `compose` command in CLI

It is proposed to have a subcommand `up` in `compose` command to _bring up_ all the applications and daprd sidecars. Later on an extension can be added to _shutdown all_ application and sidecars with a `down` subcommand.
Copy link
Member

Choose a reason for hiding this comment

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

Why introduce the 'up' command and simply have just compose to start all the applications?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was mostly following the same way docker compose, it will be familiar to the users.

@mukundansundar
Copy link
Contributor Author

@yaron2 @artursouza @msfussell Based on the offline discussion that we had, we tentatively agreed to use dapr run -f . as the flag under which we will enable this feature and not under dapr compose. The main concern that I have with this is that, this is no longer an individual command and we will be modifying an existing command. Is that something that we want to go for? This proposal from start assumes that the command will be independent and will not affect any existing commands.

Thoughts?

@yaron2
Copy link
Member

yaron2 commented Nov 29, 2022

I'm not concerned with changing the existing command, if the -f flag targets a completely different path. I think this is an implementation detail that shouldn't be discussed here - as far as the user experience goes, dapr run -f is the desired experience we'd like to see.

@msfussell
Copy link
Member

I do like the approach of extending the existing dapr run command, which was why I questioned the need to have the up command. I suggest writing out full examples for using dapr run -f and dapr stop -f or dapr stop --all as this then builds on the current CLI commands.

@mukundansundar
Copy link
Contributor Author

I do like the approach of extending the existing dapr run command, which was why I questioned the need to have the up command. I suggest writing out full examples for using dapr run -f and dapr stop -f or dapr stop --all as this then builds on the current CLI commands.

To summarize, the proposal should be modified to

  • not have process orchestration
  • extend dapr run to support running multiple files defined in an ingested "file"
  • define the file structure for the "file"
  • define a default directory struct for defining components to use with this feature
  • output logs to a file instead of STDOUT. (Let users know of file location for logs)

@yaron2 @msfussell @artursouza
Is this correct?

@yaron2
Copy link
Member

yaron2 commented Nov 29, 2022

I do like the approach of extending the existing dapr run command, which was why I questioned the need to have the up command. I suggest writing out full examples for using dapr run -f and dapr stop -f or dapr stop --all as this then builds on the current CLI commands.

To summarize, the proposal should be modified to

  • not have process orchestration
  • extend dapr run to support running multiple files defined in an ingested "file"
  • define the file structure for the "file"
  • define a default directory struct for defining components to use with this feature
  • output logs to a file instead of STDOUT. (Let users know of file location for logs)

@yaron2 @msfussell @artursouza Is this correct?

LGTM.

Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
@mukundansundar mukundansundar changed the title add dapr compose proposal enhance dapr run to run multiple applications Nov 30, 2022
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
@yaron2
Copy link
Member

yaron2 commented Dec 1, 2022

+1 binding

1 similar comment
@mukundansundar
Copy link
Contributor Author

+1 binding

@yaron2 yaron2 merged commit f946492 into dapr:main Dec 2, 2022
@mukundansundar mukundansundar deleted the run-multiple-dapr-apps branch December 2, 2022 07:49
@msfussell
Copy link
Member

I do like the approach of extending the existing dapr run command, which was why I questioned the need to have the up command. I suggest writing out full examples for using dapr run -f and dapr stop -f or dapr stop --all as this then builds on the current CLI commands.

To summarize, the proposal should be modified to

  • not have process orchestration
  • extend dapr run to support running multiple files defined in an ingested "file"
  • define the file structure for the "file"
  • define a default directory struct for defining components to use with this feature
  • output logs to a file instead of STDOUT. (Let users know of file location for logs)

@yaron2 @msfussell @artursouza Is this correct?

Agreed. LGTM

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.

6 participants