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

operator: make imagePullPolicy customizable for csi driver and ceph pods #10966

Merged
merged 1 commit into from
Sep 28, 2022

Conversation

avanthakkar
Copy link
Member

@avanthakkar avanthakkar commented Sep 13, 2022

Signed-off-by: Avan Thakkar athakkar@redhat.com

Description of your changes:
Introduce a new env variable ROOK_CSI_IMAGE_PULL_POLICY in rook operator configmap which should be used to customize the imagePullPolicy for the csi driver and imagePullPolicy property in cephVersionSpec for ceph pods.

Which issue is resolved by this Pull Request:
Resolves #10783

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide).
  • Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@mergify
Copy link

mergify bot commented Sep 13, 2022

This pull request has merge conflicts that must be resolved before it can be merged. @avanthakkar please rebase it. https://rook.io/docs/rook/latest/Contributing/development-flow/#updating-your-fork

@avanthakkar avanthakkar force-pushed the customizable-image-pull-policy branch 3 times, most recently from 65bc798 to 403dd98 Compare September 13, 2022 10:57
pkg/operator/ceph/cluster/mgr/spec.go Outdated Show resolved Hide resolved
deploy/examples/operator.yaml Outdated Show resolved Hide resolved
pkg/operator/ceph/controller/spec.go Outdated Show resolved Hide resolved
pkg/operator/ceph/controller/spec.go Outdated Show resolved Hide resolved
@travisn
Copy link
Member

travisn commented Sep 13, 2022

and please ignore my comment in our separate discussion that this overlapped with #10956, it's really a separate feature, thanks.

@avanthakkar avanthakkar requested review from subhamkrai and travisn and removed request for subhamkrai September 14, 2022 08:39
@avanthakkar avanthakkar marked this pull request as ready for review September 14, 2022 08:44
@avanthakkar avanthakkar requested review from subhamkrai and removed request for travisn September 14, 2022 08:45
Copy link
Contributor

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

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

please squash your commits since both commits are related to the same changes. Thanks!

Copy link
Contributor

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

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

LGTM!

@sp98
Copy link
Contributor

sp98 commented Sep 14, 2022

@avanthakkar Why do we need to configure image pull policies for both CSI and Ceph images separately? Won't a single configuration for both ceph and csi images suffice? I don't think user environments would require different imagepullpolicy for different images.

deploy/examples/operator.yaml Outdated Show resolved Hide resolved
deploy/examples/operator.yaml Outdated Show resolved Hide resolved
deploy/examples/operator.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

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

marking request changes until comments are addressed. Thanks!

@avanthakkar avanthakkar force-pushed the customizable-image-pull-policy branch 3 times, most recently from b4a9e56 to 1e38229 Compare September 16, 2022 14:17
Documentation/CRDs/Cluster/ceph-cluster-crd.md Outdated Show resolved Hide resolved
Documentation/CRDs/Cluster/ceph-cluster-crd.md Outdated Show resolved Hide resolved
pkg/operator/ceph/cluster/osd/spec.go Show resolved Hide resolved
pkg/apis/ceph.rook.io/v1/types.go Show resolved Hide resolved
Copy link
Contributor

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

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

I have noticed smoke suite in k82 1.25 failing constantly. PTAL

@avanthakkar avanthakkar force-pushed the customizable-image-pull-policy branch 5 times, most recently from e8af8c6 to 698119a Compare September 23, 2022 07:11
@avanthakkar avanthakkar requested review from travisn and removed request for sp98 September 23, 2022 07:21
pkg/operator/ceph/csi/spec.go Outdated Show resolved Hide resolved
Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

Approved with a small suggestion, thanks!

@@ -136,6 +137,9 @@ var (
DefaultResizerImage = "registry.k8s.io/sig-storage/csi-resizer:v1.5.0"
DefaultCSIAddonsImage = "quay.io/csiaddons/k8s-sidecar:v0.5.0"

// image pull policy
DefaultCSIImagePullPolicy = "IfNotPresent"
Copy link
Member

Choose a reason for hiding this comment

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

what about this?

Suggested change
DefaultCSIImagePullPolicy = "IfNotPresent"
DefaultCSIImagePullPolicy = string(v1.PullIfNotPresent)

Introduce a new env variable ROOK_CSI_IMAGE_PULL_POLICY in rook operator configmap which should be used to
customize the imagePullPolicy for the csi driver and imagePullPolicy property in cephVersionSpec for ceph pods.

Signed-off-by: Avan Thakkar <athakkar@redhat.com>
@@ -32,6 +32,8 @@ Settings can be specified at the global level to apply to the cluster as a whole
Tags also exist that would give the latest version, but they are only recommended for test environments. For example, the tag `v17` will be updated each time a new Quincy build is released.
Using the `v17` tag is not recommended in production because it may lead to inconsistent versions of the image running across different nodes in the cluster.
* `allowUnsupported`: If `true`, allow an unsupported major version of the Ceph release. Currently `pacific` and `quincy` are supported. Future versions such as `reef` (v18) would require this to be set to `true`. Should be set to `false` in production.
`imagePullPolicy`: The image pull policy for the ceph daemon pods. Possible values are `Always`, `IfNotPresent`, and `Never`.
Copy link
Member

Choose a reason for hiding this comment

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

just a small question, what will happen if mentioned Never, And image is not present

Copy link
Contributor

Choose a reason for hiding this comment

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

pod startup will fail.

@travisn travisn dismissed Madhu-1’s stale review September 28, 2022 13:23

feedback addressed

@travisn travisn merged commit 0779618 into rook:master Sep 28, 2022
travisn added a commit that referenced this pull request Sep 28, 2022
operator: make imagePullPolicy customizable for csi driver and ceph pods (backport #10966)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow configuration of imagePullPolicy for underlying images
6 participants