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

Add --feed-lock-timeout option #1472

Merged
merged 4 commits into from
Apr 7, 2021

Conversation

timopollmeier
Copy link
Member

@timopollmeier timopollmeier commented Apr 1, 2021

What:
This new option sets a number of seconds to retry for if the feed is
locked in contexts (like migration or rebuilds) that do not retry on
their own (like automatic syncs).

Why:
This is intended to help automated upgrades of GVM modules where
ospd-openvas needs to be restarted, which locks the feed while the
VTs are reloaded.

How did you test it:
By doing the following:

  • locking the feed lockfile and running gvmd --rebuild-scap - this should fail immediately.
  • retrying with gvmd --rebuild-scap --feed-lock-timeout 15 with the lock still active - this should generate a gvmd log message "Feed is currently locked..." and fail about 15 seconds later.
  • running gvmd --rebuild-scap --feed-lock-timeout 15 with the lock still active and releasing the lock while gvmd is waiting - this should leave a "Feed is currently locked..." gvmd log message and start the rebuild once the lock is released.
  • running gvmd --rebuild-scap --feed-lock-timeout 15 without locking - this should start the rebuild immediately without a "Feed is currently locked..." log message.

Checklist:

This new option sets a number of seconds to retry for if the feed is
locked in contexts (like migration or rebuilds) that do not retry on
their own (like automatic syncs).
This is intended to help automated upgrades of GVM modules where
ospd-openvas needs to be restarted, which locks the feed while the
VTs are reloaded.
@timopollmeier timopollmeier marked this pull request as ready for review April 1, 2021 13:13
@timopollmeier timopollmeier requested a review from a team as a code owner April 1, 2021 13:13
@timopollmeier timopollmeier added the backport-to-main This pull request will be ported to the master branch label Apr 1, 2021
Comment on lines -185 to -187
\fB--slave-commit-size=\fINUMBER\fB\f1
During slave updates, commit after every NUMBER updated results and hosts, 0 for unlimited.
.TP
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this been removed by accident or on purpose? Maybe it belongs to another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

This option is supposed to be removed. I think it was added back in by accident in a backport or it was just forgotten before.

@@ -6345,7 +6419,7 @@ gvm_migrate_secinfo (int feed_type)
return -1;
}

ret = feed_lockfile_lock (&lockfile);
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the old function? Is is still used or can it be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The old function is still used in the automatic sync that has essentially its own retry loop.
I've also changed feed_lockfile_lock_timeout to use it now so writing the timestamp doesn't have to be duplicated.

src/manage.c Outdated
Comment on lines 6048 to 6064
ret = lockfile_lock_path_nb (lockfile, get_feed_lock_path ());
if (ret == 1 && timeout_end > time (NULL))
{
if (log_timeout)
{
log_timeout = FALSE;
g_message ("%s: Feed is currently locked by another process,"
" will retry until %s.",
__func__, iso_time (&timeout_end));
}
gvm_sleep (1);
}
else if (ret)
{
return ret;
}
} while (ret);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the meaning of ret here? 1 == locked? If yes maybe you could rename it to somthing like is_locked.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've renamed the variable and added a few comments about the conditions.

src/gvmd.c Outdated
&feed_lock_timeout,
"Sets the number of seconds to retry for if the feed is locked"
" in contexts (like migration or rebuilds) that do not retry"
" on their own (like automatic syncs). Defaults to 0.",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would explain the meaning of 0 too (i guess it is nowait).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the text to mention that the default 0 means there is no retry.

The feed_lockfile_lock function is used to avoid duplicating the code to
write the timestamp. Also, the return value checks are clarified.
The options help and man page now mention that the value 0 means
no retry.
@timopollmeier timopollmeier merged commit 99c2cf4 into greenbone:gvmd-21.04 Apr 7, 2021
timopollmeier added a commit that referenced this pull request Apr 9, 2021
@timopollmeier timopollmeier added the backport-to-oldstable This pull request will be backported to the oldstable branch label Apr 9, 2021
@timopollmeier
Copy link
Member Author

@Mergifyio refresh

@timopollmeier
Copy link
Member Author

@Mergifyio backport gvmd-20.08

@mergify
Copy link
Contributor

mergify bot commented Apr 9, 2021

Command refresh: success

Pull request refreshed

@mergify
Copy link
Contributor

mergify bot commented Apr 9, 2021

Command backport gvmd-20.08: success

Backports have been created

timopollmeier added a commit that referenced this pull request Apr 9, 2021
This fixes the changelog by putting the entry in the 20.08.2 section
and adds the slave-commit-size option back to the man page.
bjoernricks added a commit that referenced this pull request Apr 13, 2021
@timopollmeier timopollmeier deleted the feed-lock-timeout branch October 15, 2021 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-main This pull request will be ported to the master branch backport-to-oldstable This pull request will be backported to the oldstable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants