Skip to content
This repository was archived by the owner on Nov 20, 2023. It is now read-only.

Tye seems to start an adjacent Dapr placement service #1175

Closed
BigMorty opened this issue Sep 22, 2021 · 6 comments · Fixed by #1191
Closed

Tye seems to start an adjacent Dapr placement service #1175

BigMorty opened this issue Sep 22, 2021 · 6 comments · Fixed by #1191
Assignees
Labels
bug Something isn't working
Milestone

Comments

@BigMorty
Copy link

Tye seems to start an adjacent placement service and set of services outside of a core Dapr run, or what Dapr list sees. This breaks the docs and interop.

Tye is explicitly creating an additional placement service, this may be something from the past that is no longer needed because daprd was used originally – Justin would know

@BigMorty BigMorty added the bug Something isn't working label Sep 22, 2021
@philliphoff
Copy link
Contributor

The original PRs that led to the by-default creation of the placement service are #763 and #767. It appears that the current "on by default" behavior was done, at least in part, to avoid having to try to detect a running placement service (and its port). However, this may have been due to the requirement of daprd that the placement server address be specified; dapr does not have such a requirement (and uses defaults in that case).

@philliphoff
Copy link
Contributor

So I tinkered with not starting a placement server by default. If you've done a standard dapr init, actors still work fine. I did run into an issue when there wasn't an existing placement service running, however (such as when using dapr init --slim). Even if Tye starts the placement service, it's not necessarily sufficient to use actors because the placement service has a dependency on a state store (typically the Redis container started by Dapr alongside the placement service). What's more, there is no default component configuration that connects the placement service with the state store in that case. So even standing up both the placement service and state store isn't enough; Tye would also have to scaffold component configurations (or assume the user has done so, which somewhat defeats the purpose).

In the end, I'm not convinced having Tye manage the placement service, even when opt-in, provides a lot of value. I'm therefore inclined to remove that altogether and just allow the user to optionally specify an alternative placement port. If the user doesn't have either the placement service and/or its state store, they can always use Tye's container configuration to create one or both, and point the Dapr extension to the (fixed) port of the binding. (I could see that improved by allowing the Dapr extension configuration be able to refer to the binding of the placement server container, to avoid dealing with fixed ports but, again, I'm not sure whether that's something typical users will do.) Going this route will also make #1177 moot.

@philliphoff
Copy link
Contributor

@BigMorty Let me know what you think.

@dasiths As the person that added the placement service creation logic, let me know if I've missed some important use case.

@philliphoff philliphoff added this to the 0.11 milestone Sep 30, 2021
@BigMorty
Copy link
Author

BigMorty commented Oct 1, 2021

I tend to agree with you @philliphoff, not sure trying to manage the placement service makes sense. If we could detect it is not running and tell the user to do a dapr init, that might be a reasonable alternative.

@dasiths
Copy link
Member

dasiths commented Oct 1, 2021

@BigMorty Let me know what you think.

@dasiths As the person that added the placement service creation logic, let me know if I've missed some important use case.

I don't fully recall but I think my change was motivated by the fact that there would be an error if the Dapr placement container was already running on that default port 6050. At the time Tye was trying to always create the Dapr placement container itself on that used port.

The behaviour I was after was exactly what you intend here I think. Which is that it works by default when Dapr is installed. Which is more likely the case when you want to use the Dapr extension in Tye.

In my example the default config should be something like

extensions:
- name: dapr
  exclude-placement-container: true
  placement-port: 6050

We didn't remove the capacity of Tye to spin up a Dapr placement container at the time though. Which is what is discussed here. I don't have a strong opinion there but I do agree that if that is the case it needs to be communicated to the user effectively so they can do the dapr init if required.

BTW I couldn't do enough investigation but when we use the Dapr extension, viewing the captured traces in Zipkin telemetry (via Tye dashboard) was broken at the time. I wasn't sure if it had anything to do with using the existing placement container or not. Might be worth a check if we are completely removing the Tye managed placement container.

@philliphoff
Copy link
Contributor

We didn't remove the capacity of Tye to spin up a Dapr placement container at the time though. Which is what is discussed here. I don't have a strong opinion there but I do agree that if that is the case it needs to be communicated to the user effectively so they can do the dapr init if required.

I think that's fair and, in #1191, I've added a check that will fail with a "you need to run dapr init" type error if Tye sees Dapr is not initialized at all. (If Tye can't tell, it will issue a warning but just cross its fingers and keep going.) There's no definitive way to know whether a given Tye application's services make use of Dapr Actors--they can be used without assistance of the SDK--so there's definitive way to determine whether the placement service is required, so we can't just check whether it's missing (as when Dapr is initialized via dapr init --slim).

@philliphoff philliphoff self-assigned this Oct 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants