Skip to content

Manual verification for snap yaml schema#99

Merged
blackboxsw merged 4 commits into
cloud-init:masterfrom
lucasmoura:snap-yaml-check
Jun 8, 2020
Merged

Manual verification for snap yaml schema#99
blackboxsw merged 4 commits into
cloud-init:masterfrom
lucasmoura:snap-yaml-check

Conversation

@lucasmoura
Copy link
Copy Markdown

Manually verify behavior for #364 and #370

Since this issue is not related to a launchpad bug, I have named as:
gh-364.txt, where 364 is the number of one of the PRs related to that issue.

This naming idea was proposed by @raharper

Also, since I am addressing two distinct PRs, should I add both of them to the name of the file as well ?

Comment thread bugs/gh-364.txt Outdated
Comment on lines +34 to +83
for SERIES in bionic eoan focal xenial; do
echo '=== BEGIN ' $SERIES
ref=$SERIES-proposed
name=test-$SERIES
lxc delete $name --force 2> /dev/null
lxc-proposed-snapshot --add-archive ppa:~cloud-init-dev/proposed --publish $SERIES $ref | egrep "Creating|cloud-init"
lxc launch $ref $name
lxc file push snap.yaml $name/var/tmp/
lxc file push setup_dev_proposed.sh $name/var/tmp/
lxc exec $name -- cloud-init status --wait --long
lxc exec $name -- cloud-init devel schema -c /var/tmp/snap.yaml || echo "FAILURE"
done
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future, since you are really only testing updated commandline tooling here, you don't really need to use lxc-proposed-snapshot at all, just your lxc file push setup_dev_proposed.sh is sufficient.

The main benefit of using lxc-proposed-shapshot is create a base image that hasn't booted yet, which already has the updated cloud-init from the -proposed pocket.

In your case here it is unnecessary (especially because you are re-installing the setup_dev_proposed.sh version from our PPA even after you've already installed the xenial-proposed version from ubuntu proper.

It did look like in this case you pushed setup_dev_proposed.sh over to the container, but didn't run it, so we'll want to actually call it too.

And you probably also want you verification test to show the failure case before the upgrade to the new -poposed cloud-init

Unfortunately

lxc exec $name -- cloud-init devel schema -c /var/tmp/snap.yaml won't actually exit 1 for you if the config is valid yaml but uncovered by any defined schema. All it validates in that case is that the file begins with a #cloud-config header and contains valid YAML.

So in your schema tests, you'll probably want to provide an invalid-snap.yaml with invalid schema types for both assertions and commands (like providing a - value or something). assert in the first test that invalid_snap.yaml passes on current cloud-init because it is valid YAML, but happens to be invalid schema types.

Then you need to run lxc exec $name -- /var/tmp/setup_dev_proposed.sh to install -proposed version of cloud-init

And re-run cloud-init schema devel -c /var/tmp/invalid_snap.yaml to ensure it fails with the proper schema failure message, then run your valid_snap.yaml and ensure it passes.

In this test case I think the following makes sense:

Suggested change
for SERIES in bionic eoan focal xenial; do
echo '=== BEGIN ' $SERIES
ref=$SERIES-proposed
name=test-$SERIES
lxc delete $name --force 2> /dev/null
lxc-proposed-snapshot --add-archive ppa:~cloud-init-dev/proposed --publish $SERIES $ref | egrep "Creating|cloud-init"
lxc launch $ref $name
lxc file push snap.yaml $name/var/tmp/
lxc file push setup_dev_proposed.sh $name/var/tmp/
lxc exec $name -- cloud-init status --wait --long
lxc exec $name -- cloud-init devel schema -c /var/tmp/snap.yaml || echo "FAILURE"
done
for SERIES in bionic eoan focal xenial; do
echo '=== BEGIN ' $SERIES
ref=$SERIES-proposed
name=test-$SERIES
lxc delete $name --force 2> /dev/null
lxc launch ubuntu-daily:$SERIES $name
lxc file push valid_snap.yaml $name/var/tmp/
lxc file push invalid_snap.yaml $name/var/tmp/
lxc exec $name -- cloud-init status --wait --long
# add test on current cloud-init that both invalid and valid_snap.yaml pass (because we have no schema validation
lxc exec $name -- cloud-init devel schema -c /var/tmp/invalid_snap.yaml || echo "FAILURE"
lxc exec $name -- cloud-init devel schema -c /var/tmp/valid_snap.yaml || echo "FAILURE"
# upgrade cloud-init to proposed
lxc file push setup_dev_proposed.sh $name/var/tmp/
lxc exec $name -- bash /var/tmp/setup_dev_proposed.sh
# re test on current cloud-init that both invalid and valid_snap.yaml pass (because we have no schema validation
echo "Expect schema failure on invalid_snap"
lxc exec $name -- cloud-init devel schema -c /var/tmp/invalid_snap.yaml
echo "Expect success on valid_snap.yaml"
lxc exec $name -- cloud-init devel schema -c /var/tmp/valid_snap.yaml || echo "FAILURE"
lxc exec $name -- bash /var/tmp/setup_dev_proposed.sh
lxc exec $name -- cloud-init devel schema -c /var/tmp/snap.yaml || echo "FAILURE"
done

Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can you add a link for results to the top-level readme to this release. No need to create separate results files

diff --git a/20200529/README.md b/20200529/README.md
index 6b7dcff..8422f73 100644
--- a/20200529/README.md
+++ b/20200529/README.md
@@ -13,6 +13,7 @@ The links listed below are to bugs fixed in this SRU and the verification of tho
 ## SRU verification content
 | Bug | Verification Script and Output |
 | -------- |  -------- |
+| [# gh-364](http://github.com/canonical/cloud-init/364) and [# ghh-370](http://github.com/canonical/cloud-init/370) | [verification output](../bugs/gh-364.txt) |
 | [# 1880279](http://pad.lv/1880279) | [verification output](../bugs/lp-1880279.txt) |
 | [# 1858884](http://pad.lv/1858884) | [verification output](../bugs/lp-1858884.txt) |
 | [# 1876414](http://pad.lv/1876414) | [verification output](../bugs/lp-1876414.txt) |

@lucasmoura
Copy link
Copy Markdown
Author

@blackboxsw thanks for the review. Yes, your approach is way better than the one I proposed, specially for replicating the issue in the past version and then showing that in the updated version the problem goes away. I have also updated the README file to point to this bug as well

@lucasmoura lucasmoura requested a review from blackboxsw June 8, 2020 14:08
Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a couple minor nits left. probably want to test the invalid and valid yaml in the pre-upgrade case, and drop the proposed-snapshot script check

Comment thread bugs/gh-364.txt Outdated

lxc exec $name -- cloud-init status --wait --long
lxc exec $name -- cloud-init devel schema -c /var/tmp/invalid_snap.yaml || echo "FAILURE"
lxc exec $name -- cloud-init devel schema -c /var/tmp/invalid_snap.yaml || echo "FAILURE"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
lxc exec $name -- cloud-init devel schema -c /var/tmp/invalid_snap.yaml || echo "FAILURE"
lxc exec $name -- cloud-init devel schema -c /var/tmp/valid_snap.yaml || echo "FAILURE"

Comment thread bugs/gh-364.txt Outdated
Comment on lines +14 to +19
# Check if necessary scripts are found
if ! which lxc-proposed-snapshot >/dev/null; then
echo "Cannot find lxc-proposd-snapshot, get from qa-scripts repo";
return 1
fi

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed/used for this script

Suggested change
# Check if necessary scripts are found
if ! which lxc-proposed-snapshot >/dev/null; then
echo "Cannot find lxc-proposd-snapshot, get from qa-scripts repo";
return 1
fi

@lucasmoura
Copy link
Copy Markdown
Author

@blackboxsw done

@lucasmoura lucasmoura requested a review from blackboxsw June 8, 2020 16:20
@blackboxsw blackboxsw merged commit 1f976a4 into cloud-init:master Jun 8, 2020
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.

2 participants