You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/01/26 06:12:28 UTC
[GitHub] [airflow] Junnplus opened a new pull request #13906: Make webserver rolling update
Junnplus opened a new pull request #13906:
URL: https://github.com/apache/airflow/pull/13906
webserver pod waiting long time for init container complete, using one replica will make the webserver unavailable.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r571820427
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+{{ toYaml .Values.webserver.rollingUpdate | indent 6 }}
Review comment:
Like you said, `maxUnavailable` maybe more straight forward, but the PR title is "make webserver rolling update", not just `maxUnavailable`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r568613870
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+ maxUnavailable: 0
Review comment:
I agree it's reasonable to let the users decide `maxUnavailable` (e.g. in `values.yaml`), **with proper default/fall-back value.**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r566646789
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+ maxUnavailable: 0
Review comment:
@mik-laj Can we let the user decide `maxUnavailable` value?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r568004168
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+ maxUnavailable: 0
Review comment:
> Can we let users decide maxUnavailable ?
ping @mik-laj @XD-DENG
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+ maxUnavailable: 0
Review comment:
> Can we let users decide maxUnavailable ?
ping @mik-laj @XD-DENG
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
Junnplus commented on pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#issuecomment-774636975
ping @XD-DENG @mik-laj
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13906: Support rollingUpdate config of webserver
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r571837101
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+{{ toYaml .Values.webserver.rollingUpdate | indent 6 }}
Review comment:
`rollingUpdate.maxSurge` is an optional field, default value is 25%, I added the comment of maxsurge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r565000311
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+ maxUnavailable: 0
Review comment:
> It's possible that users explicitly specify the replicas to 1, for some purposes. In that case, users take the responsibility and should be aware of the result.
I don't think so, now replicas defaults to 1, upgrade chart always waiting long time, this will make the webserver unavailable.
Of course, set replicas to >=2 can solve this problem, but everyone needs to do it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#issuecomment-774701367
I'm ok to merge this once @XD-DENG 's comments are addressed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r565000311
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+ maxUnavailable: 0
Review comment:
> It's possible that users explicitly specify the replicas to 1, for some purposes. In that case, users take the responsibility and should be aware of the result.
I don't think so, now replicas defaults to 1, upgrade chart always waiting long time, this will make the webserver unavailable.
Of course, set replicas to >=2 maybe can solve this problem, but everyone needs to do it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13906: Support rollingUpdate config of webserver
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r571829290
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+{{ toYaml .Values.webserver.rollingUpdate | indent 6 }}
Review comment:
Rolling Update of kubernetes not just `maxUnavailable`
>I think, It may be better to support both maxSurge and maxUnavailable.
> https://github.com/apache/airflow/pull/13906/files#diff-a9d07052701ab3b718c82ac0fe74b965a8260b06bb5906362081718b0ee593b4R419
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r571585641
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+{{ toYaml .Values.webserver.rollingUpdate | indent 6 }}
Review comment:
maybe more straight forward to have `maxUnavailable: {{ .Values.webserver.rollingUpdate.maxUnavailable }}` here. Let me know if you have different opinion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus closed pull request #13906: Support rollingUpdate config of webserver
Posted by GitBox <gi...@apache.org>.
Junnplus closed pull request #13906:
URL: https://github.com/apache/airflow/pull/13906
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r566679722
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+ maxUnavailable: 0
Review comment:
Can we let users decide `maxUnavailable` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13906: Support rollingUpdate config of webserver
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r571837101
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+{{ toYaml .Values.webserver.rollingUpdate | indent 6 }}
Review comment:
`rollingUpdate.maxSurge` is an optional field, default value is 25%, I added the annotation of maxsurge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r571817647
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+{{ toYaml .Values.webserver.rollingUpdate | indent 6 }}
Review comment:
I think, It may be better to support both `maxSurge` and `maxUnavailable`.
https://github.com/apache/airflow/pull/13906/files#diff-a9d07052701ab3b718c82ac0fe74b965a8260b06bb5906362081718b0ee593b4R419
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #13906: Support rollingUpdate config of webserver
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r571830308
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+{{ toYaml .Values.webserver.rollingUpdate | indent 6 }}
Review comment:
Then where is your `maxSurge` in `values.yaml`? :-)
As well as the necessary update in `chart/README.md`?
Please make *all* necessary changes and rebase.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r571821793
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+{{ toYaml .Values.webserver.rollingUpdate | indent 6 }}
Review comment:
As I mentioned above, the change you intend to make here is no longer "make webserver rolling update", instead, it's allowing more configuraitons.
As already suggested, please update the PR title and description; or create a new PR accordingly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r566169763
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+ maxUnavailable: 0
Review comment:
I'm not really convinced by this. I would like to have some valuable inputs from other committers as well @mik-laj @dimberman
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r568613870
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+ maxUnavailable: 0
Review comment:
I agree it's reasonable to let the users decide `maxUnavailable` (e.g. in `values.yaml`), **with proper default/fall-back value.**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Junnplus commented on a change in pull request #13906: Support rollingUpdate config of webserver
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r571824564
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+{{ toYaml .Values.webserver.rollingUpdate | indent 6 }}
Review comment:
@XD-DENG Well.. I modified the title.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r566608579
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+ maxUnavailable: 0
Review comment:
It makes sense for me to provide support for service updates by default, with a guarantee of its continuous availability. I don't think HA for the webserver is often deployed because the webserver is not used very heavily because the key feature are provided by the webserver and worker. The web server only supports the use of Airflow but is an optional component. The Web Server only supports the use of Airflow, but from a design perspective it is an optional, non-essential component, so in most cases, HA is not necessary.
However, to handle both cases, we can add a condition. If `replicas > 2`, then the `maxUnavailable = replicas - 1,` If `replicas == 1`, then `maxUnavailable = 0`. What do you think about it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r569153218
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+ maxUnavailable: 0
Review comment:
I agree with both.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #13906: Make webserver rolling update
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r564603095
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+ maxUnavailable: 0
Review comment:
The original code (`maxUnavailable: 1`) already looks good to me when `replicas` >=2.
It's possible that users explicitly specify the `replicas` to 1, for some purposes. In that case, users take the responsibility and should be aware of the result.
Please correct me if I misunderstand/miss anything.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #13906: Support rollingUpdate config of webserver
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13906:
URL: https://github.com/apache/airflow/pull/13906#discussion_r571826925
##########
File path: chart/templates/webserver/webserver-deployment.yaml
##########
@@ -39,7 +39,7 @@ spec:
strategy:
type: RollingUpdate
rollingUpdate:
- maxUnavailable: 1
+{{ toYaml .Values.webserver.rollingUpdate | indent 6 }}
Review comment:
Do you mind addressing the comment above? I don't see it necessary to use `toYaml` here.
> maybe more straight forward to have `maxUnavailable: {{ .Values.webserver.rollingUpdate.maxUnavailable }}` here. Let me know if you have different opinion.
In addition, please also update the commit subject and add description properly (that's what appear eventually in the commit history)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org