app: smarter deployment change detection #984
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, when setting the `override-commit` key in the origin, the upgrader pulls that commit checksum directly and then updates the refspec to point to it. This behaviour was inherited from its ostree version; at the time it was implemented, the pull code didn't support passing a specific commit for a given refspec. However, we now have the override-commit-ids option, which will make libostree update the ref for us. We change the code here to make use of it and simplify the function. This also fixes the corner case of local branches: we shouldn't change the ref if we're on a local branch. This is actually what drove me to this patch as I was debugging coreos#981. (Aside: I'm still not convinced updating the refspec is always the correct thing to do even in the remote case, though it's a bit messy to disentangle).
Also something I noticed while working on coreos#981. When sitting on a livefs commit, once a user does `rpm-ostree cleanup --pending --rollback`, it's impossible to redeploy the same booted commit. Let's allow users to do this.
Commands like `upgrade` and `deploy` need to know if a new deployment was actually laid down so that it may print a pkg diff if so. This is implemented by listening for changes to the DefaultDeployment D-Bus property. D-Bus emits a signal when the deployment variant changes value. However, in coreos#595, with the introduction of `pending-*` related keys, the deployment variant no longer represents data solely tied to that specific deployment. In this case, because `deploy` operations currently set the ref to the resolved checksum, it can happen that deploying the same base commit when the current refspec *isn't* pointing to that base commit will result in the `pending-*` keys dropping out and a default deployment change notification going out. In this patch, we strengthen how we determine whether a new deployment was laid down by actually looking at the deployment id, rather than just assuming that a change to the property implies a new deployment. Closes: coreos#981
|
Hmm, not sure what's going on with the CentOS one: Seems like the packages in the image are newer than those in the repos. |
cgwalters
reviewed
Sep 12, 2017
src/app/rpmostree-libbuiltin.c
Outdated
| g_autoptr(GVariantDict) dict = g_variant_dict_new (deployment); | ||
| g_autofree char *id; | ||
| g_assert (g_variant_dict_lookup (dict, "id", "s", &id)); | ||
| return g_steal_pointer (&id); |
There was a problem hiding this comment.
Why not just g_variant_lookup(deployment, "id", "&s", &id) ?
There was a problem hiding this comment.
Yeah, I had it return a new string because we weren't keeping a reference to the associated GVariant. In the new approach, we do.
src/app/rpmostree-libbuiltin.c
Outdated
| g_autoptr(GVariant) default_deployment = rpmostree_os_dup_default_deployment (os_proxy); | ||
| g_autofree char *original_id = get_id_from_deployment_variant (default_deployment); | ||
|
|
||
| /* we use a quark here so original_id automatically gets freed with os_proxy (but also as |
There was a problem hiding this comment.
I tend to view gobject data as a hack - it is a practical tool but the problem comes when you start using it a lot. Which we aren't at all, this would be the first. I'm OK keeping this but would be happier with a struct. Though hum I forget now...is there some reason we can't just query the deployment property before and after?
There was a problem hiding this comment.
No reason, just wanted to make it easy for callers to use it without much prep. :)
Though it turned out pretty straightforward still. Fixup! ⬇️
tests/vmcheck/test-basic.sh
Outdated
| # https://github.com/projectatomic/rpm-ostree/issues/981 | ||
| vm_rpmostree cleanup -rp | ||
| csum=$(vm_cmd ostree commit -b vmcheck --tree=ref=vmcheck) | ||
| # restart to make daemon see the pending cheksum |
|
Very nice work on all of this! The commit messages are great! |
|
Ah yep definitely nicer. Hmm...the signal before thing may have actually been an obscure workaround for the real bug in 454139d originally? |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Sep 12, 2017
Commands like `upgrade` and `deploy` need to know if a new deployment was actually laid down so that it may print a pkg diff if so. This is implemented by listening for changes to the DefaultDeployment D-Bus property. D-Bus emits a signal when the deployment variant changes value. However, in #595, with the introduction of `pending-*` related keys, the deployment variant no longer represents data solely tied to that specific deployment. In this case, because `deploy` operations currently set the ref to the resolved checksum, it can happen that deploying the same base commit when the current refspec *isn't* pointing to that base commit will result in the `pending-*` keys dropping out and a default deployment change notification going out. In this patch, we strengthen how we determine whether a new deployment was laid down by actually looking at the deployment id, rather than just assuming that a change to the property implies a new deployment. Closes: #981 Closes: #984 Approved by: cgwalters
|
☀️ Test successful - status-atomicjenkins |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commands like
upgradeanddeployneed to know if a new deploymentwas actually laid down so that it may print a pkg diff if so. This is
implemented by listening for changes to the
DefaultDeploymentD-Busproperty. D-Bus emits a signal when the deployment variant changes
value.
However, in #595, with the introduction of
pending-*related keys, thedeployment variant no longer represents data solely tied to that
specific deployment. In this case, because
deployoperations currentlyset the ref to the resolved checksum, it can happen that deploying the
same base commit when the current refspec isn't pointing to that base
commit will result in the
pending-*keys dropping out and a defaultdeployment change notification going out.
In this patch, we strengthen how we determine whether a new deployment
was laid down by actually looking at the deployment id, rather than just
assuming that a change to the property implies a new deployment.
Closes: #981