Storage: Allow disabling of kOps's management of StorageClasses #10733

seh · 2021-02-04T23:01:23Z

Introduce a new "manageStorageClasses" field in the cloud provider portion of the Cluster specification to allow operators to opt out of kOps creating and reconciling StorageClass objects. The default value for the new "spec.cloudConfig.manageStorageClasses" field is true, to maintain compatibility with the existing behavior. When set to false, kOps will no longer create any StorageClass objects. Any such objects that kOps created in the past are left as is, and kOps and will no longer reconcile them against future changes.

The existing "spec.cloudConfig.openstack.blockStorage.createStorageClass" field remains in place. However, if both that and the new "spec.cloudConfig.manageStorageClasses" field are populated, they must agree: It is invalid both to disable management of StorageClass objects globally but to enable them for OpenStack and, conversely, to enable management globally but disable it for OpenStack.

Please see this discussion in the "kops-users" channel of the "Kubernetes" Slack workspace for the negotiation that led to this change. @zetaab, take note.

This change is

seh · 2021-02-04T23:03:58Z

Note that running hack/update-expected.sh changed 109 files. I didn't want to do that, but several integration tests failed until I allowed it to do its thing.

joshbranham · 2021-02-05T02:33:23Z

upup/models/cloudup/resources/addons/storage-aws.addons.k8s.io/v1.15.0.yaml

@@ -15,7 +15,7 @@ kind: StorageClass
 metadata:
  name: gp2
  annotations:
-    storageclass.beta.kubernetes.io/is-default-class: "false"
+    storageclass.kubernetes.io/is-default-class: "false"


I may be mistaken but I think the reason I left the old annotations was because k8s was reading both still? As in, not setting storageclass.beta.kubernetes.io/is-default-class: "false" on the gp2 class and trying to just set storageclass.kubernetes.io/is-default-class: "false" caused the same issues with which one ended up being the default. Would have to test to confirm though

Apply should remove all unset annotations.

olemarkus · 2021-02-05T05:43:35Z

pkg/model/components/cloudconfiguration.go

+	}
+
+	if c.ManageStorageClasses == nil {
+		c.ManageStorageClasses = fi.Bool(true)


I am not sure you really need this file since you need to use WithDefaultBool in the templates anyway. And you can set true as the default value there.

Okay. Since CloudConfig isn't set, you need this. I ran into nil pointer issues in my PR as well.

this change will break backwards compatbility with openstack. We cannot now update existing clusters because it says

spec.cloudConfig.manageStorageClasses: Forbidden: Management of storage classes and OpenStack block storage classes are both specified but disagree

Good catch!

This doesn't really break BC per se. It only breaks when you have explicitly disabled storageclass on OpenStack.

If we don't set this, it will break for pretty much everyone else. The error message seems clear on what actions users need to take.
Alternative is to ignore this field for OpenStack and document as such, but that may be more confusing for users? Could potentially lead to misconfigurations.

But open to suggestions how to fix this. I suggest at minimum a clear "Actions needed" in the release notes.

I did migration script for our clusters which runs kops set cluster spec.cloudConfig.manageStorageClasses=false after that it works fine. In my opinion it is pretty easy step? okay kops set is still behind featureflag? Anyways it works when executing that. So if people has disabled storageclass in openstack, they need to set manageStorageClasses=false, otherwise they cannot run kops update cluster or other commands

olemarkus · 2021-02-05T05:47:12Z

nodeup/pkg/model/tests/golden/awsiam/tasks-kube-apiserver.yaml

@@ -42,6 +42,7 @@ contents: |
      - --authorization-mode=AlwaysAllow
      - --bind-address=0.0.0.0
      - --client-ca-file=/srv/kubernetes/ca.crt
+      - --cloud-config=/etc/kubernetes/cloud.config


These do look very strange, but we make some funny assumptions about spec.cloudConfig. I guess at some point the intention was that this flag only was for configuring the cloud.config file, but then got (ab)used for other things later.

I think we always write cloud.config though, so it should be safe to always reference this file.

That change came by way of running hack/update-expected.sh, so it wasn't a conscious decision of mine to add the flag.

olemarkus · 2021-02-05T08:17:14Z

/lgtm

Leaving it unapproved in case someone else also wants to review.

seh · 2021-02-05T13:40:46Z

Another potential design we could consider here to allow more control over StorageClass objects in the future: introduce a "storageClasses" field with an "enabled" field within. It would look like this:

spec:
  cloudConfig:
    storageClasses:
      enabled: true

If we had that, we could later add finer-grained control over whether and which class gets nominated as the cluster default. Do you think that would be worth it?

Also, as noted in Slack, I found that when I populated my new field, all the ASG launch templates got updated, inducing machine replacement at the next kops rolling-update cluster invocation. That seems like too much churn, given that we shouldn't need to replace any machines to add a few StorageClass objects. Is that indicating that maybe "spec.cloudConfig" isn't the right place for this new field?

olemarkus · 2021-02-05T14:22:34Z

Another potential design we could consider here to allow more control over StorageClass objects in the future: introduce a "storageClasses" field with an "enabled" field within. It would look like this:
spec:
  cloudConfig:
    storageClasses:
      enabled: true
If we had that, we could later add finer-grained control over whether and which class gets nominated as the cluster default. Do you think that would be worth it?

Also, as noted in Slack, I found that when I populated my new field, all the ASG launch templates got updated, inducing machine replacement at the next kops rolling-update cluster invocation. That seems like too much churn, given that we shouldn't need to replace any machines to add a few StorageClass objects. Is that indicating that maybe "spec.cloudConfig" isn't the right place for this new field?

The roll happens because the kube-apiserver flags change. And that happens because cloudConfig is not nil anymore. I think this is a necessary one-time evil.

If you want to fix this, we need a better check around when to add the cloud-config flag.

seh · 2021-02-05T14:26:05Z

The roll happens because the kube-apiserver flags change.

I saw all the launch templates change—even for worker machines that don't host the Kubernetes API server—because the "spec.cloudConfig" field's value makes it into the conf/cluster_spec.yaml file included in the user data.

olemarkus · 2021-02-05T14:28:51Z

People don't change this too often though. So I am fine with leaving it. We have other options that triggers unecessary rolls, and we have changes that should trigger rolls that don't. Not ideal, but not easy to solve now.

seh · 2021-02-05T14:30:04Z

People don't change this too often though.

Yes, that's a good point. This isn't something we're likely to toggle more than once.

olemarkus · 2021-02-11T17:40:12Z

/lgtm

olemarkus · 2021-02-11T17:40:18Z

/approve

k8s-ci-robot · 2021-02-11T17:40:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus, seh

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:

~~OWNERS~~ [olemarkus]

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

…igin-release-1.20 Automated cherry pick of #10733: Define "ManageStorageClasses" cloud config field

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 4, 2021

k8s-ci-robot requested review from KashifSaadat and zetaab February 4, 2021 23:01

k8s-ci-robot added area/api area/nodeup size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 4, 2021

seh mentioned this pull request Feb 4, 2021

Add snapshot-controller #10730

Merged

joshbranham reviewed Feb 5, 2021

View reviewed changes

olemarkus reviewed Feb 5, 2021

View reviewed changes

k8s-ci-robot assigned olemarkus Feb 5, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2021

seh added 6 commits February 11, 2021 10:48

Define "ManageStorageClasses" cloud config field

f63a5d3

Validate CloudConfiguration values

4a7b970

Establish default CloudConfiguration values

d1432bf

Use graduated annotation for default StorageClass

0948a72

Make StorageClass manifests conditional on field

9f5b90e

Capture outcome of "hack/update-expected.sh" run

d44612c

seh force-pushed the allow-disabling-of-storage-class-mgmt branch from ca04fd6 to d44612c Compare February 11, 2021 15:50

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2021

k8s-ci-robot merged commit ec49519 into kubernetes:master Feb 11, 2021

k8s-ci-robot added this to the v1.20 milestone Feb 11, 2021

seh mentioned this pull request Feb 14, 2021

Automated cherry pick of #10733: Define "ManageStorageClasses" cloud config field #10821

Merged

k8s-ci-robot added a commit that referenced this pull request Feb 14, 2021

Merge pull request #10821 from seh/automated-cherry-pick-of-#10733-or…

19e2f37

…igin-release-1.20 Automated cherry pick of #10733: Define "ManageStorageClasses" cloud config field

seh mentioned this pull request Mar 9, 2021

Storage: Amend default choice for StorageClass management to honor a specified OpenStack-related value #11002

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage: Allow disabling of kOps's management of StorageClasses #10733

Storage: Allow disabling of kOps's management of StorageClasses #10733

seh commented Feb 4, 2021 •

edited

Loading

seh commented Feb 4, 2021

joshbranham Feb 5, 2021

olemarkus Feb 5, 2021

olemarkus Feb 5, 2021

olemarkus Feb 5, 2021

zetaab Mar 9, 2021

olemarkus Mar 9, 2021

zetaab Mar 9, 2021 •

edited

Loading

olemarkus Feb 5, 2021

seh Feb 5, 2021

olemarkus commented Feb 5, 2021

seh commented Feb 5, 2021

olemarkus commented Feb 5, 2021 •

edited

Loading

seh commented Feb 5, 2021 •

edited

Loading

olemarkus commented Feb 5, 2021

seh commented Feb 5, 2021

olemarkus commented Feb 11, 2021

olemarkus commented Feb 11, 2021

k8s-ci-robot commented Feb 11, 2021

Storage: Allow disabling of kOps's management of StorageClasses #10733

Storage: Allow disabling of kOps's management of StorageClasses #10733

Conversation

seh commented Feb 4, 2021 • edited Loading

seh commented Feb 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetaab Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olemarkus commented Feb 5, 2021

seh commented Feb 5, 2021

olemarkus commented Feb 5, 2021 • edited Loading

seh commented Feb 5, 2021 • edited Loading

olemarkus commented Feb 5, 2021

seh commented Feb 5, 2021

olemarkus commented Feb 11, 2021

olemarkus commented Feb 11, 2021

k8s-ci-robot commented Feb 11, 2021

seh commented Feb 4, 2021 •

edited

Loading

zetaab Mar 9, 2021 •

edited

Loading

olemarkus commented Feb 5, 2021 •

edited

Loading

seh commented Feb 5, 2021 •

edited

Loading