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

Docs Fleet add section about RollingUpdate fields #1228

Merged
merged 2 commits into from
Jan 9, 2020

Conversation

aLekSer
Copy link
Collaborator

@aLekSer aLekSer commented Dec 11, 2019

RollingUpdate is not working when replicas number is updated.

@aLekSer
Copy link
Collaborator Author

aLekSer commented Dec 11, 2019

Requested and more details and logs here:
#1156

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 08d3be35-737e-4e35-bcfd-2a677d5dc1da

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1228/head:pr_1228 && git checkout pr_1228
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.2.0-b042525


1. Adds the `maxSurge` number of `GameServers` to the Fleet.
1. Shutdown the `maxUnavailable` number of `GameServers` in the Fleet, skipping `Allocated` `GameServers`.
1. Repeat above steps until all the previous `GameServer` configurations have been `Shutdown` and deleted.

{{< alert title="Note" color="info">}}
When fleet update contains changes of `replicas` parameter, than new gameservers would be created/deleted straight away,
Copy link
Member

Choose a reason for hiding this comment

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

Just making a note from a conversation in chat - looks like this isn't 100% accurate, since you can get rolling updates even with a replica change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, if there is only replicas change then there would be fast scale up, say from 3 to 30. If replicas + label then it would go stepwise.

Copy link
Member

Choose a reason for hiding this comment

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

Gentle bump on this - should we be adding some changes to the above to document the chat conversation we had?

Copy link
Collaborator Author

@aLekSer aLekSer Dec 19, 2019

Choose a reason for hiding this comment

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

Yes, definitely. Editing this

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: abdc41df-54a6-4e81-a1af-1403c91964f0

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1228/head:pr_1228 && git checkout pr_1228
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-e24a408

@markmandel markmandel added kind/cleanup Refactoring code, fixing up documentation, etc kind/documentation Documentation for Agones labels Dec 19, 2019
Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Looks good, just some suggested editing.

{{< alert title="Note" color="info">}}
When `Fleet` update contains only changes to `replicas` parameter, than new gameservers would be created/deleted straight away,
which means in that case `maxSurge` and `maxUnavailable` steps parameters will not be used.
Thus RollingUpdate strategy takes place when you update only not `replicas` parameters. If you update `replicas` parameter and some other parameters, then it could be a case when more than `maxSurge` parameters GameServers would be created.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Thus RollingUpdate strategy takes place when you update only not `replicas` parameters. If you update `replicas` parameter and some other parameters, then it could be a case when more than `maxSurge` parameters GameServers would be created.
The RollingUpdate strategy takes place when you update `spec` parameters other than `replicas`.


1. Adds the `maxSurge` number of `GameServers` to the Fleet.
1. Shutdown the `maxUnavailable` number of `GameServers` in the Fleet, skipping `Allocated` `GameServers`.
1. Repeat above steps until all the previous `GameServer` configurations have been `Shutdown` and deleted.

{{< alert title="Note" color="info">}}
When `Fleet` update contains only changes to `replicas` parameter, than new gameservers would be created/deleted straight away,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
When `Fleet` update contains only changes to `replicas` parameter, than new gameservers would be created/deleted straight away,
When `Fleet` update contains only changes to the `replicas` parameter, then new GameServers will be created/deleted straight away,


1. Adds the `maxSurge` number of `GameServers` to the Fleet.
1. Shutdown the `maxUnavailable` number of `GameServers` in the Fleet, skipping `Allocated` `GameServers`.
1. Repeat above steps until all the previous `GameServer` configurations have been `Shutdown` and deleted.

{{< alert title="Note" color="info">}}
When `Fleet` update contains only changes to `replicas` parameter, than new gameservers would be created/deleted straight away,
which means in that case `maxSurge` and `maxUnavailable` steps parameters will not be used.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
which means in that case `maxSurge` and `maxUnavailable` steps parameters will not be used.
which means in that case `maxSurge` and `maxUnavailable` parameters for a RollingUpdate will not be used.

which means in that case `maxSurge` and `maxUnavailable` steps parameters will not be used.
Thus RollingUpdate strategy takes place when you update only not `replicas` parameters. If you update `replicas` parameter and some other parameters, then it could be a case when more than `maxSurge` parameters GameServers would be created.

For the case when you want to be flexible on Fleet size, Fleetautoscaler and your prefer `kubectl apply` Fleet config with omitted `replicas` parameter should be used with.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For the case when you want to be flexible on Fleet size, Fleetautoscaler and your prefer `kubectl apply` Fleet config with omitted `replicas` parameter should be used with.
For the case when you want to be flexible on Fleet size, Fleetautoscaler and your prefer `kubectl apply` Fleet config with omitted `replicas` parameter should be used.

Thus RollingUpdate strategy takes place when you update only not `replicas` parameters. If you update `replicas` parameter and some other parameters, then it could be a case when more than `maxSurge` parameters GameServers would be created.

For the case when you want to be flexible on Fleet size, Fleetautoscaler and your prefer `kubectl apply` Fleet config with omitted `replicas` parameter should be used with.
Do not change `replicas` field manually when you use `kubectl edit fleet` for a fleet which is controlled by a Fleetautoscaler.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Do not change `replicas` field manually when you use `kubectl edit fleet` for a fleet which is controlled by a Fleetautoscaler.
Do not change the `replicas` field manually when you use `kubectl edit fleet` for a fleet which is controlled by a Fleetautoscaler.

If you want to update a `Fleet`, which has `RollingUpdate` replacement strategy and is controlled by a `FleetAutoscaler`, you could omit `Replicas` parameter in a `Fleet` Spec.
Otherwise fleet would be scaled according to Fleet `Replicas` parameter first and only after certain amount of time it would be rescaled to fit `FleetAutoscaler` `BufferSize` parameter.
If you want to update a `Fleet` which has `RollingUpdate` replacement strategy and is controlled by a `FleetAutoscaler`:
1. With `kubectl apply`: you should omit `replicas` parameter in a `Fleet` Spec before creating this `Fleet`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. With `kubectl apply`: you should omit `replicas` parameter in a `Fleet` Spec before creating this `Fleet`.
1. With `kubectl apply`: you should omit `replicas` parameter in a `Fleet` Spec before re-applying the `Fleet` configuration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Used your suggestion.

1. With `kubectl apply`: you should omit `replicas` parameter in a `Fleet` Spec before creating this `Fleet`.
1. With `kubectl edit`: you should not change `replicas` parameter in a `Fleet` Spec when updating other field parameter.

If you follow both rules above, then `maxSurge` and `maxUnavaiable` parameters would be used and update would actually be a RollingUpdate.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you follow both rules above, then `maxSurge` and `maxUnavaiable` parameters would be used and update would actually be a RollingUpdate.
If you follow the rules above, then `maxSurge` and `maxUnavaiable` parameters will be used as the RollingUpdate strategy updates your Fleet.

1. With `kubectl edit`: you should not change `replicas` parameter in a `Fleet` Spec when updating other field parameter.

If you follow both rules above, then `maxSurge` and `maxUnavaiable` parameters would be used and update would actually be a RollingUpdate.
Otherwise fleet would be scaled according to Fleet `replicas` parameter first and only after certain amount of time it would be rescaled to fit `FleetAutoscaler` `BufferSize` parameter.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Otherwise fleet would be scaled according to Fleet `replicas` parameter first and only after certain amount of time it would be rescaled to fit `FleetAutoscaler` `BufferSize` parameter.
Otherwise the Fleet would be scaled according to Fleet `replicas` parameter first and only after certain amount of time it would be rescaled to fit `FleetAutoscaler` `BufferSize` parameter.

If you follow both rules above, then `maxSurge` and `maxUnavaiable` parameters would be used and update would actually be a RollingUpdate.
Otherwise fleet would be scaled according to Fleet `replicas` parameter first and only after certain amount of time it would be rescaled to fit `FleetAutoscaler` `BufferSize` parameter.

You could also check the behaviour of the Fleet with Fleetautoscaler on a test `Fleet` to see what you would get on your production environment.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You could also check the behaviour of the Fleet with Fleetautoscaler on a test `Fleet` to see what you would get on your production environment.
You could also check the behaviour of the Fleet with Fleetautoscaler on a test `Fleet` to preview what would occur on your production environment.

@aLekSer aLekSer force-pushed the fix/rollingupdate branch 2 times, most recently from c631361 to 813e0bf Compare December 20, 2019 11:42
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 5f98a027-d3c1-4247-9127-a81f1fdcedee

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1228/head:pr_1228 && git checkout pr_1228
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-c631361

@aLekSer
Copy link
Collaborator Author

aLekSer commented Dec 20, 2019

Resolved all the comments. The most tricky one was:

With `kubectl apply`: you should omit `replicas` parameter in a `Fleet` Spec before creating (suuggested : before re-applying) this `Fleet`. 

I also think that should be like this.
Small concern is in two different behaviours:

  1. if we remove replicas and change other parameter - all is as you describe
  2. if we only remove replicas on second apply - scale down immediately to 0 and then scaled up to BufferPolicy.
    2nd is covered by fleet_updates.md so both are covered.
When `Fleet` update contains only changes to the `replicas` parameter, then new GameServers will be created/deleted straight away,
which means in that case `maxSurge` and `maxUnavailable` parameters for a RollingUpdate will not be used.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 06cfcf1a-03b0-4519-a59d-2f46927a8d32

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 03dfb967-eca5-4205-b8a9-3082beb1606a

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1228/head:pr_1228 && git checkout pr_1228
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-afaa2c1

@aLekSer
Copy link
Collaborator Author

aLekSer commented Dec 20, 2019

Issue in website check:

Get https://v1-12.docs.kubernetes.io/docs/reference/generated/kubernetes-api/v1.12/#podtemplatespec-v1-core: x509: certificate is valid for *.netlify.com, netlify.com, not v1-12.docs.kubernetes.io --- site/docs/reference/gameserver/index.html --> https://v1-12.docs.kubernetes.io/docs/reference/generated/kubernetes-api/v1.12/#podtemplatespec-v1-core

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 5ed57b2f-1ab0-468d-b2d7-bd4302ce7315

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1228/head:pr_1228 && git checkout pr_1228
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-b4c6556

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: d05747c5-8de0-4226-a658-12b827c97ef8

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1228/head:pr_1228 && git checkout pr_1228
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-65b944e

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 08436151-aaa7-4a55-9716-9f20311a9c1a

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1228/head:pr_1228 && git checkout pr_1228
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-6df259f

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Like this much better!

For the case when you want to be flexible on Fleet size, Fleetautoscaler and your prefer `kubectl apply` Fleet config with omitted `replicas` parameter should be used.
Do not change the `replicas` field manually when you use `kubectl edit fleet` for a fleet which is controlled by a Fleetautoscaler.

[Read Fleetautoscaler guide]({{< relref "../Getting Started/create-fleetautoscaler.md#7-change-autoscaling-parameters" >}}) for more details.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[Read Fleetautoscaler guide]({{< relref "../Getting Started/create-fleetautoscaler.md#7-change-autoscaling-parameters" >}}) for more details.
[Read the Fleetautoscaler guide]({{< relref "../Getting Started/create-fleetautoscaler.md#7-change-autoscaling-parameters" >}}) for more details.


[Read Fleetautoscaler guide]({{< relref "../Getting Started/create-fleetautoscaler.md#7-change-autoscaling-parameters" >}}) for more details.

You could also check the behaviour of the Fleet with RollingUpdate strategy on a test `Fleet` to preview your upcoming updates.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You could also check the behaviour of the Fleet with RollingUpdate strategy on a test `Fleet` to preview your upcoming updates.
You could also check the behaviour of the Fleet with a RollingUpdate strategy on a test `Fleet` to preview your upcoming updates.

[Read Fleetautoscaler guide]({{< relref "../Getting Started/create-fleetautoscaler.md#7-change-autoscaling-parameters" >}}) for more details.

You could also check the behaviour of the Fleet with RollingUpdate strategy on a test `Fleet` to preview your upcoming updates.
Use `kubectl describe fleet` to track scaling events in a fleet.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Use `kubectl describe fleet` to track scaling events in a fleet.
Use `kubectl describe fleet` to track scaling events in a Fleet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changing this throughout a file

which means in that case `maxSurge` and `maxUnavailable` parameters for a RollingUpdate will not be used.
The RollingUpdate strategy takes place when you update `spec` parameters other than `replicas`.

For the case when you want to be flexible on Fleet size, Fleetautoscaler and your prefer `kubectl apply` Fleet config with omitted `replicas` parameter should be used.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand this sentence (and maybe the next one)

Could this be something more like:

If you are using a Fleet which is scaled by a FleetAutoscaler, [read the Fleetautoscaler guide]({{< relref "../Getting Started/create-fleetautoscaler.md#7-change-autoscaling-parameters" >}}) for more details on how RollingUpdates with FleetAutoscalers need to implemented.

Rather than writing the same documentation twice?
(If I'm hitting the idea of what you were trying to say)

@markmandel
Copy link
Member

Gentle bump on this - would be good to have this in next week's RC release.

@aLekSer
Copy link
Collaborator Author

aLekSer commented Jan 8, 2020

Thanks @markmandel on updates.
Sorry for the delay, long holidays.
Happy to update this PR according to your comments.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: a0a39eba-85d7-47de-b446-fe8ea5e57449

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1228/head:pr_1228 && git checkout pr_1228
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-6f3fa20

RollingUpdate is not working when replicas number is updated.
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 4f91fb34-3a6c-4f68-8c53-207c6e3e3b4b

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1228/head:pr_1228 && git checkout pr_1228
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-8c81cbb

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

🔥

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aLekSer, markmandel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-robot
Copy link

New changes are detected. LGTM label has been removed.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 06c88139-f6ae-4574-8904-07455ecc43d2

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1228/head:pr_1228 && git checkout pr_1228
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.3.0-cd6d354

@markmandel markmandel merged commit c439d4b into googleforgames:master Jan 9, 2020
@markmandel markmandel added this to the 1.3.0 milestone Jan 9, 2020
@aLekSer aLekSer deleted the fix/rollingupdate branch January 13, 2020 15:25
ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020
RollingUpdate is not working when replicas number is updated.

Co-authored-by: Mark Mandel <mark.mandel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved kind/cleanup Refactoring code, fixing up documentation, etc kind/documentation Documentation for Agones size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants