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.

Sign up for GitHub

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

Jump to bottom

helm: update updateStrategy #2351

Merged
merged 2 commits into from
Aug 5, 2021
Merged

helm: update updateStrategy #2351

merged 2 commits into from
Aug 5, 2021

Conversation

Yuggupta27
Copy link
Contributor

@Yuggupta27 Yuggupta27 commented Aug 3, 2021 •
edited
Loading

Update the provisioner and nodeplugin updatestrategy
to allow maxUnavailable and maxSurge
pods at a time to be 50%

Fixes: #2335

Signed-off-by: Yug Gupta yuggupta27@gmail.com

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

  • /retest ci/centos/<job-name>: retest the <job-name> after unrelated
    failure (please report the failure too!)
  • /retest all: run this in case the CentOS CI failed to start/report any test
    progress or results

@mergify mergify bot added the component/deployment Helm chart, kubernetes templates and configuration Issues/PRs label Aug 3, 2021
@Yuggupta27
Copy link
Contributor Author

/retest ci/centos/upgrade-tests-rbd

@Yuggupta27
Copy link
Contributor Author

/retest ci/centos/mini-e2e-helm/k8s-1.19

@Yuggupta27
Copy link
Contributor Author

Both jobs failed with #2264

nixpanic
nixpanic reviewed Aug 3, 2021
View reviewed changes
charts/ceph-csi-cephfs/templates/provisioner-deployment.yaml
@@ -11,6 +11,11 @@ metadata:
heritage: {{ .Release.Service }}
spec:
replicas: {{ .Values.provisioner.replicaCount }}
strategy:
type: {{ .Values.provisioner.strategy.type }}
rollingUpdate:
Copy link
Member

Choose a reason for hiding this comment

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

this should only be included when Values.provisioner.strategy.type == RollingUpdate

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, need an if check here to check the strategy.type == RollingUpdate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! Added the check.

charts/ceph-csi-cephfs/values.yaml Outdated Show resolved Hide resolved
Madhu-1
Madhu-1 requested changes Aug 4, 2021
View reviewed changes
Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

The issue exists only for deployment, not for daemonset pods? can you please send a separate PR daemonset if it's required?

charts/ceph-csi-cephfs/templates/nodeplugin-daemonset.yaml Outdated Show resolved Hide resolved
charts/ceph-csi-cephfs/templates/provisioner-deployment.yaml
@@ -11,6 +11,11 @@ metadata:
heritage: {{ .Release.Service }}
spec:
replicas: {{ .Values.provisioner.replicaCount }}
strategy:
type: {{ .Values.provisioner.strategy.type }}
rollingUpdate:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, need an if check here to check the strategy.type == RollingUpdate

@Yuggupta27
Copy link
Contributor Author

Yuggupta27 commented Aug 4, 2021 •
edited
Loading

ci/centos/upgrade-tests-rbd failed with


    Command stdout:

    

    stderr:

    Error from server (AlreadyExists): error when creating "STDIN": serviceaccounts "rbd-csi-vault-token-review" already exists

    Error from server (AlreadyExists): error when creating "STDIN": clusterroles.rbac.authorization.k8s.io "rbd-csi-vault-token-review" already exists

    Error from server (AlreadyExists): error when creating "STDIN": clusterrolebindings.rbac.authorization.k8s.io "rbd-csi-vault-token-review" already exists```

@Yuggupta27
Copy link
Contributor Author

/retest ci/centos/upgrade-tests-rbd

@humblec
Copy link
Collaborator

humblec commented Aug 4, 2021

LGTM. However will wait for review from @nixpanic as he was already reviewing it

@Yuggupta27
Copy link
Contributor Author

/retest ci/centos/mini-e2e-helm/k8s-1.19

@Yuggupta27
Copy link
Contributor Author

/retest ci/centos/mini-e2e-helm/k8s-1.21

@Yuggupta27
Copy link
Contributor Author

/retest ci/centos/mini-e2e-helm/k8s-1.21

Failed with #2264

@Yuggupta27
Copy link
Contributor Author

/retest ci/centos/mini-e2e/k8s-1.21

@Yuggupta27
Copy link
Contributor Author

/retest ci/centos/mini-e2e/k8s-1.21

Failed with #2362

@Yuggupta27
Copy link
Contributor Author

/retest ci/centos/mini-e2e-helm/k8s-1.21

@Yuggupta27
Copy link
Contributor Author

/retest ci/centos/mini-e2e-helm/k8s-1.21

Failed with #2264

@nixpanic
Copy link
Member

nixpanic commented Aug 5, 2021

/retest ci/centos/mini-e2e-helm/k8s-1.20

@nixpanic nixpanic mentioned this pull request Aug 5, 2021
Closed
@nixpanic
Copy link
Member

nixpanic commented Aug 5, 2021

/retest ci/centos/mini-e2e/k8s-1.20

1 similar comment
@Yuggupta27
Copy link
Contributor Author

/retest ci/centos/mini-e2e/k8s-1.20

@Yuggupta27
helm: update rbd provisioner updateStrategy
f683eda 
Update ceph-csi-rbd.provisioner updatestrategy
to allow maxUnavailable pods at a time to be 50%

Signed-off-by: Yug Gupta <yuggupta27@gmail.com>
@Yuggupta27
helm: update cephfs provisioner updateStrategy
6add7e0 
Update ceph-csi-cephfs.provisioner updatestrategy
to allow maxUnavailable pods at a time to be 50%

Signed-off-by: Yug Gupta <yuggupta27@gmail.com>
@mergify mergify bot merged commit 080f753 into ceph:devel Aug 5, 2021
@mergify mergify bot mentioned this pull request Aug 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nixpanic nixpanic nixpanic approved these changes

@Madhu-1 Madhu-1 Madhu-1 approved these changes

Assignees
No one assigned
Labels
component/deployment Helm chart, kubernetes templates and configuration Issues/PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pod anti-affinity breaks upgrades
4 participants
@Yuggupta27 @humblec @nixpanic @Madhu-1