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

Deployment: add end-of-life #889

Closed

Conversation

peterbaouoft
Copy link
Contributor

@peterbaouoft peterbaouoft commented Jul 21, 2017

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

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

@peterbaouoft peterbaouoft Jul 21, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

related to #142

@peterbaouoft
Copy link
Contributor Author

@cgwalters any thoughts? Thanks in advance :D

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

Choose a reason for hiding this comment

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

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.

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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


if (end_of_life_string)
{
g_print("%s", end_of_life_string);
Copy link
Member

Choose a reason for hiding this comment

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

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


if (end_of_life_string)
{
g_print("%s", end_of_life_string);
Copy link
Member

Choose a reason for hiding this comment

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

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

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

bot, retest this please

@peterbaouoft
Copy link
Contributor Author

peterbaouoft commented Jul 21, 2017

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)

@peterbaouoft peterbaouoft reopened this Jul 21, 2017
@peterbaouoft peterbaouoft changed the title Deployment: add end-of-life WIP:Deployment: add end-of-life Jul 21, 2017
@peterbaouoft peterbaouoft changed the title WIP:Deployment: add end-of-life "WIP: "Deployment: add end-of-life Jul 21, 2017
@peterbaouoft peterbaouoft changed the title "WIP: "Deployment: add end-of-life WIP: Deployment: add end-of-life Jul 21, 2017
@peterbaouoft peterbaouoft changed the title WIP: Deployment: add end-of-life WIP:Deployment: add end-of-life Jul 21, 2017
@peterbaouoft peterbaouoft force-pushed the end_of_life_notification branch 4 times, most recently from 8351279 to bb42b00 Compare July 24, 2017 18:21
base_checksum = g_strdup (csum);
{
base_checksum = g_strdup (csum);
/* we would not consider endoflife attribute for layered commits */
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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
Copy link
Member

jlebon commented Jul 24, 2017

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

@peterbaouoft peterbaouoft changed the title WIP:Deployment: add end-of-life Deployment: add end-of-life Jul 24, 2017
@peterbaouoft
Copy link
Contributor Author

bot, retest this please

3 similar comments
@peterbaouoft
Copy link
Contributor Author

bot, retest this please

@peterbaouoft
Copy link
Contributor Author

bot, retest this please

@peterbaouoft
Copy link
Contributor Author

bot, retest this please

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

Choose a reason for hiding this comment

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

Nit: space between if and (.

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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

base_checksum = g_strdup (csum);
{
base_checksum = g_strdup (csum);
/* we would not consider endoflife attribute for layered commits */
Copy link
Member

Choose a reason for hiding this comment

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

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. :)

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

Choose a reason for hiding this comment

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

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!

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@peterbaouoft
Copy link
Contributor Author

peterbaouoft commented Jul 25, 2017

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
Copy link
Member

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

@peterbaouoft
Copy link
Contributor Author

bot, retest this please

g_variant_dict_lookup (dict, "endoflife", "&s", &end_of_life_string);

if (end_of_life_string)
g_print ("\nEndOfLife: %s", end_of_life_string);
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@peterbaouoft peterbaouoft Jul 26, 2017

Choose a reason for hiding this comment

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

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
Copy link
Member

jlebon commented Jul 27, 2017

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
Copy link
Contributor Author

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

@peterbaouoft
Copy link
Contributor Author

peterbaouoft commented Jul 27, 2017

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
Copy link
Member

jlebon commented Jul 27, 2017

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
# 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}\""
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sure :)

@jlebon
Copy link
Member

jlebon commented Jul 28, 2017

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

@@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

# 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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@peterbaouoft peterbaouoft Jul 28, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I see, that makes sense :)

@cgwalters
Copy link
Member

bot, add author to whitelist

@cgwalters
Copy link
Member

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
Copy link

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

@rh-atomic-bot
Copy link

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

@peterbaouoft peterbaouoft deleted the end_of_life_notification branch June 20, 2018 14:26
@dustymabe
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

thanks! that really helps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants