USHIFT-706: Make MicroShift's CSI plugin optionally deployable

[copejon]

No description provided.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign miciah for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[copejon]

/cc @jerpeter1 @pacevedom @jakobmoellerdev @suleymanakbas91

[openshift-ci-robot]

@copejon: This pull request references USHIFT-706 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[jakobmoellerdev]

Just made a first read through and gave my 2 cents where I had concerns. I realize this is not done though and just feel free to close out / ignore comments if they are irrelevant to the final design.

[jakobmoellerdev]

This storage space equivalent to ~60kB uncompressed, in the binary it is even less than that due to binary compression. Im not sure if this is even a relevant point.

[copejon]

I'm confused as to where the ~60Kb value comes from. Note that the sentence contains a typo and should have said "LVMS and CSI container images", referring to the size of the images on disk.

[jakobmoellerdev]

Ah, well, I was talking about the manifests, clearly didnt read that at full. Sorry for the confusion :) We will have only 2 images in the future, one for lvms and 1 for csi snapshotter, where the base layers will be shared with the release payload. I at expecting at most 100mi gain, but that is still relevant.

[jakobmoellerdev]

This is actually a problem in the way that microshift handles the apply, and is unrelated (imo) to how LVMS is packaged. There could be easily an uninstall via delete operations in k8s right now with the current method.

[copejon]

That's right. The sentence needs to be amended. It should be communicating that this is how it's done right now in MicroShift.

[jakobmoellerdev]

This is already possible today through the "enabled" option, I dont really see why this is not possible today.

[copejon]

MicroShift doesn't have a config option to enable/disable CSI. Can you point me to what your referring to?

[jakobmoellerdev]

I am referring to the fact that it is quite easily possible to build a flag or something similar into the config that disables the driver. I guess Im trying to convey that we dont need such a major rework for getting that done. There may be other reasons for the extraction such as modularity of the CSI driver of course.

[copejon]

Using a config field doesn't solve the goal of separating the LVMS project from microshift. It also poses a new set of complexities where as revoking LVMS's first-class citizen status in MicroShift would provide a much more simplified solution and reduce tech debt and testing requirements.

Also, it makes it even more difficult to add support for other CSI drivers later. The tech debt would grow at a linear rate because each driver would require it's own service manager in microshift to handle deployment and config.

Lastly, using a field implies that the container images must always be present in case the config changed from "off" to "on". We can't assume a network connection is reliable, or even exists. Therefore the container images would have to be installed when MicroShift is installed, regardless of whether the user ever intended to use dynamic storage.

[jakobmoellerdev]

LVMS / TopoLVM has custom logic paths in the microshift binary right now. If we want to have them in a seperate rpm, we need a new binary that is able to run as root and run these logic paths in its stead, especially for configuring lvmd.yaml with defaults. This will get only more pressing once we incorporate the hot reloading implementation in the LVMS migration from TopoLVM. Whats the idea to build that logic or run that somewhere?

[copejon]

I believe this is something that would be well suited to a %post-install in the rpm spec. The logic is relatively simple and is mostly process calls wrapped with go code. This script should also include logic to prevent stomping on existing config file during install so that user configs aren't lost after an upgrade/downgrade.

[jakobmoellerdev]

With the migration to LVMS, to allow the hot reloading mechanism of lvmd.yaml we need to actively watch and copy over /etc/microshift/lvmd.yaml to /var/lib/microshift/lvms/lvmd.yaml

Also we currently have on every restart a copying of that file, that behavior would need to change if we move to post-install.

Not generally opposed, just want to make sure we

• dont loose the ability to enable hotreloading in lvms for microshift (allows customers to reconfigure storage without restarting)
• dont break compatibility for ostree-style commits when upgrading

[copejon]

I think this is compatible with the hotreloading behavior. The post-install script would be applying the changes to the /etc/microshift/lvmd.yaml (if not already present). So if a user were to install LVMS on a running cluster, without providing an lvmd.yaml file, the hot-reload should detect the new file and perform as expected.

[jakobmoellerdev]

They also affect LVMS due Microshift and LVMS sharing a version dependency to each other. We will no longer need to release LVMS ahead of Microshift technically and can version it separately. That also means the RPMs need to be versioned and released separately. Otherwise I believe there is no gain here for extracting them

[ggiguash]

The gain is also to support another storage provider in the future.
We need to discuss how this change affects LVMS release cycle.

[ggiguash]

Looking at the rebase changes, we assume that the RPM is still built in the MicroShift repo, so we maintain the version dependency. In general, change the build procedure and full decoupling should be treated as a separate ER, I think.

[copejon]

Agreed, I think that the first step should remain scoped to the feature work.

[jakobmoellerdev]

I really think this needs to be revisited because this logic still has to exist somewhere.

[copejon]

I've added a section called RPM-Spec about this. LMKWYT

[jakobmoellerdev]

Looks really good already, im mostly nitting on details on the LVMS + Snapshotter and the RPM replacing the internals, consider a general LGTM from my side!

[jakobmoellerdev]

Im a bit confused by this statement. LVMS was never built with the same release image and had to be rebased, what would be the process for that now?

[copejon]

What I mean to say is that the LVMS and CSI versions are tracked by the operator bundle, which is then use to correlate the microshift version to the lvms/csi versions.

[jakobmoellerdev]

Could you quickly explain why a MicroShift config flag would not optionalize the installation of the components (maybe Im just missing something here)

[copejon]

Using a flag to optionalize storage has some drawbacks (off the top of my head):

• MicroShift carries some tech-debt for rebasing, deploying, and configuring LVMS.
• treating lvms as a first class citizen in microshift isn't necessary. it's just a cluster workload like any other CSI driver.
• should the default be 'on' or 'off'? what should happen if the toggle is switched from 'on' to 'off'? How ever these are answered means more responsibility for microshift, not less.
• disk consumption is another consideration. for offline deployments, should microshift still include the lvms images just in case users switch it on after starting microshift?
• Should MicroShift add support for another driver, how should this be implemented? An enum field would be fine in the config, but it'd also mean microshift would have to first-class that driver as well. This can't scale well at all.

[jakobmoellerdev]

How would we be able to run Dev Preview of the RPM and the old installation method through microshift core in parallel?

[ggiguash]

I do not think we need Dev or Tech preview. Let's release it in GA in 4.17.

[jakobmoellerdev]

This change IMO is A) too big to handle in one shot and B) will have too much impact on LVMS / MicroShift release procedure in 4.17 together with the LVMS MicroShift migration, it feels like we are forcing a big bang

[copejon]

Pushing back. From the user perspective, this is a relatively easy change to existing blueprints. Add this to new installs:

[packages]
name = "microshift-lvms"
version = "*"

Layers built without this and which are then pushed to 4.16 clusters are unaffected.

WRT the migration work, where do you see this becoming overly complex?

[jakobmoellerdev]

Mainly in making sure we are not breaking existing users together with the LVMS migration. This is more of a too big to handle for engineering and keep stable kind of thing for me. But Ill wait for some other opinions, Im not gonna be adamant about it :)

[jakobmoellerdev]

Since snapshotting can be used for any CSI driver, WDYT about microshift-snapshotting?

That way anyone who is just interested in snapshotting capability can install that RPM and then install their own CSI driver + VolumeSnapshotClass.

LVMS can already smartly create or ignore VolumeSnapshotClass creation so this would not need any extra logic paths.

[copejon]

Addressed in an earlier comment. LVMS and Snapshotting are considered as separate installs in the EP

[jakobmoellerdev]

I think we could drop that dependency and let LVMS do dynamic adjustment to wether VolumeSnapshotClass Support exists in the cluster.

[copejon]

That's a good idea, but it also means this work would be blocked until the LVMS change was accepted and implemented, which doesn't make sense. Instead, let this change and that one be developed in parallel at their own velocities. Whenever the LVMS change is implemented, it'd just require a one-line change in the microshift rpm spec.

[jakobmoellerdev]

Pro: Unblocks engineering on both fronts
Contra: this will mean that the rpm in the future might need to keep that dependency. Maybe Im just wondering why we dont have a microshift-snapshotting rpm with all the snapshotting stuff?

[ggiguash]

We need an estimate of the consumed resources so that it guides us further in the proposal

[ggiguash]

Are we planning to install a default lvmd.yaml file? If so, specifying it in the blueprint would override the default.
Let's clarify this please

[ggiguash]

Should we swap steps 4 and 5? I mean, when is the data orphaned?
Is it after RPM uninstall, or after the actions described in step 6?

[ggiguash]

The gain is also to support another storage provider in the future.
We need to discuss how this change affects LVMS release cycle.

[ggiguash]

Looking at the rebase changes, we assume that the RPM is still built in the MicroShift repo, so we maintain the version dependency. In general, change the build procedure and full decoupling should be treated as a separate ER, I think.

[ggiguash]

We will need to ensure that MicroShift can function normally without storage optional components.
It's true that 100% of tests currently assume the presence of CSI driver.

[copejon]

No MicroShift components require dynamically provisioned persistent storage, thus they have no dependency on CSI (LVMS or otherwise).

[ggiguash]

I do not think we need Dev or Tech preview. Let's release it in GA in 4.17.

[copejon]

It's been decided that an RPM-based approach to optional installation of LVMS and CSI components is neither feasible given the total changes required nor compatible with incoming changes (openshift/microshift#3535). This PR is closed.