[Dapr Extension] Allow defining custom placement container address #767

dasiths · 2020-11-09T13:18:56Z

As referenced in #763

When running Dapr in Windows (with WSL2 backend) the Dapr placement container runs on 6050 port on the host. So it requires we do something like this in Tye.yaml (Example https://github.com/dotnet/tye/blob/master/samples/dapr/tye.yaml)

# To ensure that your are running a dapr placement container with the right binding port.
# (50005 as HostPort)
- name: placement
  image: daprio/dapr
  args: ./placement
  bindings:
    - port: 50005

Edit: With @jkotalik suggestions the PR now does the following

Creates a Dapr placement container automatically and uses it if one is not defined by you.
Allows you to instruct Tye to use a particular host port for the above.
Allows you to instruct Tye to reuse an existing Dapr placement container that's already running.

extensions:
- name: dapr

  # If you want to use a different tag or container port
  # placement-image: daprio/dapr
  # placement-container-port: 50005

  # You can instruct Tye to not create the Dapr placement container on your behalf. This is required if you have Dapr running and want to use that container.
  # Doing a `docker ps` can show if its already running. If it's running then you can set 'exclude-placement-container: true' with `placement-port: xxxx` set to the host port of that container.
  # (i.e. In Windows + WSL2, Dapr uses 6050 as the host port)

  exclude-placement-container: true
  placement-port: 6050

dnfadmin · 2020-11-09T13:19:10Z

All CLA requirements met.

jkotalik · 2020-11-09T16:36:18Z

Is the placement container something we always need to be running with dapr now? If so, I think always creating the placement service and allowing someone to specify the service to override settings may be better, rather than having placement-address.

@davidfowl thoughts?

dasiths · 2020-11-09T23:04:55Z

Is the placement container something we always need to be running with dapr now? If so, I think always creating the placement service and allowing someone to specify the service to override settings may be better, rather than having placement-address.

@davidfowl thoughts?

I'm not sure which side of the boundary the placement container instantiation ownership should sit. I feel like if it's already there created and running by Dapr we should just use it. Now that I think about it, some sort of auto detection would be a lot smarter.

jkotalik · 2020-11-12T17:45:37Z

To sum up our thoughts:

If there isn't a placement container in the tye.yaml, we should create one to be used in the dapr extension. This matches what we have for the dapr proxies.
If there is a placement container, just use it.

The only question now is auto detection of the placement container. Probably just checking for the image name daprio/dapr ./placement. Also we could add an argument to the extension called excludePlacementContainer to avoid adding it.

dasiths · 2020-11-13T01:09:19Z

If there isn't a placement container in the tye.yaml

We can then have more fine grained control by using exclude-placement-container or placement-port to reuse something running on a different port perhaps. I've updated the PR, have a look and let me know.

dasiths · 2020-11-13T15:52:00Z

@jkotalik Let's consider these scenarios

Dapr placement not running at all. So we create it and point to port 50005 (or pick port dynamically) and use it.
Dapr placement is defined in Tye.yaml. So we use it.
Dapr is already running placement container on 50005 or 6050. We need to either auto detect that and use it or allow the user to specify that in Tye.yaml (My original idea with placement-address).

If you can verify my assumptions are correct I can try and make the necessary changes.

dasiths · 2020-11-17T14:14:05Z

@jkotalik @davidfowl I've updated the PR to cover the scenarios captured in the discussion and also included a sample app to demonstrate it.

I can move the sample app to a separate PR if required. The magic in this PR happens in the src/Microsoft.Tye.Extensions/Dapr/DaprExtension.cs

The build was successful till my last commit but it's broken now due to something else in the pipeline not related to this PR. Once that is fixed the PR will pass the build and tests.

Please let me know if there are any more changes required. :)

jkotalik · 2020-11-17T19:22:41Z

@dasiths yes I think you assumptions were correct! Apologies for not confirming earlier.

jkotalik · 2020-11-17T19:29:43Z

@dasiths yes I think you assumptions were correct! Apologies for not confirming earlier.

Looks like potentially our format checker is affected by the set_env issue in github actions. I'll look into that.

mtaghavi2005 · 2020-11-17T19:39:35Z

src/Microsoft.Tye.Extensions/Dapr/DaprExtension.cs

+                var isCustomPlacementPort = false;
+
+                var existingDaprPlacementDefinition = context.Application.Services.FirstOrDefault(s =>
+                    s is ContainerServiceBuilder serviceBuilder && serviceBuilder.Image == "daprio/dapr");


In case of using ImageName + Tag, placement definition cannot be found.

The more I think about this, the more I don't like sniffing to determine if the placement port is already defined or not.

I think including an option called excludePlacementService, which default to false, is enough to have people either include or exclude it as required.

I have removed sniffing. It will now use an auto assigned port and be agnostic about the user defined services. The only way to control the port is via the placement-port property.

jkotalik · 2020-11-17T19:59:32Z

src/Microsoft.Tye.Extensions/Dapr/DaprExtension.cs

@@ -20,12 +20,67 @@ public override Task ProcessAsync(ExtensionContext context, ExtensionConfigurati

            if (context.Operation == ExtensionContext.OperationKind.LocalRun)
            {
+                // default placement port number
+                var placementPort = 50005;


Can this be a random port instead of a specific port?

It will now use the next available port, same as the other parts of the system.

jkotalik · 2020-11-17T20:00:18Z

src/Microsoft.Tye.Extensions/Dapr/DaprExtension.cs

+                            Bindings = {
+                                new BindingBuilder() {
+                                    Port = placementPort,
+                                    ContainerPort = 50005,


Is the container port always supposted to be 50005 for the placement?

Yes it's defined by Dapr. I did however introduce a new property called placement-container-port to allow the developer to set it if required. (i.e. If Dapr releases a new image with a new port). Similarly I now have a placement-image which allows the developers to define a custom image name if required. (i.e. daprio/dapr:edge)

jkotalik · 2020-11-17T20:03:07Z

src/Microsoft.Tye.Extensions/Dapr/DaprExtension.cs

+                else
+                {
+                    // use the port defined in the custom service definition is possible
+                    var definedPlacementPort = existingDaprPlacementDefinition.Bindings.FirstOrDefault(b => b.Port.HasValue);


Based on my suggestion earlier, I think if someone sets excludePlacementService, they must define placement-port so we know what that is.

Yes I added this check now. It will ignore exclude-placement-service if not placement-port has been defined.

jkotalik · 2020-11-17T20:03:19Z

Sample looks great FYI.

mtaghavi2005 · 2020-11-17T20:12:03Z

src/Microsoft.Tye.Extensions/Dapr/DaprExtension.cs

@@ -51,12 +106,12 @@ public override Task ProcessAsync(ExtensionContext context, ExtensionConfigurati

                        // These environment variables are replaced with environment variables
                        // defined for this service.
-                        Args = $"-app-id {project.Name} -app-port %APP_PORT% -dapr-grpc-port %DAPR_GRPC_PORT% --dapr-http-port %DAPR_HTTP_PORT% --metrics-port %METRICS_PORT% --placement-address localhost:50005",
+                        Args = $"-app-id {project.Name} -app-port %APP_PORT% -dapr-grpc-port %DAPR_GRPC_PORT% --dapr-http-port %DAPR_HTTP_PORT% --metrics-port %METRICS_PORT% --placement-address localhost:{placementPort}",


That would be great this issue #802 also could be addressed here.

Should be --placement-host-address, instead of --placement-address

@mtaghavi2005 @thangchung I incorporated this change here. Should now support Dapr rc1. :)

dasiths · 2020-11-18T13:23:43Z

@jkotalik I have addressed all of the comments. Please let me know if you're happy with this revision. Cheers.

Edit: There is something fishy happening with the build agents while running tests. The OSX one passed, Windows one is now hanging for 30 mins and Ubuntu one failed. There aren't any tests that are affected by my change. Weird.

https://dev.azure.com/dnceng/public/_build/results?buildId=890711&view=ms.vss-test-web.build-test-results-tab&runId=28487644&resultId=100015&paneView=debug

dasiths · 2020-11-26T14:09:08Z

@jkotalik any feedback about this?

jkotalik · 2020-12-01T16:58:48Z

/azp run

azure-pipelines · 2020-12-01T16:58:57Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotalik · 2020-12-01T17:00:26Z

src/Microsoft.Tye.Hosting/PortAssigner.cs

                foreach (var binding in service.Description.Bindings)
                {
                    // Auto assign a port
                    if (binding.Port == null)
                    {
-                        binding.Port = GetNextPort();
+                        binding.Port = NextPortFinder.Instance.GetNextPort();


Why have NextPortFinder as an instance rather than just a static method?

I followed the pattern from the GitDetector class but more than happy to make it static as it doesn't have any dependencies or logic in the constructor.

jkotalik

LGTM besides a few nits.

jkotalik · 2020-12-01T17:02:13Z

src/Microsoft.Tye.Core/NextPortFinder.cs

@@ -0,0 +1,26 @@
+using System.Net;


nit copyright

jkotalik · 2020-12-01T17:02:36Z

@dasiths apologies, holidays in the states 😄

dasiths · 2020-12-02T00:18:34Z

@dasiths apologies, holidays in the states 😄

No worries. Hope you had a good break Justin. Looking forward to holidays myself. It's another sweltering 40c December awaiting us in downunder. 😉

I've updated the PR with the changes now.

dasiths · 2020-12-11T06:27:55Z

@jkotalik Anything else required here?

thangchung · 2021-01-05T05:03:33Z

How was it going on, guys? Look like tye wasn't upgrade accordingly with dapr rc right away so that it makes it hard to use dapr with tie respectively :(

jkotalik · 2021-01-05T19:23:08Z

Thanks @dasiths!

dasiths requested review from davidfowl, jkotalik, JunTaoLuo and rynowak as code owners November 9, 2020 13:18

dasiths mentioned this pull request Nov 9, 2020

Allow Dapr placement container address to be defined in Tye.yaml #768

Open

dasiths changed the title ~~[Dapr Extension] Allow defining custom placement container address~~ [WIP][Dapr Extension] Allow defining custom placement container address Nov 13, 2020

dasiths changed the title ~~[WIP][Dapr Extension] Allow defining custom placement container address~~ [Dapr Extension] Allow defining custom placement container address Nov 15, 2020

mtaghavi2005 reviewed Nov 17, 2020

View reviewed changes

jkotalik reviewed Nov 17, 2020

View reviewed changes

mtaghavi2005 reviewed Nov 17, 2020

View reviewed changes

This was referenced Nov 18, 2020

Dapr Extension not working with Dapr v1.0.0-rc.1 #802

Closed

Refactored ConfigFileFinder.cs #786

Merged

jkotalik reviewed Dec 1, 2020

View reviewed changes

jkotalik approved these changes Dec 1, 2020

View reviewed changes

jkotalik reviewed Dec 1, 2020

View reviewed changes

dasiths added 10 commits December 2, 2020 11:06

allow defining custom placement container address

07cb48a

remove port inferring logic

3ac6c94

Spin up dapr placement container automatically

aa56c46

reorder samples

f2b084a

fix argument format

f2ab5c4

Update documentation

1f0c2c3

rename folder

6e80a2e

update sample to include a node app with dockerfile

4f3e2ac

Auto assign placement host port and remove sniffing

abb10b1

next port finder made static

ac1e237

dasiths force-pushed the feature/allow-custom-placement-address branch from c1f45cc to ac1e237 Compare December 2, 2020 00:13

jkotalik merged commit 45bf552 into dotnet:master Jan 5, 2021

dasiths deleted the feature/allow-custom-placement-address branch January 6, 2021 03:29

philliphoff mentioned this pull request Sep 30, 2021

Tye seems to start an adjacent Dapr placement service #1175

Closed

[Dapr Extension] Allow defining custom placement container address #767

[Dapr Extension] Allow defining custom placement container address #767

Conversation

dasiths commented Nov 9, 2020 • edited Loading

dnfadmin commented Nov 9, 2020 • edited Loading

jkotalik commented Nov 9, 2020

dasiths commented Nov 9, 2020

jkotalik commented Nov 12, 2020

dasiths commented Nov 13, 2020 • edited Loading

dasiths commented Nov 13, 2020

dasiths commented Nov 17, 2020 • edited Loading

jkotalik commented Nov 17, 2020

jkotalik commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasiths Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotalik commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasiths commented Nov 18, 2020 • edited Loading

dasiths commented Nov 26, 2020

jkotalik commented Dec 1, 2020

azure-pipelines bot commented Dec 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotalik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotalik commented Dec 1, 2020

dasiths commented Dec 2, 2020

dasiths commented Dec 11, 2020

thangchung commented Jan 5, 2021

jkotalik commented Jan 5, 2021

dasiths commented Nov 9, 2020 •

edited

Loading

dnfadmin commented Nov 9, 2020 •

edited

Loading

dasiths commented Nov 13, 2020 •

edited

Loading

dasiths commented Nov 17, 2020 •

edited

Loading

dasiths Nov 18, 2020 •

edited

Loading

dasiths commented Nov 18, 2020 •

edited

Loading