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

[cinder-csi-plugin] added dnsPolicy value for nodeplugin #2483

Merged
merged 3 commits into from
Dec 1, 2023

Conversation

gman0
Copy link
Member

@gman0 gman0 commented Nov 28, 2023

What this PR does / why we need it:

This PR adds dnsPolicy value to cinder-csi-plugin's Helm chart. The default value is set to ClusterFirst, which the current default.

Which issue this PR fixes(if applicable):
fixes #2481

Special notes for reviewers:

Release note:

[cinder-csi-plugin] Added .csi.plugin.nodePlugin.dnsPolicy value

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 28, 2023
@dulek
Copy link
Contributor

dulek commented Nov 28, 2023

Any reason to not just set this directly to ClusterFirstWithHostNet as in Manila chart?

@jichenjc
Copy link
Contributor

jichenjc commented Nov 29, 2023

Any reason to not just set this directly to ClusterFirstWithHostNet as in Manila chart?

I might not understand clearly, but seems the changed item is Cinder instead of manila?

@dulek
Copy link
Contributor

dulek commented Nov 29, 2023

Any reason to not just set this directly to ClusterFirstWithHostNet as in Manila chart?

I might not understand clearly, but seems the changed item is Cinder instead of manila?

Sorry for the confusion, I mean that in Manila chart we don't make this configurable, just set it directly to ClusterFirstWithHostNet. Maybe that's the way to go in Cinder case too?

@gman0
Copy link
Member Author

gman0 commented Nov 29, 2023

@dulek Sure, changed as requested. Initially I've set it to ClusterFirst as that's what currently used by the DaemonSet. PTAL.

@jichenjc
Copy link
Contributor

jichenjc commented Dec 1, 2023

/lgtm

ok, understand, thanks~ @dulek

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 1, 2023
@dulek
Copy link
Contributor

dulek commented Dec 1, 2023

I wasn't sure this is the right way, but it makes a lot of sense to be consistent here.
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2023
@k8s-ci-robot k8s-ci-robot merged commit 8a156e5 into kubernetes:master Dec 1, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cinder-csi-plugin] Missing dnsPolicy when running with hostNetwork: true
4 participants