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

Add CRUD for autoscalers #339

Merged
merged 18 commits into from
Aug 30, 2018
Merged

Add CRUD for autoscalers #339

merged 18 commits into from
Aug 30, 2018

Conversation

EtienneM
Copy link
Member

@EtienneM EtienneM commented Mar 7, 2018

Please first review Scalingo/go-scalingo#40

  • Add bash autocomplete stuff
  • scalingo ps command should display somehow that a given container type has an autoscaler
  • When manual scaling, warn about disabling the autoscaling and ask for confirmation
  • Add Update
  • Add Remove

Related to Scalingo/autoscaler-service#1

@EtienneM
Copy link
Member Author

EtienneM commented Mar 7, 2018

╰─$ scalingo -a biniou autoscalers

+--------+----------------+-------------------+--------+----------------+----------------+
| ACTIVE | CONTAINER TYPE |      METRIC       | TARGET | MIN CONTAINERS | MAX CONTAINERS |
+--------+----------------+-------------------+--------+----------------+----------------+
| false  | web            | p95_response_time |  60.00 | 2              | 7              |
| true   | web            | cpu               |   0.75 | 1              | 3              |
+--------+----------------+-------------------+--------+----------------+----------------+

@EtienneM
Copy link
Member Author

EtienneM commented Mar 7, 2018

╰─$ scalingo -a biniou autoscalers-add --container-type web --metric cpu --target 0.75 --min-containers 1 --max-containers 3

-----> Autoscaler created on biniou for web containers

@Soulou
Copy link
Member

Soulou commented Mar 7, 2018

I disagree on the first table column being Disabled. Why using a negative form instead of ACTIVE ? It should be the last one.

I think the autoscaler row should include the current scaling of the application, to avoid people to make 2 requests here.

@Soulou
Copy link
Member

Soulou commented Mar 7, 2018

So finally, no integration in the scale command then!

What happens if I try to scale my app with the CLI?

@EtienneM
Copy link
Member Author

EtienneM commented Mar 8, 2018

I found it really unclear to integrate with the scale command...

If you manually scale with the CLI, I would like to show a message warning about an autoscaler being present and asking confirmation.

@Soulou
Copy link
Member

Soulou commented Mar 8, 2018

Ok for the integration with the scale command, but then the UI should be nice.

What about the command scalingo ps, shouldn't it display also that a container type is autoscaled?

For the confirmation how will you do that, fetch the autoscalers first to see if one exists and is active, or the API will returns something?

@Soulou
Copy link
Member

Soulou commented Mar 8, 2018

Can you share how you've updated the style?

@EtienneM
Copy link
Member Author

EtienneM commented Mar 8, 2018

You are right about the scalingo ps command. I will add something.

For the confirmation, I had the first solution in mind (fetch the autoscalers first) but both are doable.

I edited the previous messages with the updated wording of the table

@Soulou
Copy link
Member

Soulou commented Mar 8, 2018

I still don't know if Active should be the first column =\

@EtienneM
Copy link
Member Author

EtienneM commented Mar 8, 2018

I had the idea to highlight with a different color the line where the autoscaler is disabled, but then it looked like a christmas tree..

@Soulou
Copy link
Member

Soulou commented Mar 8, 2018

Usually colors are not always the best ideas :-D

@EtienneM
Copy link
Member Author

EtienneM commented Mar 8, 2018

$ scalingo -a biniou autoscalers-enable web
$ scalingo -a biniou autoscalers-disable web
$ scalingo -a biniou autoscalers-update --container-type web --max-containers 5
$ scalingo -a biniou autoscalers-update --container-type web --max-containers 5 --disabled

-----> Autoscaler updated on biniou for web containers

@EtienneM
Copy link
Member Author

EtienneM commented Mar 8, 2018

╰─$ scalingo -a biniou scale web:3

  /!\  An autoscaler is configured for web container. Manually scaling it will disable the autoscaler. Do you confirm? (y/N)

 !     An error occurred:
       You didn't confirm, aborting…

Typing anything but y or Y will abort the scaling operation

@EtienneM
Copy link
Member Author

EtienneM commented Mar 8, 2018

╰─$ scalingo -a biniou ps

+---------+--------+------+---------------------+
|  NAME   | AMOUNT | SIZE |       COMMAND       |
+---------+--------+------+---------------------+
| web (*) | 1      | M    | `sample-go-martini` |
| test    | 2      | M    | `sample-go-martini` |
+---------+--------+------+---------------------+
  (*) has an autoscaler defined

We don't display anything if no autoscaler is defined

@Soulou
Copy link
Member

Soulou commented Mar 28, 2018

Please rebase

@EtienneM EtienneM force-pushed the fix/autoscaler/1 branch 3 times, most recently from be5eb51 to 61ad0db Compare April 3, 2018 09:07
apps/ps.go Outdated
for _, ct := range processes {
name := ct.Name
_, err = autoscalers.GetFromContainerType(app, name)
Copy link
Member

Choose a reason for hiding this comment

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

You need a cache here, you can't make a list request at each iteration of the loop

(same at the other place of the PR where you're doing this)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see what kind of cache I could use here. Every call to GetFromContainerType is with different parameters

Copy link
Member

Choose a reason for hiding this comment

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

@EtienneM You get all the autoscalers with a call in #index once, then you look into them instead of making plenty of API requests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Silly me... Sorry. That's obvious to me now

@Soulou Soulou merged commit c998f2c into master Aug 30, 2018
@EtienneM EtienneM deleted the fix/autoscaler/1 branch May 17, 2021 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants