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.

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

OXT-787 : Populate meta-openxt-qt #1

Merged
merged 1 commit into from
Dec 5, 2016

Conversation

dozylynx
Copy link

Signed-off-by: Christopher Clark [email protected]

Signed-off-by: Christopher Clark <[email protected]>
@dozylynx
Copy link
Author

Please could one of the repository maintainers let me know the next steps to proceed with this PR -- thanks.

@jean-edouard
Copy link
Member

Could you maybe provide a list of steps to follow to build and test these changes, in case someone out there is interested in reviewing these PRs?
Thanks

@cjp256
Copy link
Member

cjp256 commented Nov 15, 2016

As most of those bits were lifted from my github, so I'll have to say it looks good ;)

Is it worth moving db.default into its own recipe, so it's not duplicated? Perhaps not.

I don't have cycles atm to do any testing of it though.

@dozylynx
Copy link
Author

@cjp256 : thanks for the review and I think you're right that db.default probably does belong in its own recipe. I can respin an update to this PR to do that. Also: thanks for the original code!

@jean-edouard : This PR is paired with this corresponding one:
OpenXT/openxt#200

After the changes from both of these are applied, there is no immediate change to the build since the layer file is not enabled by default -- it deliberately does not have the ".layer" suffix that would enable it. If the layer is manually enabled, by removing ".optional" from the layer definition file in openxt.git, then running build-scripts/setup.sh will install it, and subsequent builds will include the qtdbd daemon.

Enabling and disabling the layer in between individual builds is performed by renaming the two layer definition files in the build user's home directory:

  • 40-meta-qt5.layer.optional
  • 45-meta-openxt-qt5.layer.optional

to add or remove the ".optional" suffix.

So after applying, testing for this PR should cover:

  • building with this layer disabled
    • build needs to succeed
    • dbd should be the OCaml implementation
    • dbd needs to function as expected
  • building with this layer enabled
    • build needs to succeed
    • dbd should be the Qt implementation
    • dbd needs to function as expected

The dbd version can be deduced via ldd /usr/bin/dbd in dom0. The OCaml version indicates:

    linux-gate.so.1 (0xb77b8000)
    libdbus-1.so.3 => /usr/lib/libdbus-1.so.3 (0xb775c000)
    libpthread.so.0 => /lib/libpthread.so.0 (0xb773f000)
    libm.so.6 => /lib/libm.so.6 (0xb76e9000)
    libdl.so.2 => /lib/libdl.so.2 (0xb76e3000)
    librt.so.1 => /lib/librt.so.1 (0xb76da000)
    libc.so.6 => /lib/libc.so.6 (0xb7523000)
    /lib/ld-linux.so.2 (0xb77b9000)

whereas the Qt variant would show a Qt library dependency.

dbd is exercised in typical VM lifecycle operations and a simple smoke test should be sufficient to check this change is OK.

@dozylynx
Copy link
Author

Actually, after a bit of consideration, I think moving the db.default file out will be better handled in a subsequent PR that does just that change. I'm happy to do that; this PR can still go in the meantime.

@dpsmith
Copy link
Member

dpsmith commented Nov 26, 2016

An initial build with this PR combined with PR #200 is available here for others that want to test.

@dpsmith
Copy link
Member

dpsmith commented Nov 27, 2016

Build was tested with one minor defect and moderate defect found with qtdbd itself.

Testing consisted of,

  • bats - Only failures there were relating to the fact that SELinux was in permissive mode in the build. This is do to the flux in xenclient-oe/master and unrelated to inclusion of qtdbd.
  • Dom0 - Exercised each of the db commands for comparison against OCaml version's behavior. A minor defect was detected when running a scenario of removing an entry in a section and then re-adding the entry. In this case the /touchpad section was the target. In the OCaml version when an entry is added, it will appear as the last entry in the section. Qtdbd does not exhibit this behavior and while the new entry's location is predictable, i.e. placed in the same location each time the test was run, without reviewing the code the method of placement is not obvious and is not the same behavior as the ocaml version.
  • NDVM - Exercised the db commands allowed by standard rpc policy, read, list(ls), exists, write, and rm. Note read_binary is allowed by policy but this is the interface used by db-cat and has been broke since 4.0. A moderate defect occurred when testing db-ls-dom0, the command timed-out and a review of the Dom0 log file showed that db-ls-dom0 command was attempting to use the dump method of the db interface. The OCaml version is able to produce a listing.

Before accepting the community needs to decide if qtdbd must reproduce the exact behavior of the OCaml version or if this approximation is acceptable. The moderate defect can be resolved by allowing the dump method to be called by domains with domstore-read-access boolean is enabled in their configuration.

@rossphilipson
Copy link
Contributor

So it seems there are two issues. I am very concerned about the first one where the order of nodes does not match in the db after re-writing one. The second issue concerning db-ls-dom0 seems like something that needs to be fixed in qtdbd. I believe the right way to fix it is to make qtdbd do whatever ocaml dbd does to do the ls rather than allowing new RPCs.

@cjp256
Copy link
Member

cjp256 commented Nov 29, 2016

  1. If the user is expected order of nodes to match, then the user is asking for problems. It's JSON... If they need to organize/sort data in a particular manner, they should be doing it themselves.
  2. Feel free to PR a change to qtdbd modifying db-ls as desired. However, dump() is far more efficient rather than attempting to walk the tree otherwise. Db-ls really shouldn't be used as anything other than a debug/print tool for the user...

@rossphilipson
Copy link
Contributor

On #2 we can alternately allow the dump method to be used in service VMs (for use cases where ls is needed). I am ok with this approach.

@dpsmith
Copy link
Member

dpsmith commented Dec 5, 2016

Going to merge this in but will not merge the openxt.git portion that activates this. For addressing the policy, instead of doing a one-off fix lets have a discussion on the mailing list what the servicevm db interface should look like. There are those that use db-ls as part of production, and not just as debug, since that was the only interface allowed by policy. Those that are concerned with policy can then provide input on what might be best and then we can address short-comings in tools.

@dpsmith dpsmith merged commit 1552950 into OpenXT:master Dec 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants