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

added config to enable local volume provisioner #303

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

Pavan-Gunda
Copy link
Contributor

@Pavan-Gunda Pavan-Gunda commented Aug 28, 2023

What this PR does / why we need it:

Which issue this PR fixes (use the format fixes #<issue number>(, fixes #<issue_number>, ...) to automatically close the issue when PR gets merged): fixes #238

Public facing documentation PR (if applicable)

Special notes for reviewer:

Checklist:

  • Proper commit message prefix on all commits
  • Updated the public facing documentation
  • Is this changeset backwards compatible for existing clusters? Applying:
    • is completely transparent, will not impact the workload in any way.
    • requires running a migration script.
    • requires draining and/or replacing nodes.
    • will break the cluster.
      I.e. full cluster migration is required.

@Pavan-Gunda Pavan-Gunda marked this pull request as ready for review August 29, 2023 07:46
@Pavan-Gunda Pavan-Gunda self-assigned this Aug 29, 2023
docs/local-volume-provisioner.md Outdated Show resolved Hide resolved
@@ -12,6 +12,7 @@ cinder_csi_enabled: true
persistent_volumes_enabled: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to only have this for openstack?
I'm guessing we could run this on exoscale for instance as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is true !!

do you recommend moving it to ck8s-k8s-cluster.yaml ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm kind of tempted to do so

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I think that makes sense. It should work for any as long as you set it up correctly. But you've specified in the docs that it's only tested on openstack so that should be fine for now imo.

Copy link
Contributor Author

@Pavan-Gunda Pavan-Gunda Aug 29, 2023

Choose a reason for hiding this comment

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

But when we do an init with non openstack clouds, these new values won't show up in the rendered manifests, so I want to propose moving the values to ck8s-k8s-cluster.yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

This does not seem to work for Cleura
We need to test and make it work on all cloud providers

Copy link
Contributor

Choose a reason for hiding this comment

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

This does not seem to work for Cleura We need to test and make it work on all cloud providers

No we don't. This is a public repo so I will skip the internal business related stuff but local storage is to only be offered, initially, on the infrastructure providers elastx and safespring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But when we do an init with non openstack clouds, these new values won't show up in the rendered manifests, so I want to propose moving the values to ck8s-k8s-cluster.yaml

Any thoughts ? if not, i'll keep them in openstack manifests since we are currently only offering it in elastx and safespring.

Copy link
Contributor

Choose a reason for hiding this comment

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

But when we do an init with non openstack clouds, these new values won't show up in the rendered manifests, so I want to propose moving the values to ck8s-k8s-cluster.yaml

So, if you move the values to ck8s-k8s-cluster.yaml they will show up when doing an init on non-openstack clouds?

docs/local-volume-provisioner.md Outdated Show resolved Hide resolved
docs/local-volume-provisioner.md Outdated Show resolved Hide resolved
@@ -12,6 +12,7 @@ cinder_csi_enabled: true
persistent_volumes_enabled: true
Copy link
Contributor

Choose a reason for hiding this comment

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

But when we do an init with non openstack clouds, these new values won't show up in the rendered manifests, so I want to propose moving the values to ck8s-k8s-cluster.yaml

So, if you move the values to ck8s-k8s-cluster.yaml they will show up when doing an init on non-openstack clouds?

@Pavan-Gunda
Copy link
Contributor Author

@OlleLarsson
Copy link
Contributor

If we move it to https://github.com/elastisys/compliantkubernetes-kubespray/blob/main/config/common/group_vars/k8s_cluster/ck8s-k8s-cluster.yaml

yes, it should show up in the rendered manifests.

Then it should like a good thing to move it there

@Pavan-Gunda Pavan-Gunda force-pushed the pg/local-volume-provisioner branch from 0c1d7f3 to 221b80c Compare September 1, 2023 13:39
@Pavan-Gunda Pavan-Gunda force-pushed the pg/local-volume-provisioner branch from bc3745d to fe86c5a Compare September 5, 2023 11:28
@Pavan-Gunda Pavan-Gunda merged commit 6dfdb86 into main Sep 5, 2023
1 check passed
@Pavan-Gunda Pavan-Gunda deleted the pg/local-volume-provisioner branch September 5, 2023 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[2] Option to install and configure local-volume-provisioner
4 participants