OCPBUGS-9996: make lvms configuration more robust #1489

dhellmann · 2023-03-11T23:52:51Z

Instead of requiring the user to configure their system with a "rhel"
volume group, look at the volume groups that exist and pick one as the default. If there is only 1, use it. If there is a "microshift" VG, use it. Otherwise, disable the CSI driver.

/assign @copejon

openshift-ci-robot · 2023-03-11T23:52:59Z

@dhellmann: This pull request references Jira Issue OCPBUGS-9996, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jogeo

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Instead of requiring the user to configure their system with a "rhel"
volume group, look at the volume groups that exist and pick one as the
default. If there is a "rhel" VG, use it. Otherwise, select the
largest volume group. If there are no volume groups at all, disable
the CSI driver.

/assign @copejon

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 kubernetes/test-infra repository.

dhellmann · 2023-03-11T23:54:28Z

/cherry-pick release-4.13

openshift-cherrypick-robot · 2023-03-11T23:54:30Z

@dhellmann: once the present PR merges, I will cherry-pick it on top of release-4.13 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.13

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 kubernetes/test-infra repository.

dhellmann · 2023-03-12T13:54:13Z

/test e2e-reboot
/test e2e-openshift-conformance-reduced

copejon · 2023-03-14T15:30:33Z

I'm pretty hesitant to make educated guesses about users' systems. For one, it means user's can't expect uniform default behavior from the platform and will get different results based on their storage stack. It'd be safer instead to select the VG that backs the user's homedir. This would mesh well with current dev env we suggest with a microshift user, be predictable, and allow users to select special VGs if they want. Making the 'rhel' name special also makes the storage configuration just a little bit more esoteric - one more reason to assume home VG if none are provided.

ggiguash · 2023-03-14T15:34:06Z

Maybe, we should not attempt selecting any volumes if 'rhel' does not exist and nothing is specified in the config?
Disabling CSI in case of a non-existent config is great.

dhellmann · 2023-03-14T16:47:00Z

I'm pretty hesitant to make educated guesses about users' systems. For one, it means user's can't expect uniform default behavior from the platform and will get different results based on their storage stack. It'd be safer instead to select the VG that backs the user's homedir. This would mesh well with current dev env we suggest with a microshift user, be predictable, and allow users to select special VGs if they want. Making the 'rhel' name special also makes the storage configuration just a little bit more esoteric - one more reason to assume home VG if none are provided.

The point about predictability is fair. There is no user with a home directory, so I'm not sure how to use that to be more deterministic. Just look for /home? Maybe the volume group where / is would be a better rule?

How many hosts where someone is likely to run MicroShift actually have multiple volume groups?

Maybe, we should not attempt selecting any volumes if 'rhel' does not exist and nothing is specified in the config?
Disabling CSI in case of a non-existent config is great.

Disabling if there is no 'rhel' VG and the user has not provided a config file would be more predictable. It's not a minimal configuration for a nice out-of-the-box experience, though, so I was hoping to make things "just work."

ggiguash · 2023-03-14T17:01:04Z

My vote would be to make it simple and robust rather than "clever". There's no end to users' imagination, so we may inadvertedly end up in situations where the volume selection logic does not behave well.

dhellmann · 2023-03-14T18:19:56Z

My vote would be to make it simple and robust rather than "clever". There's no end to users' imagination, so we may inadvertedly end up in situations where the volume selection logic does not behave well.

It's not selecting a volume. It's selecting a volume group. The vast majority of systems running MicroShift are only going to have 1 of those.

dhellmann · 2023-03-14T19:07:00Z

@ggiguash @copejon , I've changed the logic to prefer "rhel" but choose the single existing VG if it is named otherwise. If there are multiples, then it will require the user to configure the driver explicitly.

copejon · 2023-03-14T20:55:00Z

@ggiguash @copejon , I've changed the logic to prefer "rhel" but choose the single existing VG if it is named otherwise. If there are multiples, then it will require the user to configure the driver explicitly.

+1 I think that's a pretty reasonable fallback.

ggiguash · 2023-03-15T12:14:50Z

@ggiguash @copejon , I've changed the logic to prefer "rhel" but choose the single existing VG if it is named otherwise. If there are multiples, then it will require the user to configure the driver explicitly.

I totally agree that's the best approach. Thank you, @dhellmann !

ggiguash · 2023-03-15T12:14:59Z

/retest

ggiguash · 2023-03-15T12:16:14Z

@dhellmann, do we need to update GitHub docs with this change? I think we need to at least mention this new logic in the config doc, maybe a CSI design as well.

dhellmann · 2023-03-15T15:37:09Z

@dhellmann, do we need to update GitHub docs with this change? I think we need to at least mention this new logic in the config doc, maybe a CSI design as well.

Good point. See cb4e618. While I was working on that, I made a few other changes in that file in other commits.

dhellmann · 2023-03-15T18:20:49Z

/test e2e-openshift-conformance-reduced

dhellmann · 2023-03-15T19:15:55Z

CI system timed out accessing images

/test e2e-openshift-conformance-reduced

docs/default_csi_plugin.md

ggiguash · 2023-03-16T11:02:05Z

pkg/config/lvmd/lvmd.go

+// DefaultLvmdConfig returns a configuration struct for Lvmd with
+// default settings based on the current host. If the default volume
+// group name "rhel" is found, that volume group is used, otherwise
+// the largest volume group is used. If there are no volume groups,
+// the configuration returned will report that it is not enabled (see
+// IsEnabled()).


A nit: the comment needs to be fixed. The code no longer picks the largest volume group.

I fixed this and added some unit tests in the latest draft.

dhellmann · 2023-03-17T13:18:33Z

/test e2e-greenboot

dhellmann · 2023-03-18T14:41:24Z

/retest

dhellmann · 2023-03-18T19:11:09Z

/retest

Instead of requiring the user to configure their system with a "rhel" volume group, look at the volume groups that exist and apply some selection rules: * If there is a "rhel" VG, use it. * If there is only one VG, use it. * Otherwise, disable the CSI driver with a log message.

When there are multiple volume groups, look for one named "microshift" to use, instead of "rhel".

ggiguash · 2023-03-19T09:31:00Z

@dhellmann , the kutiltest is failing with the following errors in the CI:

   case.go:364: failed in step 30-
    case.go:366: deployments.apps "topolvm-controller" not found
    case.go:366: daemonsets.apps "topolvm-node" not found

Looks like the CI is falling back to the case with CSI disabled.

Mar 18 19:49:28 release-ci-ci-op-tdxlnnbc-7f12f microshift[212336]: infrastructure-services-manager I0318 19:49:28.259617  212336 lvmd.go:70] Multiple volume groups available but no configuration file is present, disabling CSI. [rhel ]
Mar 18 19:49:28 release-ci-ci-op-tdxlnnbc-7f12f microshift[212336]: infrastructure-services-manager I0318 19:49:28.259682  212336 storage.go:81] CSI is disabled. %sMultiple volume groups are available, but no configuration file was provided.

ggiguash · 2023-03-19T16:31:32Z

@dhellmann, last error remaining:

# github.com/openshift/microshift/pkg/config/lvmd [github.com/openshift/microshift/pkg/config/lvmd.test]
pkg/config/lvmd/lvmd_test.go:22:5: unknown field 'DisabledReason' in struct literal of type Lvmd
pkg/config/lvmd/lvmd_test.go:90:5: unknown field 'DisabledReason' in struct literal of type Lvmd

Include a message in the body of the file added to the config map to explain where the content came from.

dhellmann · 2023-03-19T17:39:05Z

@dhellmann, last error remaining:

# github.com/openshift/microshift/pkg/config/lvmd [github.com/openshift/microshift/pkg/config/lvmd.test]
pkg/config/lvmd/lvmd_test.go:22:5: unknown field 'DisabledReason' in struct literal of type Lvmd
pkg/config/lvmd/lvmd_test.go:90:5: unknown field 'DisabledReason' in struct literal of type Lvmd

That's what I get for not running the unit tests locally before pushing. :-/

openshift-ci-robot · 2023-03-19T18:06:54Z

@dhellmann: This pull request references Jira Issue OCPBUGS-9996, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jogeo

In response to this:

Instead of requiring the user to configure their system with a "rhel"
volume group, look at the volume groups that exist and pick one as the default. If there is only 1, use it. If there is a "microshift" VG, use it. Otherwise, disable the CSI driver.

/assign @copejon

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 kubernetes/test-infra repository.

ggiguash · 2023-03-19T20:26:10Z

/lgtm

openshift-ci · 2023-03-19T20:26:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, ggiguash

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~~ [dhellmann,ggiguash]

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

openshift-ci · 2023-03-19T21:29:41Z

@dhellmann: all tests passed!

Full PR test history. Your PR dashboard.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot · 2023-03-19T21:32:36Z

@dhellmann: Jira Issue OCPBUGS-9996: All pull requests linked via external trackers have merged:

openshift/microshift#1489

Jira Issue OCPBUGS-9996 has been moved to the MODIFIED state.

In response to this:

Instead of requiring the user to configure their system with a "rhel"
volume group, look at the volume groups that exist and pick one as the default. If there is only 1, use it. If there is a "microshift" VG, use it. Otherwise, disable the CSI driver.

/assign @copejon

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 kubernetes/test-infra repository.

openshift-cherrypick-robot · 2023-03-19T21:33:23Z

@dhellmann: new pull request created: #1530

In response to this:

/cherry-pick release-4.13

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 kubernetes/test-infra repository.

openshift-ci bot assigned copejon Mar 11, 2023

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Mar 11, 2023

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Mar 11, 2023

openshift-ci bot requested review from jogeo, benluddy and pacevedom March 11, 2023 23:53

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2023

dhellmann force-pushed the OCPBUGS-9996-lvms-default-vg branch from d3d9d1f to 4ed902f Compare March 14, 2023 19:06

copejon reviewed Mar 15, 2023

View reviewed changes

docs/default_csi_plugin.md Show resolved Hide resolved

ggiguash reviewed Mar 16, 2023

View reviewed changes

dhellmann force-pushed the OCPBUGS-9996-lvms-default-vg branch from d978782 to c70ee9c Compare March 16, 2023 19:21

dhellmann added 5 commits March 19, 2023 04:02

OCPBUGS-9996: update documentation explaining how the VG is picked

e5fd538

update LVMS documentation about configuration file location

cbedf69

our LVMS is not a fork, it is a build of upstream

b5fc30a

OCPBUGS-9996: Change default VG name to "microshift"

01502d3

When there are multiple volume groups, look for one named "microshift" to use, instead of "rhel".

dhellmann force-pushed the OCPBUGS-9996-lvms-default-vg branch from 764fb4c to 01502d3 Compare March 19, 2023 14:20

OCPBUGS-9996: show where we get the lvmd configuration

5d313f8

Include a message in the body of the file added to the config map to explain where the content came from.

dhellmann force-pushed the OCPBUGS-9996-lvms-default-vg branch from c5e813a to 5d313f8 Compare March 19, 2023 17:37

openshift-ci bot assigned ggiguash Mar 19, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2023

openshift-merge-robot merged commit cd656cd into openshift:main Mar 19, 2023
14 checks passed

openshift-cherrypick-robot mentioned this pull request Mar 19, 2023

[release-4.13] OCPBUGS-10242: make lvms configuration more robust #1530

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-9996: make lvms configuration more robust #1489

OCPBUGS-9996: make lvms configuration more robust #1489

dhellmann commented Mar 11, 2023 •

edited

openshift-ci-robot commented Mar 11, 2023

dhellmann commented Mar 11, 2023

openshift-cherrypick-robot commented Mar 11, 2023

dhellmann commented Mar 12, 2023

copejon commented Mar 14, 2023

ggiguash commented Mar 14, 2023 •

edited

dhellmann commented Mar 14, 2023

ggiguash commented Mar 14, 2023 •

edited

dhellmann commented Mar 14, 2023

dhellmann commented Mar 14, 2023

copejon commented Mar 14, 2023

ggiguash commented Mar 15, 2023

ggiguash commented Mar 15, 2023

ggiguash commented Mar 15, 2023

dhellmann commented Mar 15, 2023

dhellmann commented Mar 15, 2023

dhellmann commented Mar 15, 2023

ggiguash Mar 16, 2023

dhellmann Mar 16, 2023

dhellmann commented Mar 17, 2023

dhellmann commented Mar 18, 2023

dhellmann commented Mar 18, 2023

ggiguash commented Mar 19, 2023 •

edited

ggiguash commented Mar 19, 2023

dhellmann commented Mar 19, 2023

openshift-ci-robot commented Mar 19, 2023

ggiguash commented Mar 19, 2023

openshift-ci bot commented Mar 19, 2023

openshift-ci bot commented Mar 19, 2023

openshift-ci-robot commented Mar 19, 2023

openshift-cherrypick-robot commented Mar 19, 2023

OCPBUGS-9996: make lvms configuration more robust #1489

OCPBUGS-9996: make lvms configuration more robust #1489

Conversation

dhellmann commented Mar 11, 2023 • edited

openshift-ci-robot commented Mar 11, 2023

dhellmann commented Mar 11, 2023

openshift-cherrypick-robot commented Mar 11, 2023

dhellmann commented Mar 12, 2023

copejon commented Mar 14, 2023

ggiguash commented Mar 14, 2023 • edited

dhellmann commented Mar 14, 2023

ggiguash commented Mar 14, 2023 • edited

dhellmann commented Mar 14, 2023

dhellmann commented Mar 14, 2023

copejon commented Mar 14, 2023

ggiguash commented Mar 15, 2023

ggiguash commented Mar 15, 2023

ggiguash commented Mar 15, 2023

dhellmann commented Mar 15, 2023

dhellmann commented Mar 15, 2023

dhellmann commented Mar 15, 2023

ggiguash Mar 16, 2023

Choose a reason for hiding this comment

dhellmann Mar 16, 2023

Choose a reason for hiding this comment

dhellmann commented Mar 17, 2023

dhellmann commented Mar 18, 2023

dhellmann commented Mar 18, 2023

ggiguash commented Mar 19, 2023 • edited

ggiguash commented Mar 19, 2023

dhellmann commented Mar 19, 2023

openshift-ci-robot commented Mar 19, 2023

ggiguash commented Mar 19, 2023

openshift-ci bot commented Mar 19, 2023

openshift-ci bot commented Mar 19, 2023

openshift-ci-robot commented Mar 19, 2023

openshift-cherrypick-robot commented Mar 19, 2023

dhellmann commented Mar 11, 2023 •

edited

ggiguash commented Mar 14, 2023 •

edited

ggiguash commented Mar 14, 2023 •

edited

ggiguash commented Mar 19, 2023 •

edited