-
Notifications
You must be signed in to change notification settings - Fork 499
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
Remove helm hook annotation for initializer job #526
Conversation
Signed-off-by: Aylei <rayingecho@gmail.com>
/run-e2e-tests |
1 similar comment
/run-e2e-tests |
Signed-off-by: Aylei <rayingecho@gmail.com>
/run-e2e-tests |
Signed-off-by: Aylei <rayingecho@gmail.com>
/run-e2e-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Aylei <rayingecho@gmail.com>
After communicating with @weekface , make the original tidb-configmap stable is a better idea. I've updated this PR accordingly. @xiaojingchen PTAL again😆 |
/run-e2e-tests |
apiVersion: v1 | ||
kind: ConfigMap | ||
metadata: | ||
{{- if .Values.enableConfigMapRollout }} | ||
name: {{ template "cluster.name" . }}-tidb-{{ template "tidb-configmap.data-digest" . }} | ||
{{- else }} | ||
name: {{ template "cluster.name" . }}-tidb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think pd-configmap.yaml
and tikv-configmap.yaml
should also be modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this? pd-configmap
and tikv-configmap
are only intended to be referenced by the corresponding statefulset and there's no compatibility issue.
BTW, actually there is an issue about configmap rollout: the old configmap will be deleted when the new configmap created, which will cause the old controller-revision
's reference to the old configmap is invalid. Although this issue is not a problem now because of the careful statefulset upgrade procedure, it should (and will) be fixed for better maintainability (it is out of the scope of this issue, of course).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it is not related to this issue. Another PR is more reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to separate the initializer configurations with tidb itself configurations. So the initializer job can have a standalone stable configmap.
Yes, It should be if there's no backward compatibility issue. If we choose a different configmap name for initializer job, users who have tidb-clusters installed cannot upgrade to our new chart because of the change of configmap name in the initializer job, which will cause an error because the podTemplate of |
The job should be run once. Does helm try to upgrade the Job even if it's already completed? |
According to my previous experience, it is. That's why I added the helm hook annotation in the initializer job (to avoid upgrading after installing), but this was proven to be un-acceptable due to the wait time when install tidb-cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* zh: add get-started tutorial * Apply suggestions from code review Thanks for the review! Co-authored-by: Ran <huangran@pingcap.com> * Apply suggestions from code review Co-authored-by: Ran <huangran@pingcap.com> * Apply suggestions from code review Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com> * fix * delete kind/minikube * fix anchor links * fix * sync pingcap/docs-tidb-operator#542 * refine some expression Co-authored-by: Ran <huangran@pingcap.com> Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com>
Signed-off-by: Aylei rayingecho@gmail.com
What problem does this PR solve?
close #523
A fix for wrong conditional expression is also included (
and
&or
are functions in golang template, refer to template#functions)What is changed and how it works?
Helm hook removed
Check List
Tests
Code changes
@LinuxGit @xiaojingchen @tennix PTAL