Bug 623808

Note that at the command line level it is the vgcreate and vgextend commands that set pe_start (not pvcreate).

And a reminder of the rule that all library functionality must also be available through the LVM command line.

So we're perhaps talking about enhancing the way --test can be used to provide this information and incorporating --test into the library.

(In reply to comment #1)
> Note that at the command line level it is the vgcreate and vgextend commands
> that set pe_start (not pvcreate).

pe_start is established via pvcreate.  vgcreate can do the implicit PV creation but pvcreate does lay down the pe_start without having to add the PV to a VG.

You're correct, thanks.  So it's pvcreate.  That simplifies the problem a little.  I'm having dreams about building 'pvs' into 'pvcreate' etc....
I.e. allowing tools to output selected fields relating to the objects they have manipulated.  (Particularly useful with --test, but may be workable more generally.)

liblvm (lvm2app) will provide access to pe_start via the new generic property functions submitted upstream.  Some discussion still happening but likely to be checked in soon.

The existing liblvm code can provide the information, but it can't create the required object (PV, in this case) without writing labels. We need to defer PV creation to the VG write/commit time, which will make it possible to create PVs the same way as LVs are created: create the structures (which makes it possible to query them) and decide later whether to write the changes to disk or not.

This requires somewhat extensive changes to current code: split pv_write in format_handler into two (adjust PV fields and actually write the label), move the actual label writes into vg_write and keep around a list of writes to do.

This is turning out to be a bit too tricky, partly due to MDA wiping and partly due to lvmcache interference. One catch is, *none* of our tests fails if I simply comment out MDA wiping, so there's clearly no testing whatsoever done to verify it works. Meaning we first need to design tests to cover this area, if we want to know that we aren't breaking things.

Moreover, Peter Rajnoha is working on the same area of code, but his changes won't make it 6.1. So I am un-acking this, and bumping to 6.2, where it is more realistic to achieve correctly and without compromising stability. Luckily, the changes done by Peter should make this a lot more feasible.

Development Management has reviewed and declined this request.  You may appeal
this decision by reopening this request.

This is now halfway done and blocked on the review of this patch: http://www.redhat.com/archives/lvm-devel/2011-April/msg00056.html

The "hard" patches have been now merged (hard meaning they were potentially disruptive for normal LVM used, unrelated to liblvm). The liblvm part of the change should be relatively easy now.

QE reviewed this BZ for QA_ACK but was unable to ack due to a lack of
requirements or description of how the new feature is supposed to work or be
tested.

Please see
https://wiki.test.redhat.com/ClusterStorage/WhyNoAck

Turns out that no further changes are needed to facilitate this. I have added a test to our upstream test suite for the functionality. For completeness, I am reproducing here the C program that demonstrates how to use the requested feature:

#include "lvm2app.h"
#include "assert.h"

int main(int argc, char *argv[])
{
	lvm_t handle;
	vg_t vg = NULL;
	pv_t pv;
	struct lvm_property_value v;

	handle = lvm_init(NULL);
        assert(handle);

	vg = lvm_vg_create(handle, argv[1]);
        assert(vg);

	if (lvm_vg_extend(vg, argv[2]))
		abort();

	pv = lvm_pv_from_name(vg, argv[2]);
	assert(pv);

        v = lvm_pv_get_property(pv, "pe_start");
        assert(v.is_valid);
	fprintf(stderr, "pe_start = %d\n", (int)v.value.integer);
        assert(v.value.integer == 2048 * 512);

        lvm_vg_close(vg);
	lvm_quit(handle);
        return 0;
}

(of course, the assertions are appropriate for testing, not for production code)

The above code, according to the requirements set forth in this bugzilla, should give the correct "pe_start" value for a given volume group, even if the volume group does not physically exist, but has only been created in memory.

(gosh, sorry for the misformatted code... let me try again)

#include "lvm2app.h"
#include "assert.h"

int main(int argc, char *argv[])
{
	lvm_t handle;
	vg_t vg = NULL;
	pv_t pv;
	struct lvm_property_value v;

	handle = lvm_init(NULL);
	assert(handle);

	vg = lvm_vg_create(handle, argv[1]);
	assert(vg);

	if (lvm_vg_extend(vg, argv[2]))
		abort();

	pv = lvm_pv_from_name(vg, argv[2]);
	assert(pv);

	v = lvm_pv_get_property(pv, "pe_start");
	assert(v.is_valid);
	fprintf(stderr, "pe_start = %d\n", (int)v.value.integer);
	assert(v.value.integer == 2048 * 512);

	lvm_vg_close(vg);
	lvm_quit(handle);
	return 0;
}

Adding QA ack for 6.2. 

Based on comment #12, a test for this API feature already exists in the lvm devel test suite. 

This bug will be marked verified (SanityOnly) once lvm devel shows unit test results and final 6.2 regression testing has been completed.

Petr - Can you please add the output from the test run of the test in comment #13, so that I can mark this verified.

Running api/pe_start.sh ...                      @TESTDIR=/srv/build/lvm2/cvs-clean/default/test/LVMTEST21976.NusqXjqwP8
@PREFIX=LVMTEST21976

aux prepare_devs 2
+ aux prepare_devs 2
## preparing loop device....ok (/dev/loop1)
## preparing 2 devices...ok
aux apitest pe_start test_vg $dev1
+ aux apitest pe_start test_vg /srv/build/lvm2/cvs-clean/default/test/LVMTEST21976.NusqXjqwP8/dev/mapper/LVMTEST21976pv1
  No physical volume label read from /srv/build/lvm2/cvs-clean/default/test/LVMTEST21976.NusqXjqwP8/dev/mapper/LVMTEST21976pv1
pe_start = 1048576
not vgs test_vg
+ not vgs test_vg
  Volume group "test_vg" not found
not pvs $dev1                               
+ not pvs /srv/build/lvm2/cvs-clean/default/test/LVMTEST21976.NusqXjqwP8/dev/mapper/LVMTEST21976pv1
  No physical volume label read from /srv/build/lvm2/cvs-clean/default/test/LVMTEST21976.NusqXjqwP8/dev/mapper/LVMTEST21976pv1                        
  Failed to read physical volume "/srv/build/lvm2/cvs-clean/default/test/LVMTEST21976.NusqXjqwP8/dev/mapper/LVMTEST21976pv1"                          
aux teardown                                
+ aux teardown                              
## teardown......ok                         
passed.    0:00                             

The test itself, pe_test.c, reads:

#undef NDEBUG

#include "lvm2app.h"
#include "assert.h"

int main(int argc, char *argv[])
{
        lvm_t handle;
        vg_t vg = NULL;
        pv_t pv;
        struct lvm_property_value v;

        handle = lvm_init(NULL);
        assert(handle);

        vg = lvm_vg_create(handle, argv[1]);
        assert(vg);

        if (lvm_vg_extend(vg, argv[2]))
                abort();

        pv = lvm_pv_from_name(vg, argv[2]);
        assert(pv);

        v = lvm_pv_get_property(pv, "pe_start");
        assert(v.is_valid);
        fprintf(stderr, "pe_start = %d\n", (int)v.value.integer);
        assert(v.value.integer == 2048 * 512);

        lvm_vg_close(vg);
        lvm_quit(handle);
        return 0;
}

Marking verified based on comment #17.

    Technical note added. If any revisions are required, please edit the "Technical Notes" field
    accordingly. All revisions will be proofread by the Engineering Content Services team.
    
    New Contents:
Before, it was not possible to create a PV object with all properties calculated (e.g. the PE start value) without a need to write the PV label on disk while using LVM2 library (lvm2app). This has been changed so that the PV label is written out later in the process as part of the lvm_vg_write call making it possible to calculate all PV properties and query them without actually writing the PV label on disk.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2011-1522.html