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

LVM module #184

Merged
merged 71 commits into from Nov 10, 2016

Conversation

Projects
None yet
5 participants
@avnik
Copy link
Contributor

avnik commented Aug 22, 2016

This is straight rewrite of mantl-storage-setup.py (at the moment)

Architecture caveats:
1 we need to install lvm tools, and start lvm-metad service before we start
and we can't issue install requirement in lvm module.
2 we need to express all LVM structure in single hcl expression, otherwise we
can't plan/check VG/LV part, until PV/VG part commited.

Possible future ways:

  • all storage/lvm structure as single expression
  • step-by-step executing to eliminate difference between "desired state" (by hcl expression) and "current state" (which exposed by lvs/pvs/vgs commands)

Funny go language issues:
we can exec.Command(name, args...) but not `exec.Command(name, arg, more_args...)``

So let discuss it

@avnik

This comment has been minimized.

Copy link
Contributor

avnik commented Aug 22, 2016

suggestions how to make LVM related code testable also welcomed

@rebeccaskinner

View changes

resource/lvm/vg/util.go Outdated
}
}
return "", fmt.Errorf("too many symlinks for %s", dev)
}

This comment has been minimized.

@rebeccaskinner

rebeccaskinner Aug 23, 2016

Contributor

Could we use filepath.EvalSymlinks here?

https://golang.org/pkg/path/filepath/#EvalSymlinks

This comment has been minimized.

@avnik

avnik Aug 23, 2016

Contributor

definelly yes

@rebeccaskinner

View changes

load/resource.go Outdated
@@ -58,6 +59,9 @@ func SetResources(ctx context.Context, g *graph.Graph) (*graph.Graph, error) {
case "file.content":
dest = new(content.Preparer)

case "lvm.vg":

This comment has been minimized.

@rebeccaskinner

rebeccaskinner Aug 23, 2016

Contributor

Would using lvm.VolumeGroup be more clear here?

This comment has been minimized.

@avnik

avnik Aug 23, 2016

Contributor

yes, anyway is just prototype, I not sure if we still use this syntax

@rebeccaskinner

This comment has been minimized.

Copy link
Contributor

rebeccaskinner commented Aug 23, 2016

Could we consider using LogicalVolume, PhysicalVolume and VolumeGroup instead of lv, pv and vg? I know that's more verbose than the tool itself but might make scripts more readable for people who aren't as familiar with lvm?

@avnik avnik force-pushed the feature/lvm branch 2 times, most recently to 8768f24 Sep 12, 2016

@stevendborrelli stevendborrelli added this to the 0.3 milestone Sep 17, 2016

@BrianHicks BrianHicks removed the in progress label Sep 19, 2016

@avnik avnik force-pushed the feature/lvm branch from 8768f24 to 4e05de7 Sep 26, 2016

@avnik avnik force-pushed the feature/lvm branch 2 times, most recently from 1911304 to af116b9 Oct 3, 2016

@stevendborrelli stevendborrelli changed the title [WIP] LVM proof-of-concept LVM module Oct 6, 2016

@stevendborrelli

This comment has been minimized.

Copy link
Member

stevendborrelli commented Oct 6, 2016

please make sure your example has a check for the pvs, etc. commands or it will fail

@stevendborrelli

This comment has been minimized.

Copy link
Member

stevendborrelli commented Oct 6, 2016

Errors are not clear during a failure. You are logging tasks in INFO but it is not clear why this is failing:

root/lvm.lv.lv-test:
    Messages:
    Has Changes: no
    Changes: No changes

root/lvm.fs.mnt-me:
    Error: exit status 2
    Messages:
    Has Changes: no
    Changes: No changes

root/lvm.vg.mantl:
    Messages:
    Has Changes: no
    Changes: No changes

Summary: 2 errors, 0 changes

 * root: error in dependency "root/lvm.fs.mnt-me"
 * root/lvm.fs.mnt-me: exit status 2
}

lvm.fs "mnt-me" {
device = "/dev/mapper/mantl-test"

This comment has been minimized.

@stevendborrelli

stevendborrelli Oct 6, 2016

Member

this should be /dev/mapper/lv-test

@stevendborrelli

This comment has been minimized.

Copy link
Member

stevendborrelli commented Oct 6, 2016

  1. The module should check for the lvm commands (pvs, lvs, .etc.) before trying to execute.
  2. errors are not propagated back to the user
  3. I think some of the checks are not working. I am getting a clear on the /mnt/ filesystem even though it is not mounted. Perhaps the checks needs to go a little deeper and confirm the right configuration.

We really need to throw a lot of errors at it and make sure it is providing helpful feedback.

@stevendborrelli

This comment has been minimized.

Copy link
Member

stevendborrelli commented Oct 6, 2016

Are pvs created automatically? I think it would be helpful to separate that out as a module

root/lvm.vg.mantl:
    Messages:
    Has Changes: yes
    Changes:
        /dev/sdb: "<none>" => "member of volume group mantl"
        mantl: "<not exists>" => "/dev/sdb"

Not really clear what is happening here. Is the lv being created or the vg?

root/lvm.lv.lv-test:
    Messages:
    Has Changes: yes
    Changes:
        group: mantl: "<not exists>" => "created"

@avnik avnik force-pushed the feature/lvm branch from 095298b to c00ba39 Oct 14, 2016

@arichardet

This comment has been minimized.

Copy link
Contributor

arichardet commented Oct 14, 2016

@avnik You have failing tests - please take a look and make appropriate changes. Thanks!

@stevendborrelli stevendborrelli modified the milestones: 0.4.0, 0.3.0 Oct 14, 2016

@avnik avnik force-pushed the feature/lvm branch from 96bbe8c to 4b4975a Oct 18, 2016

@@ -0,0 +1,18 @@
lvm.vg "mantl" {

This comment has been minimized.

@arichardet

arichardet Oct 18, 2016

Contributor

Please remove "mantl" in all places and replace with "test" or something else.

This comment has been minimized.

@avnik

avnik Oct 19, 2016

Contributor

done (also reoved from history)

@@ -0,0 +1,18 @@
lvm.vg "mantl" {
name = "mantl"
devices = [ "/dev/sdb" ]

This comment has been minimized.

@arichardet

arichardet Oct 18, 2016

Contributor

can you change this to a parameter so users can provide the value; you could set "/dev/sdb" as default.

)

type ResourceFS struct {
mount *Mount

This comment has been minimized.

@arichardet

arichardet Oct 18, 2016

Contributor

mount, lvm, etc. should be capitalized since the struct is exported

device, err := render.Render("device", p.Device)
if err != nil {
return nil, err
}

This comment has been minimized.

@arichardet

arichardet Oct 18, 2016

Contributor

This is no longer necessary and is handled for you. You can simply set "What" to p.device in Mount, etc.

group, err := render.Render("group", p.Group)
if err != nil {
return nil, err
}

This comment has been minimized.

@arichardet

arichardet Oct 18, 2016

Contributor

These checks are now done for you; just use the value of p.Group, etc.

func (*OsExec) Run(prog string, args []string) error {
log.WithField("module", "lvm").Infof("Executing %s: %v", prog, args)
e := exec.Command(prog, args...).Run()
if e == nil {

This comment has been minimized.

@arichardet

arichardet Oct 18, 2016

Contributor

Probably only need the err != nil case here.

"testing"
)

func TestExecRun(t *testing.T) {

This comment has been minimized.

@arichardet

arichardet Oct 18, 2016

Contributor

It might be easier to follow if you have one Test[FuncName], and then use t.Run to test the different cases of success and failure resulting in error. Using t.Run is what we are trying to do now, so updating throughout is probably a good idea.

)

type ResourceLV struct {
group string

This comment has been minimized.

@arichardet

arichardet Oct 18, 2016

Contributor

group, name, etc. should be capitalized since this struct is exported.

status := &Status{}
ok, err := r.checkVG()
if err != nil {
return nil, err

This comment has been minimized.

@arichardet

arichardet Oct 18, 2016

Contributor

Probably a good idea to wrap the errors with "check", "apply", etc. to give the user more information.


pvs, err := r.lvm.QueryPhysicalVolumes()
if err != nil {
return nil, err

This comment has been minimized.

@arichardet

arichardet Oct 18, 2016

Contributor

Wrapping errors here in check and apply would be helpful for the user.

@avnik avnik force-pushed the feature/lvm branch 5 times, most recently from 5ecc8c0 to 092c008 Oct 19, 2016

avnik and others added some commits Oct 31, 2016

lvm: whitespace in comments
CodeClimate aggroing on then
lvm: rename resources to full names
Also move init() to preparer.go as in all other resources
lvm/vg: More tests
coverage: 98.2% of statements
Rebecca Skinner
Rebecca Skinner
Rebecca Skinner

@rebeccaskinner rebeccaskinner force-pushed the feature/lvm branch from 0b02675 to 19c98dc Nov 10, 2016

Rebecca Skinner added some commits Nov 10, 2016

Rebecca Skinner
Rebecca Skinner
Rebecca Skinner
Rebecca Skinner
Rebecca Skinner

@arichardet arichardet merged commit 11beb43 into master Nov 10, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
wercker/build Wercker pipeline passed
Details

@arichardet arichardet deleted the feature/lvm branch Nov 10, 2016

@arichardet arichardet referenced this pull request Nov 14, 2016

Closed

LVM module #270

BrianHicks pushed a commit that referenced this pull request Dec 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment