Support rollingUpdate config of webserver#13906
Support rollingUpdate config of webserver#13906junnplus wants to merge 1 commit intoapache:masterfrom
Conversation
junnplus
force-pushed
the
roll-patch
branch
2 times, most recently
from
2e2f345 to
85ff2d3
Compare
XD-DENG
left a comment
XD-DENG
left a comment
There was a problem hiding this comment.
Thanks for the changes.
Other than the in-line comment below, the PR title and description should be updated accordingly, after your latest change. The scope now is more like "allow configuring rollingUpdate.maxUnavailable" instead of "make webserver rolling update" (This is also why I didn't realise there is new change to review in this PR).
There was a problem hiding this comment.
maybe more straight forward to have maxUnavailable: {{ .Values.webserver.rollingUpdate.maxUnavailable }} here. Let me know if you have different opinion.
There was a problem hiding this comment.
I think, It may be better to support both maxSurge and maxUnavailable.
There was a problem hiding this comment.
Like you said, maxUnavailable maybe more straight forward, but the PR title is "make webserver rolling update", not just maxUnavailable.
There was a problem hiding this 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.
There was a problem hiding this 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)
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this comment.
rollingUpdate.maxSurge is an optional field, default value is 25%, I added the annotation of maxsurge.
|
I'm ok to merge this once @XD-DENG 's comments are addressed |
junnplus
changed the title
junnplus
force-pushed
the
roll-patch
branch
3 times, most recently
from
20fa7e4 to
02a9f75
Compare
4 participants
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 for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.