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.

Sign up for GitHub

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

OCPBUGS-34227: fix: actually pass through and validate chunk size #643

Merged
merged 1 commit into from
Jun 25, 2024
Merged

OCPBUGS-34227: fix: actually pass through and validate chunk size #643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions internal/controllers/vgmanager/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
"github.com/openshift/lvm-operator/internal/controllers/vgmanager/lvm"
"github.com/openshift/lvm-operator/internal/controllers/vgmanager/lvmd"
"github.com/openshift/lvm-operator/internal/controllers/vgmanager/wipefs"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/utils/ptr"

corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -570,6 +569,10 @@ func (r *Reconciler) validateLVs(ctx context.Context, volumeGroup *lvmv1alpha1.L
"you should manually extend the metadata_partition or you will risk data loss: metadata_percent: %v", metadataPercentage, lv.MetadataPercent)
}

if err := verifyChunkSizeForPolicy(volumeGroup.Spec.ThinPoolConfig, lv); err != nil {
return err
}

logger.V(1).Info("confirmed created logical volume has correct attributes", "lv_attr", lvAttr.String())
}
if !thinPoolExists {
Expand Down Expand Up @@ -611,19 +614,24 @@ func (r *Reconciler) addThinPoolToVG(ctx context.Context, vgName string, config
}

logger.Info("creating lvm thinpool")
if err := r.LVM.CreateLV(ctx, config.Name, vgName, config.SizePercent, convertChunkSize(config.ChunkSize)); err != nil {
if err := r.LVM.CreateLV(ctx, config.Name, vgName, config.SizePercent, convertChunkSize(config)); err != nil {
return fmt.Errorf("failed to create thinpool: %w", err)
}
logger.Info("successfully created thinpool")

return nil
}

func convertChunkSize(size *resource.Quantity) int64 {
if size == nil {
// convertChunkSize converts the chunk size from the ThinPoolConfig to the correct value for the LVM API
// if the ChunkSizeCalculationPolicy is set to Host, it will return -1, signaling the LVM API to use the Host value.
func convertChunkSize(config *lvmv1alpha1.ThinPoolConfig) int64 {
if config.ChunkSizeCalculationPolicy == lvmv1alpha1.ChunkSizeCalculationPolicyHost {
return -1
}
if config.ChunkSize == nil {
return lvmv1alpha1.ChunkSizeDefault.Value()
}
return size.Value()
return config.ChunkSize.Value()
}

func (r *Reconciler) extendThinPool(ctx context.Context, vgName string, lvSize string, config *lvmv1alpha1.ThinPoolConfig) error {
Expand Down Expand Up @@ -741,3 +749,20 @@ func (r *Reconciler) NormalEvent(ctx context.Context, obj *lvmv1alpha1.LVMVolume
r.Event(obj, corev1.EventTypeNormal, string(reason),
fmt.Sprintf("update on node %s: %s", client.ObjectKeyFromObject(nodeStatus), message))
}

func verifyChunkSizeForPolicy(config *lvmv1alpha1.ThinPoolConfig, lv lvm.LogicalVolume) error {
if config.ChunkSizeCalculationPolicy == lvmv1alpha1.ChunkSizeCalculationPolicyHost {
// Host policy means that the chunk size is not set by the user, but by the host and cannot be validated
// against the spec
return nil
}
if chunkSizeBytes, err := strconv.ParseInt(lv.ChunkSize, 10, 64); err == nil {
if chunkSizeBytes != convertChunkSize(config) {
return fmt.Errorf("chunk size of logical volume %s does not match the static policy: %w", lv.Name, err)
}
} else {
return fmt.Errorf("could not parse chunk size from logical volume %s to verify against static policy: %w", lv.Name, err)
}

return nil
}
2 changes: 2 additions & 0 deletions internal/controllers/vgmanager/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"os"
"path/filepath"
"strconv"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -313,6 +314,7 @@ func testVGWithLocalDevice(ctx context.Context, vgTemplate lvmv1alpha1.LVMVolume
LvAttr: "twi---tz--",
LvSize: "1.0G",
MetadataPercent: "10.0",
ChunkSize: strconv.FormatInt(ptr.To(resource.MustParse("128Ki")).Value(), 10),
}
createdVG = lvm.VolumeGroup{
Name: vg.GetName(),
Expand Down
32 changes: 29 additions & 3 deletions internal/controllers/vgmanager/lvm/lvm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,18 @@ const (
lvmsTag = "@lvms"
)

var (
DefaultListLVColumns = []string{
"lv_name",
"vg_name",
"pool_lv",
"lv_attr",
"lv_size",
"metadata_percent",
"chunk_size",
}
)

// VGReport represents the output of the `vgs --reportformat json` command
type VGReport struct {
Report []struct {
Expand Down Expand Up @@ -85,6 +97,7 @@ type LogicalVolume struct {
LvAttr string `json:"lv_attr"`
LvSize string `json:"lv_size"`
MetadataPercent string `json:"metadata_percent"`
ChunkSize string `json:"chunk_size"`
}

type LVM interface {
Expand Down Expand Up @@ -402,7 +415,15 @@ func (hlvm *HostLVM) ListLVsByName(ctx context.Context, vgName string) ([]string
func (hlvm *HostLVM) ListLVs(ctx context.Context, vgName string) (*LVReport, error) {
res := new(LVReport)
args := []string{
"-S", fmt.Sprintf("vgname=%s", vgName), "--units", "g", "--reportformat", "json",
"-S",
fmt.Sprintf("vgname=%s", vgName),
"--units",
"b",
"--nosuffix",
"--reportformat",
"json",
"-o",
strings.Join(DefaultListLVColumns, ","),
}
if err := hlvm.RunCommandAsHostInto(ctx, res, lvsCmd, args...); err != nil {
return nil, err
Expand Down Expand Up @@ -462,8 +483,13 @@ func (hlvm *HostLVM) CreateLV(ctx context.Context, lvName, vgName string, sizePe
return fmt.Errorf("failed to create logical volume in volume group: size percent should be greater than 0")
}

args := []string{"-l", fmt.Sprintf("%d%%FREE", sizePercent),
"-c", fmt.Sprintf("%vb", chunkSizeBytes), "-Z", "y", "-T", fmt.Sprintf("%s/%s", vgName, lvName)}
args := []string{"-l", fmt.Sprintf("%d%%FREE", sizePercent), "-Z", "y", "-T"}

if chunkSizeBytes > 0 {
args = append(args, "-c", fmt.Sprintf("%vb", chunkSizeBytes))
}

args = append(args, fmt.Sprintf("%s/%s", vgName, lvName))

if err := hlvm.RunCommandAsHost(ctx, lvCreateCmd, args...); err != nil {
return fmt.Errorf("failed to create logical volume %q in the volume group %q using command '%s': %w",
Expand Down
26 changes: 23 additions & 3 deletions internal/controllers/vgmanager/validatelvs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ import (
mockExec "github.com/openshift/lvm-operator/internal/controllers/vgmanager/exec/test"
"github.com/openshift/lvm-operator/internal/controllers/vgmanager/lvm"
"github.com/stretchr/testify/assert"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/ptr"
"k8s.io/utils/strings/slices"
"sigs.k8s.io/controller-runtime/pkg/log"
)
Expand Down Expand Up @@ -51,7 +53,7 @@ var mockLvsOutputThinPoolValid = `
"report": [
{
"lv": [
{"lv_name":"thin-pool-1", "vg_name":"vg1", "lv_attr":"twi-a-tz--", "lv_size":"26.96g", "pool_lv":"", "origin":"", "data_percent":"0.00", "metadata_percent":"10.52", "move_pv":"", "mirror_log":"", "copy_percent":"", "convert_lv":""}
{"lv_name":"thin-pool-1", "vg_name":"vg1", "lv_attr":"twi-a-tz--", "lv_size":"26.96g", "pool_lv":"", "origin":"", "data_percent":"0.00", "metadata_percent":"10.52", "move_pv":"", "mirror_log":"", "copy_percent":"", "convert_lv":"", "chunk_size": "524288"}
]
}
]
Expand Down Expand Up @@ -105,9 +107,12 @@ func TestVGReconciler_validateLVs(t *testing.T) {
"-S",
"vgname=vg1",
"--units",
"g",
"b",
"--nosuffix",
"--reportformat",
"json",
"-o",
strings.Join(lvm.DefaultListLVColumns, ","),
}

mockExecutorForLVSOutput := func(output string) lvmexec.Executor {
Expand Down Expand Up @@ -135,11 +140,26 @@ func TestVGReconciler_validateLVs(t *testing.T) {
args: args{volumeGroup: &lvmv1alpha1.LVMVolumeGroup{
ObjectMeta: metav1.ObjectMeta{Name: "vg1", Namespace: "default"},
Spec: lvmv1alpha1.LVMVolumeGroupSpec{ThinPoolConfig: &lvmv1alpha1.ThinPoolConfig{
Name: "thin-pool-1",
Name: "thin-pool-1",
ChunkSize: ptr.To(resource.MustParse("512Ki")),
}},
}},
wantErr: assert.NoError,
},
{
name: "Invalid LV due to Chunk Size",
fields: fields{
executor: mockExecutorForLVSOutput(mockLvsOutputRAID),
},
args: args{volumeGroup: &lvmv1alpha1.LVMVolumeGroup{
ObjectMeta: metav1.ObjectMeta{Name: "vg1", Namespace: "default"},
Spec: lvmv1alpha1.LVMVolumeGroupSpec{ThinPoolConfig: &lvmv1alpha1.ThinPoolConfig{
Name: "thin-pool-1",
ChunkSize: ptr.To(resource.MustParse("1Ki")),
}},
}},
wantErr: assert.Error,
},
{
name: "Invalid LV due to Type not being Thin Pool",
fields: fields{
Expand Down