Adjust min-free-space-percent config option to accept specfic size #1616

uajain · 2018-06-07T16:36:49Z

No description provided.

cgwalters · 2018-06-07T19:14:27Z

src/libostree/ostree-repo-commit.c

+
+      char unit = dup_str[i];
+      dup_str[i] = '\0';
+      gint size = g_ascii_strtoull (dup_str, NULL, 10);


guint64 again.

cgwalters · 2018-06-07T19:15:31Z

src/libostree/ostree-repo-private.h

@@ -150,7 +150,7 @@ struct OstreeRepo {
  uid_t owner_uid; /* Cache of repo's owner uid */
  uid_t target_owner_uid; /* Ensure files are chowned to this uid/gid */
  gid_t target_owner_gid;
-  guint min_free_space_percent; /* See the min-free-space-percent config option */
+  const char *min_free_space_str; /* See the min-free-space config option */


I'm in favor of adding min-free-space as an alias for min-free-space-percent, but AFAICS this patch isn't doing that yet.

cgwalters · 2018-06-07T19:15:59Z

src/libostree/ostree-repo.c

+min_free_space_str_validate (char *min_free_space_str)
+{
+  GRegex *percent_regex = g_regex_new ("^([0-9]|[1-9][0-9])%$" , 0,0, NULL);
+  GRegex *size_regex = g_regex_new ("^[0-9]+(G|M|T)B$", 0,0, NULL);


g_autoptr() here - these are leaked currently. (Also we tend to use g_once_init...() for regexps, see elsewhere in the code)

cgwalters · 2018-06-07T19:16:49Z

src/libostree/ostree-repo.c

    /* If changing this, be sure to change the man page too */
-    const char *default_min_free_space = "3";
+    const char *default_min_free_space = "3%";


Well...but we have a backcompat hazard here as we didn't previously require %. So perhaps it is best to introduce a new config option.

Or interpret a bare number as a percentage for the back compat case?

@ramcq Won't it give a wrong idea? For e.g.
(i) min-free-space-percent=3
(ii) min-free-space-percent=150MB
or am I missing something?

I wonder if adding an alias and introducing new config option are same thing? Do we have an exisitng alias case which I can look-up?

I wonder if adding an alias and introducing new config option are same thing

They're different. I was leaning towards an alias at first, but I think we need to add a new option to sanely differentiate. We could in theory require G|M|T for sizes but...it's ugly to have the option named -percent when it's not.

If we have two options, an interesting question arises if we want to support setting both simultaneously - I'd be fine trying to do so, I'd also be fine making it an error for now.

I want to be clear here: Is this just an idea of yours or are you planning to actually ship a non-zero min-free-space in EndlessOS?

If we have two options, an interesting question arises if we want to support setting both simultaneously - I'd be fine trying to do so, I'd also be fine making it an error for now.

I think, making it an error sounds logical to me here.

I want to be clear here: Is this just an idea of yours or are you planning to actually ship a non-zero min-free-space in EndlessOS?

It's not just an idea but a step in direction of #1602 (comment) . Also, min-free-space (% or specific size) should guard against possibility of hitting ENOSPC. So yes, Endless would be very much interesting in this as it provides a bit more fine tuning of the config option.

Endless certainly wants to ship something that does this kind of thing.

@cgwalters Quick question: If both min-free-space and min-free-space-percent are specified in the config file(for some odd reasons), do we error that out, or treat one of them as default (say min-free-space) and proceed ahead with discarding the other one ?

cgwalters · 2018-06-07T19:21:26Z

src/libostree/ostree-repo.c

@@ -2771,18 +2784,22 @@ reload_core_config (OstreeRepo          *self,
      self->zlib_compression_level = OSTREE_ARCHIVE_DEFAULT_COMPRESSION_LEVEL;
  }

-  { g_autofree char *min_free_space_percent_str = NULL;
+  { g_autofree char *min_free_space_str = NULL;
+    self->min_free_space_str = NULL;


g_clear_pointer (&self->min_free_space_str, g_free)

cgwalters · 2018-06-07T19:22:04Z

src/libostree/ostree-repo.c

-    if (self->min_free_space_percent > 99)
-      return glnx_throw (error, "Invalid min-free-space-percent '%s'", min_free_space_percent_str);
+   if (min_free_space_str_validate (min_free_space_str))
+     self->min_free_space_str = g_strdup (min_free_space_str);


self->min_free_space_str = g_steal_pointer (&min_free_space_str); and avoid an extra strdup.

cgwalters · 2018-06-07T19:23:15Z

src/libostree/ostree-repo.c

+  GRegex *size_regex = g_regex_new ("^[0-9]+(G|M|T)B$", 0,0, NULL);
+
+  if (g_regex_match (percent_regex, min_free_space_str, 0, NULL) ||
+      g_regex_match (size_regex, min_free_space_str, 0, NULL))


If we're going to use regexps it feels like we might as well use capture groups to get the data out, then min_free_space_str_parse() would just need to handle a (G|M|T)B suffix.

cgwalters · 2018-06-07T19:34:09Z

src/libostree/ostree-repo-commit.c

+            break;
+          default:
+                g_warning ("Should not be reached, min_free_space_parse\n");
+	}


Indentation seems off in various places like this.

cgwalters · 2018-06-07T19:35:21Z

Also can you add commit messages to your commits?

Similar to min-free-space-percent but it supports specific sizes (in MB, GB or TB). Also, making min-free-space-percent and -size mutually exclusive. min-free-space-percent does not give a fine tuning of the free disk space that a user might decide to keep. It can translate to very large size (e.g. 1% = ~10GB on 1TB HDD) or very small (e.g. 1% = ~330MB on 32GB system like Endless devices). Hence, it makes sense to introduce a config option to honor specific size as per the user.

uajain · 2018-06-12T14:28:06Z

Updated the PR. I have kept the same min-free-space-percent option as default (instead of min-free-space-size) for now; But in near future, it makes sense to keep min-free-space-size as default. Also, it needs some discussion to decide on the default value.

jlebon · 2018-06-13T18:57:35Z

This looks sane to me. I added a test to itest-pull-space.sh as well.
@rh-atomic-bot r+ b7729cd

rh-atomic-bot · 2018-06-13T18:57:40Z

⌛ Testing commit b7729cd with merge 31809d3...

rh-atomic-bot · 2018-06-13T19:36:41Z

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 31809d3 to master...

cgwalters reviewed Jun 7, 2018

View reviewed changes

uajain force-pushed the min-free-space-adjust branch from a752d3c to 665b1d2 Compare June 12, 2018 14:22

fixup! lib/repo: Add min-free-space-size option

b7729cd

jlebon added the homu/approved label Jun 13, 2018

rh-atomic-bot closed this in 31809d3 Jun 13, 2018

uajain deleted the min-free-space-adjust branch June 13, 2018 20:12

uajain mentioned this pull request Jun 13, 2018

Cleanup current boot's staging dir if we hit min-free-space-* condition #1602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust min-free-space-percent config option to accept specfic size #1616

Adjust min-free-space-percent config option to accept specfic size #1616

uajain commented Jun 7, 2018

cgwalters Jun 7, 2018

cgwalters Jun 7, 2018

cgwalters Jun 7, 2018

cgwalters Jun 7, 2018

ramcq Jun 7, 2018

uajain Jun 7, 2018

cgwalters Jun 8, 2018

uajain Jun 8, 2018

nedrichards Jun 11, 2018

uajain Jun 11, 2018

cgwalters Jun 7, 2018

cgwalters Jun 7, 2018

cgwalters Jun 7, 2018

cgwalters Jun 7, 2018

cgwalters commented Jun 7, 2018

uajain commented Jun 12, 2018

jlebon commented Jun 13, 2018

rh-atomic-bot commented Jun 13, 2018

rh-atomic-bot commented Jun 13, 2018

Adjust min-free-space-percent config option to accept specfic size #1616

Adjust min-free-space-percent config option to accept specfic size #1616

Conversation

uajain commented Jun 7, 2018

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

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

cgwalters commented Jun 7, 2018

uajain commented Jun 12, 2018

jlebon commented Jun 13, 2018

rh-atomic-bot commented Jun 13, 2018

rh-atomic-bot commented Jun 13, 2018