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

Sets the minimum number of Fly machines to 1 #1535

Merged
merged 7 commits into from
Nov 6, 2023
Merged

Conversation

infomiho
Copy link
Contributor

@infomiho infomiho commented Oct 25, 2023

Closes #1466

It updates the fly-server.toml file by replacing a line. The technique of replacing the line has already been used to setup some other things in the client TOML file.

Signed-off-by: Mihovil Ilakovac <[email protected]>
@infomiho infomiho self-assigned this Oct 25, 2023
@Martinsos
Copy link
Member

All right, nice!

Client -> it will have 0 minimum instances -> yeah, that makes sense to me.

Server -> we set it to 1 -> that makes sense, that way we are sure jobs and other stuff works.
What if they want to set it to 0 though? Would it be hard? Do we have any instructions for that? Should we explain this to them?

Db -> it will have 0 minimum instances -> I guess that is ok, right? Could that cause any issues? Connection dropping between server and db maybe?

Btw, while this is separate issue, I thought this might be a good moment to also give a bit of thought to this one: #1342 .

@infomiho
Copy link
Contributor Author

This PR

What if they want to set it to 0 though?

They can edit the fly-server.toml and redeploy. These are just the initial values which we set once and then they can do whatever they want to the Fly's TOML file.

Do we have any instructions for that?

We do not have explicit instructions for editing the TOML files. We only mention that they should include them their version control: https://wasp-lang.dev/docs/advanced/deployment/cli#setup

Should we explain this to them?

We should 👍

Db -> it will have 0 minimum instances -> I guess that is ok, right? Could that cause any issues? Connection dropping between server and db maybe?

In our setup, the minimum for the DB is 1 and we don't use their "Scale to zero" feature for Postgres. https://fly.io/docs/postgres/managing/scale-to-zero/

The two machines issue

There is an option to disable HA -> https://fly.io/docs/reference/app-availability/#turn-off-redundancy-on-deploy
We should address it in a separate PR.

@Martinsos
Copy link
Member

So regarding instructions to users about how we set min number of machines and that they can set it to 0 if they don't have any Jobs / long running stuff -> you said above that we should provide them with those instructions -> let's do it in this PR, while we are at it? I guess we should put this info into docs? But I think it is also interesting to put this info into output of wasp deploy fly.

Database scaling down, two machines -> ok yeah, we can tackle those separately.

Anything else needed to finalize this PR?

Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

@infomiho nice! Some changes are required: the main thing is I suggested some wording changes -> it is better to be redundant in this case and offer a bit longer explanation, then leave them wondering what this is about. I also wanted to let them know to reach out to us, so we learn about these issues.

@infomiho
Copy link
Contributor Author

infomiho commented Nov 6, 2023

I've adjusted the message 👍

Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

Looking good @infomiho !

Copy link
Member

Choose a reason for hiding this comment

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

Nice job adding prettier, but it really makes it hard to review a PR, because there are so many changes and I have to check what is just formatting and what is something more!

It is usually advised to do formatting as a separate PR, not as part of another PR, for this reason.

@infomiho infomiho merged commit 2752f88 into main Nov 6, 2023
4 checks passed
@infomiho infomiho deleted the miho-fly-min-machines branch November 6, 2023 14:47
martinovicdev pushed a commit to martinovicdev/wasp that referenced this pull request Nov 15, 2023
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.

Update our fly.toml files to have correct minimum number of instances running
2 participants