Deployment: add end-of-life #889

peterbaouoft · 2017-07-21T00:46:43Z

When metadata contains end-of-life attribute,
its information will be added to the deployment Variant,
which will later be shown in rpm-ostree status command

peterbaouoft · 2017-07-21T02:16:17Z

src/daemon/rpmostreed-deployment-utils.c

+    {
+      g_variant_lookup (metadata, "version", "s", &version_commit);
+      /* avoid double insertion */
+      if (prefix != NULL)


do we only want base commit deployment to have this end-of-life attribute, or all type of deployments should contain end-of-life attribute if they exist?

I think end-of-life is only for base commits, right? It's something we get from the remote repository. Now we could also reflect it in the derived commit, but I'd say it's simplest to only store the data once.

peterbaouoft · 2017-07-21T13:15:01Z

related to #142

peterbaouoft · 2017-07-21T13:24:54Z

@cgwalters any thoughts? Thanks in advance :D

cgwalters · 2017-07-21T13:30:22Z

src/daemon/rpmostreed-deployment-utils.c

+    {
+      g_variant_lookup (metadata, "version", "s", &version_commit);
+      /* avoid double insertion */
+      if (prefix != NULL)


I think end-of-life is only for base commits, right? It's something we get from the remote repository. Now we could also reflect it in the derived commit, but I'd say it's simplest to only store the data once.

cgwalters · 2017-07-21T13:30:40Z

src/daemon/rpmostreed-deployment-utils.c

+      g_variant_lookup (metadata, "version", "s", &version_commit);
+      /* avoid double insertion */
+      if (prefix != NULL)
+        g_variant_lookup(metadata, "end-of-life", "s", &end_of_life);


Code style has space between identifier and parenthesis, i.e. g_variant_lookup (metadata, ....

Also, for ostree metadata there's a convention to use namespace prefixes; for example, most of the metadata that rpm-ostree uses starts with rpmostree., like rpmostree.inputhash. Theversion metadata key should probably have been ostree.version.

I think we should have this metadata defined in ostree, so it'd be ostree.end-of-life. In fact...this is even an example in ostree today:

$ git describe --tags v2017.8-28-ge87102b7 $ git grep end-of-life man/ostree-summary.xml: <command>exampleos.end-of-life "@t 1445385600"</command>. src/libostree/ostree-repo.h: * `exampleos.end-of-life`. The `ostree.` prefix is reserved.

Maybe we make a document upstream that has well-known metadata keys?

Done in ostreedev/ostree#1024

I also noted that we also have ostree.endoflife mentioned in the test suite; I think for consistency we should change the bits above to be ostree.endoflife since we already support ostree.endoflife-rebase. Want to review my PR and once it lands, do a patch on top of it?

Also, for ostree metadata there's a convention to use namespace prefixes

So in order to match the namespace prefixes, it will be 'g_variant_lookup(metadata, "ostree.end-of-life", "s", &end_of_life)' instead?

I think we should have this metadata defined in ostree, so it'd be ostree.end-of-life. In fact...this is even an example in ostree today:

I was thinking earlier that end-of-life attribute may show up in metadata only if OS vendor add it with the commit like
'ostree commit --add-detached-metadata-string="endoflife =" ' , so each commit will potentially not have end-of-life attribute at all. I am assuming if we have this metadata defined in ostree, we would need to include it in every ostree commit?

ah, I missed the last comment for end-of-life(was typing the respone), currently looking it :p.

I also noted that we also have ostree.endoflife mentioned in the test suite; I think for consistency we should change the bits above to be ostree.endoflife since we already support ostree.endoflife-rebase. Want to review my PR and once it lands, do a patch on top of it?

sure, currently looking it

cgwalters · 2017-07-21T13:30:58Z

src/app/rpmostree-builtin-status.c

+
+      if (end_of_life_string)
+        {
+          g_print("%s", end_of_life_string);


Space between identifier and paren; we also usually don't do curly braces for single lines, but I'm fine keeping it too.

jlebon · 2017-07-21T14:41:06Z

src/app/rpmostree-builtin-status.c

+
+      if (end_of_life_string)
+        {
+          g_print("%s", end_of_life_string);


Can we prefix it with something, e.g. end-of-life: %s ?

peterbaouoft · 2017-07-21T15:59:38Z

bot, retest this please

peterbaouoft · 2017-07-21T19:13:22Z

It seems like when you have multiple deployments(could be wrong), end of life string will be displayed multiple times, because the end-of-life appears in the deployment dictionary, currently working on to find a fix for that. (p.s: problem involves a bit more stuff than I thought, will reconsider this and submit another one)

jlebon · 2017-07-24T19:24:58Z

src/daemon/rpmostreed-deployment-utils.c

-    base_checksum = g_strdup (csum);
+    {
+      base_checksum = g_strdup (csum);
+      /* we would not consider endoflife attribute for layered commits */


I think we still want layered commits to "inherit" the end-of-life aspect though, right? I.e. when I do rpm-ostree status, I still want to see the end-of-life message even if my commit is a layered one, right?

So really, I think we should just always call variant_add_metadata_attribute() with base_commit after this if-statement.

ah, I was thinking that, once the user has updated the commit to the newest, if end_of_life attribute is present, I am thinking that user should not be able to perform actions such as install/upgrade any more because the upstream branch stopped updating?

As a result, I think if we do not offer install/upgrade actions for that particular upstream branch, theoretically layered commit containing end-of-life attribute should not exist?( I am assuming that layered commits can only be achieved via install/upgrade actions, could be wrong tho)

Hmm, I think it still makes sense to allow pkg-layering on the end-of-life commit though. E.g. if a user isn't ready to rebase to the new branch, we don't want to prevent them from staying there as long as they need to by adding artificial limits, right? The status output will still be there to remind them that they're at the end of the line. :)

hmm, for that case I agree, your way makes more sense overall :), will work on to change it.

jlebon · 2017-07-24T19:26:21Z

src/daemon/rpmostreed-deployment-utils.c

+    {
+      g_variant_lookup (metadata, attribute, "s", &attribute_string_value);
+      if (attribute_string_value != NULL)
+        g_variant_dict_insert (dict, attribute, "s", attribute_string_value);


Hmm, it might be called ostree.endoflife in the ostree commit metadata, but is that what we want it to be called in the deployment GVariant too? The key name chosen here shows up also in the output of rpm-ostree status --json. Maybe just end-of-life would be more appropriate there?

jlebon · 2017-07-24T19:28:09Z

Remember to drop the "WIP:" prefix from the PR title before merging, otherwise the merge bot won't accept it!

peterbaouoft · 2017-07-25T00:45:56Z

bot, retest this please

peterbaouoft · 2017-07-25T00:57:39Z

bot, retest this please

peterbaouoft · 2017-07-25T13:13:15Z

bot, retest this please

peterbaouoft · 2017-07-25T14:04:40Z

bot, retest this please

jlebon · 2017-07-25T14:26:01Z

src/daemon/rpmostreed-deployment-utils.c

+      if (attribute_string_value != NULL)
+        {
+          /* for attributes starting with ostree., remove the prefix */
+          if(g_str_has_prefix (attribute, "ostree."))


Nit: space between if and (.

jlebon · 2017-07-25T14:27:39Z

src/app/rpmostree-builtin-status.c

+      g_variant_dict_lookup (dict, "endoflife", "&s", &end_of_life_string);
+
+      if (end_of_life_string)
+       g_print ("\n end-of-life: %s", end_of_life_string);


Nit: there's a space between the new line and the "end-of-life" string. Is that intended?

oops, that space was put there just to make the code look "neater", will remove

jlebon · 2017-07-25T14:30:59Z

src/daemon/rpmostreed-deployment-utils.c

-    base_checksum = g_strdup (csum);
+    {
+      base_checksum = g_strdup (csum);
+      /* we would not consider endoflife attribute for layered commits */


Hmm, I think it still makes sense to allow pkg-layering on the end-of-life commit though. E.g. if a user isn't ready to rebase to the new branch, we don't want to prevent them from staying there as long as they need to by adding artificial limits, right? The status output will still be there to remind them that they're at the end of the line. :)

jlebon · 2017-07-25T14:33:10Z

src/daemon/rpmostreed-deployment-utils.c

+      g_variant_lookup (metadata, attribute, "s", &attribute_string_value);
+      if (attribute_string_value != NULL)
+        {
+          /* for attributes starting with ostree., remove the prefix */


This seems kinda arbitrary to generalize given that it's only used for one field right now. I'd just make attribute and new_attribute parameters, though it's up to you!

jlebon · 2017-07-25T14:34:59Z

src/daemon/rpmostreed-deployment-utils.c

+          if(g_str_has_prefix (attribute, "ostree."))
+            {
+              const char *new_string = strchr (attribute, '.');
+              const char *new_attribute = new_string + 1;


You could just do const char *new_attribute = attribute + strlen ("ostree."); here, right?

ah, nice point :D, did not think about that

peterbaouoft · 2017-07-25T18:00:18Z

umm it seems like for f26-compose, the test fails quite frequently with Error: Failed to synchronize cache for repo 'updates', but I dont think any code related to the stuff I added is being called yet, and I dont think any rpm-ostree command is being called yet. is that common for other tests too? @jlebon

cgwalters · 2017-07-25T19:28:49Z

Yeah, it's not related to your changes. See projectatomic/papr#53

peterbaouoft · 2017-07-26T00:21:09Z

bot, retest this please

jlebon · 2017-07-26T20:58:51Z

src/app/rpmostree-builtin-status.c

+      g_variant_dict_lookup (dict, "endoflife", "&s", &end_of_life_string);
+
+      if (end_of_life_string)
+       g_print ("\nEndOfLife: %s", end_of_life_string);


Actually, (sorry for the bikeshedding!), any reason we don't just print it like any other kv using print_kv? (Preferably in red & bold as well).

ah, no worries at all, all of this is helping me to improve(when contributing) next time :D

For the print, there is actually no reason, I just did not think about that :p (may consider more carefully to follow the style of the file next time :p). I also agree it might be better to add the red & bold for the highlights, and use print_kv similar to other key/value pairs. :)

jlebon · 2017-07-27T14:13:28Z

That looks great! The only thing missing is a test. :) Maybe just something in vmcheck/test-basic.sh that creates a new commit with the eol set, then upgrades, and checks that it made it in the status JSON?

peterbaouoft · 2017-07-27T14:30:13Z

sure, will take some time to learn the style of vmcheck tests, but working on it :)

peterbaouoft · 2017-07-27T19:16:44Z

hmm, @jlebon, making an "upstream commit"( assuming that is how upgrade can be performed ) seems difficult, is it fine to just do local ostree commits and a layered commit to check for the functionality?( or is there some documents for simulating a fake "upstream commit" for upgrade?)

jlebon · 2017-07-27T20:04:30Z

There's a couple of examples around, e.g. check maybe in test-layering-relayer.sh. It goes something like this:

vm_cmd ostree commit -b vmcheck --tree=ref=vmcheck

In your case of course, you'd add --add-metadata-string and then do an upgrade (the test VM will already be on the vmcheck branch). To check that it works for layered commits as well, you can do something like:

vm_build_rpm foo
vm_rpmostree install foo
# check JSON of deployment 0 here using e.g. vm_assert_status_jq or vm_get_deployment_info

Maybe put your test in test-basic.sh? Or you can create a new file if you'd prefer!

When commit metadata contains ostree.endoflife attribute, its information will be added to the deployment Variant, which will later be shown as a red & bold message when 'rpm-ostree status' command is called. A test is added for future regression

jlebon · 2017-07-28T14:13:14Z

tests/vmcheck/test-basic.sh

+# Build a layered commit and check if EndOfLife still present
+vm_build_rpm foo
+vm_rpmostree install foo
+vm_assert_status_jq ".deployments[0][\"endoflife\"] == \"${META_ENDOFLIFE_MESSAGE}\""


Maybe add an "ok" message here? (See the echo "ok ..." messages up above). These msgs show up in the test output and make it easier to narrow down where testing stopped.

jlebon · 2017-07-28T14:38:11Z

Great! 👍 LGTM.
I'll give @cgwalters time to have a final look.

cgwalters · 2017-07-28T15:05:51Z

tests/vmcheck/test-basic.sh

@@ -59,3 +59,17 @@ echo "ok status doesn't require root"
 # Also check that we can do status as non-root non-active
 vm_cmd runuser -u bin rpm-ostree status
 echo "ok status doesn't require active PAM session"
+
+# Add metadata string containing EnfOfLife attribtue
+META_ENDOFLIFE_MESSAGE="this_is_a_test"


Any reason not to use spaces in this? We need that to work.

e.g. META_ENDOFLIFE_MESSAGE="Game over! Insert 25¢ to continue."

ah, the main reason is probably I dont know how to handle with quotes related syntax in shell , thus trying to stay away from it ( a bad habit from school I would say :( ). Currently working on to add it.

cgwalters · 2017-07-28T15:06:18Z

tests/vmcheck/test-basic.sh

+# Add metadata string containing EnfOfLife attribtue
+META_ENDOFLIFE_MESSAGE="this_is_a_test"
+commit=$(vm_cmd ostree commit -b vmcheck \
+            --tree=ref=vmcheck --add-metadata-string=ostree.endoflife=$META_ENDOFLIFE_MESSAGE)


Then this would need to be --add-metadata-string=ostree.endoflife="${META_ENDOFLIFE_MESSAGE}".

the above implementation seems not functioning,( which I have no clue why), it seems like when I have the above implementation, there is a random single quote get introduced.
Here is the test output with the above implementation, (the line with single quote starts with the second line)
https://paste.fedoraproject.org/paste/fNGo0KGnJKH~2Q3VSRvOTQ

However, the implementation below seems to pass the test,
--add-metadata-string=ostree.endoflife=\"${META_ENDOFLIFE_MESSAGE}\",
but seeing the testing output, it makes me more confused
https://paste.fedoraproject.org/paste/OAenqkrjH-MALc8m6MPzhw, some single quote gets inserted again. However, the output seems valid(no failures) when the double quote gets escaped( but IDK why too .....)

I was trying to look for some problems similar online, but was unable to do so, is there a possible reason for this behaviour(introduction of single quotes) to happen?

The issue you're probably hitting is the fact that ssh (note vm_cmd uses ssh) in essence does an sh -c "$cmd $arg1 $arg2 ...". This basically means you lose one level of quoting. So you have to combat that by adding another level of quoting. See https://unix.stackexchange.com/a/212298/40299.

ah I see, that makes sense :)

cgwalters · 2017-07-28T17:17:23Z

bot, add author to whitelist

cgwalters · 2017-08-02T13:53:08Z

Sorry, I didn't notice that you'd pushed another fixup here! Currently in github we tend to add a comment after pushing an update.

Looks good to me, thanks!

@rh-atomic-bot r+ 467c40d

rh-atomic-bot · 2017-08-02T13:53:13Z

⌛ Testing commit 467c40d with merge 53c3963...

rh-atomic-bot · 2017-08-02T14:11:22Z

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing 53c3963 to master...

dustymabe · 2018-09-05T21:56:56Z

if I was going to run an rpm-ostree compose and I wanted to resulting commit to have an end of line message. how would that be achieved ?

cgwalters · 2018-09-05T21:59:05Z

Probably the best way is to just "recommit" an existing content set; that's what happens in the tests, see:
https://github.com/projectatomic/rpm-ostree/pull/889/files#diff-b161f799eb3e1f08b09b7cb0fb0c250bR66

cgwalters · 2018-09-05T22:13:02Z

That said of course

# rpm-ostree compose tree --help|grep metadata
  --add-metadata-string=KEY=VALUE     Append given key and value (in string format) to metadata
  --add-metadata-from-json=JSON       Parse the given JSON file as object, convert to GVariant, append to OSTree commit

dustymabe · 2018-09-05T23:29:58Z

thanks! that really helps

peterbaouoft commented Jul 21, 2017

View reviewed changes

cgwalters reviewed Jul 21, 2017

View reviewed changes

jlebon reviewed Jul 21, 2017

View reviewed changes

peterbaouoft force-pushed the end_of_life_notification branch 2 times, most recently from 0522b95 to d99608b Compare July 21, 2017 15:53

peterbaouoft closed this Jul 21, 2017

peterbaouoft reopened this Jul 21, 2017

peterbaouoft changed the title ~~Deployment: add end-of-life~~ WIP:Deployment: add end-of-life Jul 21, 2017

peterbaouoft changed the title ~~WIP:Deployment: add end-of-life~~ "WIP: "Deployment: add end-of-life Jul 21, 2017

peterbaouoft changed the title ~~"WIP: "Deployment: add end-of-life~~ WIP: Deployment: add end-of-life Jul 21, 2017

peterbaouoft changed the title ~~WIP: Deployment: add end-of-life~~ WIP:Deployment: add end-of-life Jul 21, 2017

peterbaouoft force-pushed the end_of_life_notification branch 4 times, most recently from 8351279 to bb42b00 Compare July 24, 2017 18:21

jlebon reviewed Jul 24, 2017

View reviewed changes

peterbaouoft changed the title ~~WIP:Deployment: add end-of-life~~ Deployment: add end-of-life Jul 24, 2017

peterbaouoft force-pushed the end_of_life_notification branch from bb42b00 to b91ad0c Compare July 24, 2017 21:45

jlebon reviewed Jul 25, 2017

View reviewed changes

jlebon reviewed Jul 26, 2017

View reviewed changes

peterbaouoft force-pushed the end_of_life_notification branch from 634630d to cc63bcb Compare July 26, 2017 21:34

Deployment: add endoflife

50afe9f

When commit metadata contains ostree.endoflife attribute, its information will be added to the deployment Variant, which will later be shown as a red & bold message when 'rpm-ostree status' command is called. A test is added for future regression

peterbaouoft force-pushed the end_of_life_notification branch from 26a380e to 50afe9f Compare July 27, 2017 22:23

jlebon reviewed Jul 28, 2017

View reviewed changes

cgwalters reviewed Jul 28, 2017

View reviewed changes

peterbaouoft force-pushed the end_of_life_notification branch from c9fbb60 to 7889a9e Compare July 28, 2017 17:17

fixup! Deployment: add endoflife

467c40d

peterbaouoft force-pushed the end_of_life_notification branch from 7889a9e to 467c40d Compare July 28, 2017 17:18

cgwalters added the reviewed/approved label Aug 2, 2017

rh-atomic-bot closed this in 53c3963 Aug 2, 2017

peterbaouoft deleted the end_of_life_notification branch June 20, 2018 14:26

cgwalters mentioned this pull request Sep 4, 2018

end-of-life notification #142

Closed

jlebon mentioned this pull request Apr 28, 2021

Update-Flow Breakpoints / Required Updates #2788

Open

Deployment: add end-of-life #889

Deployment: add end-of-life #889

Conversation

peterbaouoft commented Jul 21, 2017 • edited Loading

peterbaouoft Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbaouoft commented Jul 21, 2017

peterbaouoft commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbaouoft commented Jul 21, 2017

peterbaouoft commented Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Jul 24, 2017

peterbaouoft commented Jul 25, 2017

peterbaouoft commented Jul 25, 2017

peterbaouoft commented Jul 25, 2017

peterbaouoft commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbaouoft commented Jul 25, 2017 • edited Loading

cgwalters commented Jul 25, 2017

peterbaouoft commented Jul 26, 2017

Choose a reason for hiding this comment

peterbaouoft Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

jlebon commented Jul 27, 2017

peterbaouoft commented Jul 27, 2017

peterbaouoft commented Jul 27, 2017 • edited Loading

jlebon commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbaouoft Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Jul 28, 2017

cgwalters commented Aug 2, 2017

rh-atomic-bot commented Aug 2, 2017

rh-atomic-bot commented Aug 2, 2017

dustymabe commented Sep 5, 2018

cgwalters commented Sep 5, 2018

cgwalters commented Sep 5, 2018

dustymabe commented Sep 5, 2018

peterbaouoft commented Jul 21, 2017 •

edited

Loading

peterbaouoft Jul 21, 2017 •

edited

Loading

peterbaouoft commented Jul 21, 2017 •

edited

Loading

peterbaouoft commented Jul 25, 2017 •

edited

Loading

peterbaouoft Jul 26, 2017 •

edited

Loading

peterbaouoft commented Jul 27, 2017 •

edited

Loading

peterbaouoft Jul 28, 2017 •

edited

Loading